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

[Derek Lesho]

On 2020-03-20 07:16, Nikolay Sivov wrote:
> From: Derek Lesho
> Signed-off-by: Nikolay Sivov
> - 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.
> -
>
> 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.
> - {
> -
>    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.
> -
> - 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;