[v3 PATCH 2/2] mfreadwrite: Pass source samples through decoder as they arrive.

[Nikolay Sivov]

On 3/20/20 6:00 PM, Derek Lesho wrote:
> On 2020-03-20 07:16, Nikolay Sivov wrote:
>
>> From: Derek Lesho <dlesho at codeweavers.com>
>>
>> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
>> ---
>>
>> - moved decoding calls to event handler, queue now always contains 
>> processed samples;
>> - removed draining logic for now, I believe we can do that explicitly 
>> on ReadSample() or
>>    on EndOfStream instead. Same helper could be used for sync/async 
>> read and EOS event;
> FWIW, the draining logic is crucial, as every h.264 decoder I'm aware 
> of won't output the most recently sent samples until a drain is 
> activated.  In the case of the Microsoft H.264 decoder, 20 samples are 
> buffered until drain is called, and with the openh264 decoder, it's ~3 
> samples.  With the current logic, the application will get the EOS 
> flag well before all the samples have run out, and the decoder will 
> have a good amount of uncompressed samples just sitting there.
Makes sense, I'll do that then.
>> - added a check for output transform flags;
>>
>>   dlls/mfreadwrite/main.c | 81 +++++++++++++++++++++++++++++++++++------
>>   1 file changed, 69 insertions(+), 12 deletions(-)
>>
>> diff --git a/dlls/mfreadwrite/main.c b/dlls/mfreadwrite/main.c
>> index d3c10a4d11..052575c100 100644
>> --- a/dlls/mfreadwrite/main.c
>> +++ b/dlls/mfreadwrite/main.c
>> @@ -366,6 +366,71 @@ static ULONG WINAPI 
>> source_reader_stream_events_callback_Release(IMFAsyncCallbac
>>       return IMFSourceReader_Release(&reader->IMFSourceReader_iface);
>>   }
>>   +static void source_reader_queue_sample(struct media_stream 
>> *stream, IMFSample *sample)
>> +{
>> +    struct sample *pending_sample;
>> +
>> +    if (!sample)
>> +        return;
>> +
>> +    pending_sample = heap_alloc(sizeof(*pending_sample));
>> +    pending_sample->sample = sample;
>> +    IMFSample_AddRef(pending_sample->sample);
>> +
>> +    list_add_tail(&stream->samples, &pending_sample->entry);
>> +}
>> +
>> +static HRESULT source_reader_process_sample(struct media_stream 
>> *stream, IMFSample *sample)
>> +{
>> +    MFT_OUTPUT_STREAM_INFO stream_info = { 0 };
>> +    MFT_OUTPUT_DATA_BUFFER out_buffer;
>> +    DWORD status;
>> +    HRESULT hr;
>> +
>> +    if (!stream->decoder)
>> +    {
>> +        source_reader_queue_sample(stream, sample);
>> +        return S_OK;
>> +    }
>> +
>> +    /* It's assumed that decoder has 1 input and 1 output, both id's 
>> are 0. */
>> +
>> +    if (FAILED(hr = 
>> IMFTransform_GetOutputStreamInfo(stream->decoder, 0, &stream_info)))
>> +    {
>> +        WARN("Failed to get output stream info, hr %#x.\n", hr);
>> +        return hr;
>> +    }
>> +
>> +    if (!(stream_info.dwFlags & MFT_OUTPUT_STREAM_PROVIDES_SAMPLES))
> This can probably be changed to MFT_OUTPUT_STREAM_CAN_PROVIDE_SAMPLES, 
> not too important though.
I remember testing this specifically for session logic, and as I 
remember it responded only on PROVIDES, I'll take another look.
>> +    {
>> +        FIXME("Transform does not provide samples.\n");
>> +        return E_NOTIMPL;
>> +    }
>> +
>> +    while (hr == S_OK)
>> +    {
>> +        memset(&out_buffer, 0, sizeof(out_buffer));
>> +        if (SUCCEEDED(hr = 
>> IMFTransform_ProcessOutput(stream->decoder, 0, 1, &out_buffer, 
>> &status)))
>> +        {
>> +            source_reader_queue_sample(stream, out_buffer.pSample);
>> +            if (out_buffer.pSample)
>> +                IMFSample_Release(out_buffer.pSample);
>> +            if (out_buffer.pEvents)
>> +                IMFCollection_Release(out_buffer.pEvents);
>> +        }
>> +    }
>> +
>> +    if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT)
>> +    {
>> +        if (FAILED(hr = IMFTransform_ProcessInput(stream->decoder, 
>> 0, sample, 0)))
>> +            WARN("Transform failed to process input, hr %#x.\n", hr);
>> +    }
>> +    else
>> +        WARN("Transform failed to process output, hr %#x.\n", hr);
> This logic is wrong, there will always be a leftover sample, since you 
> don't call ProcessOutput after giving the transform the incoming 
> sample from the source.  The way this is supposed to work is that you 
> first empty any pending outputs, to ensure that the MFT has space for 
> more input, then input the sample, then redo the ProcessOutput step.  
> Additionally, since we're buffering all the output samples, we could 
> simplify this and just call ProcessInput first, since it should never 
> return MF_E_NOTACCEPTING.
I see, so failed ProcessInput() would mean early return.
>> +
>> +    return hr;
>> +}
>> +
>>   static HRESULT source_reader_media_sample_handler(struct 
>> source_reader *reader, IMFMediaStream *stream,
>>           IMFMediaEvent *event)
>>   {
>> @@ -393,21 +458,14 @@ static HRESULT 
>> source_reader_media_sample_handler(struct source_reader *reader,
>>       {
>>           if (id == reader->streams[i].id)
>>           {
>> -            struct sample *pending_sample;
>> -
>> -            if (!(pending_sample = 
>> heap_alloc(sizeof(*pending_sample))))
>> -            {
>> -                hr = E_OUTOFMEMORY;
>> -                goto failed;
>> -            }
>> + EnterCriticalSection(&reader->streams[i].cs);
>>   -            pending_sample->sample = sample;
>> -            IMFSample_AddRef(pending_sample->sample);
>> +            hr = source_reader_process_sample(&reader->streams[i], 
>> sample);
>>   - EnterCriticalSection(&reader->streams[i].cs);
>> -            list_add_tail(&reader->streams[i].samples, 
>> &pending_sample->entry);
>> LeaveCriticalSection(&reader->streams[i].cs);
>>   +            /* FIXME: propagate processing errors? */
>> +
>> WakeAllConditionVariable(&reader->streams[i].sample_event);
>>                 break;
>> @@ -417,7 +475,6 @@ static HRESULT 
>> source_reader_media_sample_handler(struct source_reader *reader,
>>       if (i == reader->stream_count)
>>           WARN("Stream with id %#x was not present in presentation 
>> descriptor.\n", id);
>>   -failed:
>>       IMFSample_Release(sample);
>>         return hr;