[PATCH v1 1/3] mfreadwrite: Find a decoder in IMFSourceReader::SetCurrentMediaType.

[Derek Lesho]

Signed-off-by: Derek Lesho 
---
 dlls/mfreadwrite/main.c | 192 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 188 insertions(+), 4 deletions(-)

diff --git a/dlls/mfreadwrite/main.c b/dlls/mfreadwrite/main.c
index 56ec036089..bc5bf0fde9 100644
--- a/dlls/mfreadwrite/main.c
+++ b/dlls/mfreadwrite/main.c
@@ -94,6 +94,7 @@ struct media_stream
 {
     IMFMediaStream *stream;
     IMFMediaType *current;
+    IMFTransform *decoder;
     DWORD id;
     CRITICAL_SECTION cs;
     CONDITION_VARIABLE sample_event;
@@ -576,6 +577,8 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface)
                 IMFMediaStream_Release(stream->stream);
             if (stream->current)
                 IMFMediaType_Release(stream->current);
+            if (stream->decoder)
+                IMFTransform_Release(stream->decoder);
             DeleteCriticalSection(&stream->cs);

LIST_FOR_EACH_ENTRY_SAFE(ptr, next, &stream->samples, struct sample, entry)
@@ -755,6 +758,15 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
         IMFMediaType *type)
 {
     struct source_reader *reader = impl_from_IMFSourceReader(iface);
+    DWORD equal_flags;
+    GUID major_type, target_subtype, decoder_category;
+    IMFStreamDescriptor *stream_descriptor = NULL;
+    BOOL selected;
+    IMFMediaTypeHandler *stream_type_handler = NULL;
+    IMFMediaType *in_stream_type = NULL, *potential_out_stream_type = NULL;
+    CLSID decoder_id;
+    BOOL decoder_found = FALSE;
+    IMFTransform *decoder = NULL;
     HRESULT hr;

TRACE("%p, %#x, %p, %p.\n", iface, index, reserved, type);
@@ -774,14 +786,186 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
     if (index >= reader->stream_count)
         return MF_E_INVALIDSTREAMNUMBER;

-    /* FIXME: validate passed type and current presentation state. */
-
     EnterCriticalSection(&reader->cs);

-    hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
+    if (FAILED(hr = IMFMediaType_IsEqual(type, reader->streams[index].current, &equal_flags)))
+        goto done;

-    LeaveCriticalSection(&reader->cs);
+    if (!(equal_flags & MF_MEDIATYPE_EQUAL_MAJOR_TYPES))
+    {
+        /* Not gonna happen */
+        hr = MF_E_INVALIDMEDIATYPE;
+        goto done;
+    }
+
+    if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
+    {
+        /* Similar enough not to need to refind a decoder */
+        hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
+        goto done;
+    }
+
+    if (FAILED(hr = IMFMediaType_GetMajorType(type, &major_type)))
+        goto done;
+
+    if (FAILED(hr = IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &target_subtype)))
+        goto done;
+
+    if (FAILED(hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(reader->descriptor, index, &selected, &stream_descriptor)))
+        goto done;
+
+    if (FAILED(hr = IMFStreamDescriptor_GetMediaTypeHandler(stream_descriptor, &stream_type_handler)))
+        goto done;
+
+    /* first, check if it's a native type */
+    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
+    {
+        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
+            continue;
+
+        if (FAILED(hr = IMFMediaType_IsEqual(type, in_stream_type, &equal_flags)))
+            goto done;
+
+        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
+        {
+            if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
+                goto done;
+            hr = IMFMediaType_CopyAllItems(in_stream_type, (IMFAttributes *)reader->streams[index].current);
+            goto done;
+        }
+
+        IMFMediaType_Release(in_stream_type);
+        in_stream_type = NULL;
+    }
+
+    /* TODO: should we check if the source type is compressed? */
+
+    if (IsEqualGUID(&major_type, &MFMediaType_Video))
+    {
+        decoder_category = MFT_CATEGORY_VIDEO_DECODER;
+    }
+    else if (IsEqualGUID(&major_type, &MFMediaType_Audio))
+    {
+        decoder_category = MFT_CATEGORY_AUDIO_DECODER;
+    }
+    else
+    {
+        hr = MF_E_TOPO_CODEC_NOT_FOUND;
+        goto done;
+    }
+
+    hr = S_OK;
+    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
+    {
+        GUID source_subtype;
+        MFT_REGISTER_TYPE_INFO in_type, out_type;
+        CLSID *decoder_ids;
+        ULONG decoder_count;
+
+        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
+            continue;
+
+        if (FAILED(hr = IMFMediaType_GetGUID(in_stream_type, &MF_MT_SUBTYPE, &source_subtype)))
+            goto done;
+
+        in_type.guidMajorType = major_type;
+        in_type.guidSubtype = source_subtype;
+
+        out_type.guidMajorType = major_type;
+        out_type.guidSubtype = target_subtype;

+        if (FAILED(hr = MFTEnum(decoder_category, 0, &in_type, &out_type, NULL, &decoder_ids, &decoder_count)) || decoder_count == 0)
+        {
+            if (!(decoder_found))
+            {
+                /* see if there are other decoders for this stream */
+                if (SUCCEEDED(MFTEnum(decoder_category, 0, &in_type, NULL, NULL, &decoder_ids, &decoder_count)) && decoder_count > 0)
+                {
+                    decoder_found = TRUE;
+                    CoTaskMemFree(decoder_ids);
+                }
+            }
+
+            IMFMediaType_Release(in_stream_type);
+            in_stream_type = NULL;
+            continue;
+        }
+
+        if (decoder_count > 1)
+            FIXME("Just using the first decoder.\n");
+        decoder_id = decoder_ids[0];
+        CoTaskMemFree(decoder_ids);
+
+        if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
+            goto done;
+
+        break;
+    }
+
+    if (!in_stream_type)
+    {
+        ERR("Failed to find decoder matching request, source has decoder: %u.\n", decoder_found);
+        hr = decoder_found ? MF_E_INVALIDREQUEST : MF_E_TOPO_CODEC_NOT_FOUND;
+        goto done;
+    }
+
+    /* we have the decoder id at this point */
+    if (FAILED(hr = CoCreateInstance(&decoder_id, NULL, CLSCTX_INPROC_SERVER, &IID_IMFTransform, (void**)&decoder)))
+        goto done;
+
+    {
+        DWORD in_min, in_max, out_min, out_max;
+
+        if (FAILED(hr = IMFTransform_GetStreamLimits(decoder, &in_min, &in_max, &out_min, &out_max)))
+            goto done;
+        if (in_min != 1 || in_max != 1 || out_min != 1 || out_max != 1)
+        {
+            hr = ERROR_INTERNAL_ERROR;
+            goto done;
+        }
+    }
+
+    if (FAILED(hr = IMFTransform_SetInputType(decoder, 0, in_stream_type, 0)))
+        goto done;
+
+    /* find the relevant output type */
+    for (unsigned int i = 0;;i++)
+    {
+        if (FAILED(hr = IMFTransform_GetOutputAvailableType(decoder, 0, i, &potential_out_stream_type)))
+            goto done;
+
+        if (FAILED(hr = IMFMediaType_IsEqual(type, potential_out_stream_type, &equal_flags)))
+            goto done;
+
+        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
+        {
+            if (FAILED(hr = IMFTransform_SetOutputType(decoder, 0, potential_out_stream_type, 0)))
+                goto done;
+
+            if (FAILED(hr = IMFMediaType_CopyAllItems(potential_out_stream_type, (IMFAttributes *)reader->streams[index].current)))
+                goto done;
+
+            break;
+        }
+        IMFMediaType_Release(potential_out_stream_type);
+        potential_out_stream_type = NULL;
+    }
+
+    reader->streams[index].decoder = decoder;
+    decoder = NULL;
+
+    done:
+    LeaveCriticalSection(&reader->cs);
+    if (stream_descriptor)
+        IMFStreamDescriptor_Release(stream_descriptor);
+    if (stream_type_handler)
+        IMFMediaTypeHandler_Release(stream_type_handler);
+    if (in_stream_type)
+        IMFMediaType_Release(in_stream_type);
+    if (decoder)
+        IMFTransform_Release(decoder);
+    if (potential_out_stream_type)
+        IMFMediaType_Release(potential_out_stream_type);
     return hr;
 }

[PATCH v1 2/3] mfreadwrite: Decode source's samples in IMFSourceReader::ReadSample.

[Derek Lesho]

Signed-off-by: Derek Lesho 
---
 dlls/mfreadwrite/main.c | 111 +++++++++++++++++++++++++++++++---------
 1 file changed, 87 insertions(+), 24 deletions(-)

diff --git a/dlls/mfreadwrite/main.c b/dlls/mfreadwrite/main.c
index bc5bf0fde9..7ebfa0cc21 100644
--- a/dlls/mfreadwrite/main.c
+++ b/dlls/mfreadwrite/main.c
@@ -988,7 +988,7 @@ static HRESULT WINAPI src_reader_SetCurrentPosition(IMFSourceReader *iface, REFG
     return IMFMediaSource_Start(reader->source, reader->descriptor, format, position);
 }

-static IMFSample *media_stream_pop_sample(struct media_stream *stream, DWORD *stream_flags)
+static IMFSample *media_stream_pop_sample(struct media_stream *stream)
 {
     IMFSample *ret = NULL;
     struct list *head;
@@ -1001,8 +1001,6 @@ static IMFSample *media_stream_pop_sample(struct media_stream *stream, DWORD *st
         heap_free(pending_sample);
     }

-    *stream_flags = stream->state == STREAM_STATE_EOS ? MF_SOURCE_READERF_ENDOFSTREAM : 0;
-
     return ret;
 }

@@ -1041,6 +1039,88 @@ static HRESULT source_reader_start_source(struct source_reader *reader)
     return hr;
 }

+static IMFSample *next_source_sample(struct media_stream *stream)
+{
+    IMFSample *ret;
+    HRESULT hr;
+
+    EnterCriticalSection(&stream->cs);
+
+    while (list_empty(&stream->samples) && stream->state != STREAM_STATE_EOS)
+    {
+        if (stream->stream)
+        {
+            if (FAILED(hr = IMFMediaStream_RequestSample(stream->stream, NULL)))
+                WARN("Sample request failed, hr %#x.\n", hr);
+        }
+        SleepConditionVariableCS(&stream->sample_event, &stream->cs, INFINITE);
+    }
+
+    ret = media_stream_pop_sample(stream);
+
+    LeaveCriticalSection(&stream->cs);
+
+    return ret;
+}
+
+static HRESULT next_sample(struct media_stream *stream, IMFSample **out_sample, DWORD *stream_flags, BOOL drain)
+{
+    HRESULT hr = S_OK;
+
+    *stream_flags = 0;
+    *out_sample = NULL;
+
+    if (stream->decoder)
+    {
+        DWORD status;
+        MFT_OUTPUT_DATA_BUFFER out_buffer = {};
+
+        out_buffer.dwStreamID = 0;
+        for(;;)
+        {
+            if (list_empty(&stream->samples) && (stream->state == STREAM_STATE_EOS || drain))
+            {
+                hr = IMFTransform_ProcessMessage(stream->decoder, MFT_MESSAGE_COMMAND_DRAIN, 0);
+                if (FAILED(hr))
+                    return hr;
+            }
+
+            hr = IMFTransform_ProcessOutput(stream->decoder, 0, 1, &out_buffer, &status);
+            if (hr == S_OK)
+            {
+                *out_sample = out_buffer.pSample;
+                return hr;
+            }
+            else if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT)
+            {
+                IMFSample *next_sample = drain ? media_stream_pop_sample(stream) : next_source_sample(stream);
+                if (!next_sample)
+                {
+                    *stream_flags = MF_SOURCE_READERF_ENDOFSTREAM;
+                    return S_OK;
+                }
+
+                hr = IMFTransform_ProcessInput(stream->decoder, 0, next_sample, 0);
+                if (hr == MF_E_NOTACCEPTING)
+                    ERR("ProcessInput returned MF_E_NOTACCEPTING after ProcessOutput returned MF_E_TRANSFORM_NEED_MORE_INPUT\n");
+                if (hr != S_OK)
+                    return hr;
+            }
+            else
+            {
+                return hr;
+            }
+        }
+    }
+    else
+    {
+        *out_sample = drain ? media_stream_pop_sample(stream) : next_source_sample(stream);
+        if (stream->state == STREAM_STATE_EOS)
+            *stream_flags = MF_SOURCE_READERF_ENDOFSTREAM;
+        return S_OK;
+    }
+}
+
 static HRESULT source_reader_read_sample(struct source_reader *reader, DWORD index, DWORD flags, DWORD *actual_index,
         DWORD *stream_flags, LONGLONG *timestamp, IMFSample **sample)
 {
@@ -1087,31 +1167,14 @@ static HRESULT source_reader_read_sample(struct source_reader *reader, DWORD ind
         *actual_index = stream_index;

stream = &reader->streams[stream_index];
-
-    EnterCriticalSection(&stream->cs);
-
     if (SUCCEEDED(hr = source_reader_start_source(reader)))
     {
-        if (!(flags & MF_SOURCE_READER_CONTROLF_DRAIN))
-        {
-            while (list_empty(&stream->samples) && stream->state != STREAM_STATE_EOS)
-            {
-                if (stream->stream)
-                {
-                    if (FAILED(hr = IMFMediaStream_RequestSample(stream->stream, NULL)))
-                        WARN("Sample request failed, hr %#x.\n", hr);
-                }
-                SleepConditionVariableCS(&stream->sample_event, &stream->cs, INFINITE);
-            }
-        }
-
-        *sample = media_stream_pop_sample(stream, stream_flags);
+        if (FAILED(hr = next_sample(stream, sample, stream_flags, flags & MF_SOURCE_READER_CONTROLF_DRAIN)))
+            WARN("Failed to get sample, hr = %#x.\n", hr);
+        else
+            TRACE("Got sample %p.\n", *sample);
     }

-    LeaveCriticalSection(&stream->cs);
-
-    TRACE("Got sample %p.\n", *sample);
-
     if (timestamp)
     {
         /* TODO: it's possible timestamp has to be set for some events.

[PATCH v1 3/3] mfreadwrite: Implement MF_SOURCE_READER_ASYNC_CALLBACK.

[Derek Lesho]

Signed-off-by: Derek Lesho 
---
 dlls/mfreadwrite/main.c         | 143 +++++++++++++++++++++++++++++++-
 dlls/mfreadwrite/tests/mfplat.c |  20 ++++-
 2 files changed, 159 insertions(+), 4 deletions(-)

diff --git a/dlls/mfreadwrite/main.c b/dlls/mfreadwrite/main.c
index 7ebfa0cc21..2a0a1287d6 100644
--- a/dlls/mfreadwrite/main.c
+++ b/dlls/mfreadwrite/main.c
@@ -102,6 +102,7 @@ struct media_stream
     enum media_stream_state state;
     BOOL selected;
     BOOL presented;
+    DWORD read_samples_queue;
 };

struct source_reader
@@ -109,6 +110,7 @@ struct source_reader
     IMFSourceReader IMFSourceReader_iface;
     IMFAsyncCallback source_events_callback;
     IMFAsyncCallback stream_events_callback;
+    IMFAsyncCallback read_samples_callback;
     LONG refcount;
     IMFMediaSource *source;
     IMFPresentationDescriptor *descriptor;
@@ -143,6 +145,11 @@ static struct source_reader *impl_from_stream_callback_IMFAsyncCallback(IMFAsync
     return CONTAINING_RECORD(iface, struct source_reader, stream_events_callback);
 }

+static struct source_reader *impl_from_read_samples_callback_IMFAsyncCallback(IMFAsyncCallback *iface)
+{
+    return CONTAINING_RECORD(iface, struct source_reader, read_samples_callback);
+}
+
 static inline struct sink_writer *impl_from_IMFSinkWriter(IMFSinkWriter *iface)
 {
     return CONTAINING_RECORD(iface, struct sink_writer, IMFSinkWriter_iface);
@@ -587,6 +594,8 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface)
                 list_remove(&ptr->entry);
                 heap_free(ptr);
             }
+
+            MFUnlockWorkQueue(stream->read_samples_queue);
         }
         heap_free(reader->streams);
         DeleteCriticalSection(&reader->cs);
@@ -1187,13 +1196,138 @@ static HRESULT source_reader_read_sample(struct source_reader *reader, DWORD ind
     return hr;
 }

-static HRESULT source_reader_read_sample_async(struct source_reader *reader, DWORD index, DWORD flags)
+static HRESULT WINAPI source_reader_read_samples_callback_QueryInterface(IMFAsyncCallback *iface,
+        REFIID riid, void **obj)
+{
+    TRACE("%p, %s, %p.\n", iface, debugstr_guid(riid), obj);
+
+    if (IsEqualIID(riid, &IID_IMFAsyncCallback) ||
+            IsEqualIID(riid, &IID_IUnknown))
+    {
+        *obj = iface;
+        IMFAsyncCallback_AddRef(iface);
+        return S_OK;
+    }
+
+    WARN("Unsupported %s.\n", debugstr_guid(riid));
+    *obj = NULL;
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI source_reader_read_samples_callback_AddRef(IMFAsyncCallback *iface)
+{
+    struct source_reader *reader = impl_from_read_samples_callback_IMFAsyncCallback(iface);
+    return IMFSourceReader_AddRef(&reader->IMFSourceReader_iface);
+}
+
+static ULONG WINAPI source_reader_read_samples_callback_Release(IMFAsyncCallback *iface)
 {
-    FIXME("Async mode is not implemented.\n");
+    struct source_reader *reader = impl_from_read_samples_callback_IMFAsyncCallback(iface);
+    return IMFSourceReader_Release(&reader->IMFSourceReader_iface);
+}

+static HRESULT WINAPI source_reader_read_samples_callback_GetParameters(IMFAsyncCallback *iface,
+        DWORD *flags, DWORD *queue)
+{
     return E_NOTIMPL;
 }

+static HRESULT WINAPI source_reader_read_samples_callback_Invoke(IMFAsyncCallback *iface, IMFAsyncResult *result)
+{
+    struct source_reader *reader = impl_from_read_samples_callback_IMFAsyncCallback(iface);
+    IMFMediaStream *state = (IMFMediaStream *) IMFAsyncResult_GetStateNoAddRef(result);
+    DWORD id = 0;
+    HRESULT hr;
+
+    TRACE("%p, %p\n", iface, result);
+
+    if (FAILED(hr = media_stream_get_id(state, &id)))
+    {
+        WARN("Bad stream %p, hr %#x.\n", state, hr);
+    }
+
+    for (unsigned int i = 0; i < reader->stream_count; ++i)
+    {
+        if (id == reader->streams[i].id)
+        {
+            struct media_stream *stream = &reader->streams[i];
+            IMFSample *sample = NULL;
+            DWORD stream_flags;
+            LONGLONG timestamp = 0;
+
+            hr = next_sample(stream, &sample, &stream_flags, FALSE);
+            if (sample)
+            {
+                IMFSample_GetSampleTime(sample, &timestamp);
+            }
+
+            TRACE("Invoking read sample callback %p with (hr = %#x, stream_idx = %u, flags = %#x, timestamp %lu, sample %p)\n", reader->async_callback, hr, i, stream_flags, timestamp, sample);
+            hr = IMFSourceReaderCallback_OnReadSample(reader->async_callback, hr, i, stream_flags, timestamp, sample);
+            IMFSample_Release(sample);
+            return hr;
+        }
+    }
+
+    return S_OK;
+}
+
+static const IMFAsyncCallbackVtbl read_samples_callback_vtbl =
+{
+    source_reader_read_samples_callback_QueryInterface,
+    source_reader_read_samples_callback_AddRef,
+    source_reader_read_samples_callback_Release,
+    source_reader_read_samples_callback_GetParameters,
+    source_reader_read_samples_callback_Invoke,
+};
+
+static HRESULT source_reader_read_sample_async(struct source_reader *reader, DWORD index, DWORD flags)
+{
+    struct media_stream *stream;
+    DWORD stream_index;
+    HRESULT hr = S_OK;
+    BOOL selected;
+
+    switch (index)
+    {
+        case MF_SOURCE_READER_FIRST_VIDEO_STREAM:
+            stream_index = reader->first_video_stream_index;
+            break;
+        case MF_SOURCE_READER_FIRST_AUDIO_STREAM:
+            stream_index = reader->first_audio_stream_index;
+            break;
+        case MF_SOURCE_READER_ANY_STREAM:
+            FIXME("Non-specific requests are not supported.\n");
+            return E_NOTIMPL;
+        default:
+            stream_index = index;
+    }
+
+    /* Can't read from deselected streams. */
+    if (FAILED(hr = source_reader_get_stream_selection(reader, stream_index, &selected)) && !selected)
+        return hr;
+
+    stream = &reader->streams[stream_index];
+
+    if (FAILED(hr = source_reader_start_source(reader)))
+        return hr;
+
+    EnterCriticalSection(&stream->cs);
+    while(!stream->stream)
+    {
+        SleepConditionVariableCS(&stream->sample_event, &stream->cs, INFINITE);
+    }
+    LeaveCriticalSection(&stream->cs);
+
+    TRACE("Dispatching read sample callback for stream %p\n", stream->stream);
+    if (FAILED(hr = MFPutWorkItem(stream->read_samples_queue, &reader->read_samples_callback, (IUnknown*)stream->stream)))
+    {
+        WARN("Failed to submit item hr = %#x\n", hr);
+        return E_FAIL;
+    }
+
+    return S_OK;
+}
+
 static HRESULT WINAPI src_reader_ReadSample(IMFSourceReader *iface, DWORD index, DWORD flags, DWORD *actual_index,
         DWORD *stream_flags, LONGLONG *timestamp, IMFSample **sample)
 {
@@ -1379,6 +1513,7 @@ static HRESULT create_source_reader_from_source(IMFMediaSource *source, IMFAttri
     object->IMFSourceReader_iface.lpVtbl = &srcreader_vtbl;
     object->source_events_callback.lpVtbl = &source_events_callback_vtbl;
     object->stream_events_callback.lpVtbl = &stream_events_callback_vtbl;
+    object->read_samples_callback.lpVtbl = &read_samples_callback_vtbl;
     object->refcount = 1;
     object->source = source;
     IMFMediaSource_AddRef(object->source);
@@ -1404,6 +1539,10 @@ static HRESULT create_source_reader_from_source(IMFMediaSource *source, IMFAttri
         IMFMediaType *src_type;
         BOOL selected;

+
+        if (FAILED(hr = MFAllocateWorkQueue(&object->streams[i].read_samples_queue)))
+            break;
+
         if (FAILED(hr = MFCreateMediaType(&object->streams[i].current)))
             break;

diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c
index e03d74981a..2bc9e2c40f 100644
--- a/dlls/mfreadwrite/tests/mfplat.c
+++ b/dlls/mfreadwrite/tests/mfplat.c
@@ -562,11 +562,18 @@ static ULONG WINAPI async_callback_Release(IMFSourceReaderCallback *iface)
     return refcount;
 }

+static HANDLE on_read_sample_event = INVALID_HANDLE_VALUE;
+
 static HRESULT WINAPI async_callback_OnReadSample(IMFSourceReaderCallback *iface, HRESULT hr, DWORD stream_index,
         DWORD stream_flags, LONGLONG timestamp, IMFSample *sample)
 {
-    ok(0, "Unexpected call.\n");
-    return E_NOTIMPL;
+    ok(hr == S_OK, "Unexpected hr %#x\n", hr);
+    ok(!stream_flags, "Unexpected stream flags %#x.\n", stream_flags);
+    ok(!!sample, "Didn't receive sample.\n");
+
+    SetEvent(on_read_sample_event);
+
+    return S_OK;
 }

static HRESULT WINAPI async_callback_OnFlush(IMFSourceReaderCallback *iface, DWORD stream_index)
@@ -951,6 +958,15 @@ todo_wine
     hr = IMFSourceReader_ReadSample(reader, 0, 0, NULL, NULL, NULL, &sample);
     ok(hr == E_INVALIDARG, "Unexpected hr %#x.\n", hr);

+    on_read_sample_event = CreateEventA(NULL, FALSE, FALSE, NULL);
+
+    hr = IMFSourceReader_ReadSample(reader, 0, 0, NULL, NULL, NULL, NULL);
+    ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
+
+    ok(WaitForSingleObject(on_read_sample_event, 3000) == WAIT_OBJECT_0, "Sample never triggered.\n");
+
+    CloseHandle(on_read_sample_event);
+
     IMFSourceReader_Release(reader);
 }

[PATCH v1 3/3] mfreadwrite: Implement MF_SOURCE_READER_ASYNC_CALLBACK.

[Nikolay Sivov]

On 3/17/20 7:48 PM, Derek Lesho wrote:
> @@ -587,6 +594,8 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface)
>                   list_remove(&ptr->entry);
>                   heap_free(ptr);
>               }
> -
> -
>        MFUnlockWorkQueue(stream->read_samples_queue);
>     }
>     heap_free(reader->streams);
>     DeleteCriticalSection(&reader->cs);

Is additional queue necessary for synchronous case?
> - if (FAILED(hr = media_stream_get_id(state, &id)))
> - {
> -
>    WARN("Bad stream %p, hr %#x.\n", state, hr);
> - }
> -
> - for (unsigned int i = 0; i < reader->stream_count; ++i)
> - {
> -
>    if (id == reader->streams[i].id)
> -
>    {
> -
>        struct media_stream *stream = &reader->streams[i];
> -
>        IMFSample *sample = NULL;
> -
>        DWORD stream_flags;
> -
>        LONGLONG timestamp = 0;
> -
> -
>        hr = next_sample(stream, &sample, &stream_flags, FALSE);
> -
>        if (sample)
> -
>        {
> -
>            IMFSample_GetSampleTime(sample, &timestamp);
> -
>        }
> -
> -
>        TRACE("Invoking read sample callback %p with (hr = %#x, stream_idx = %u, flags = %#x, timestamp %lu, sample %p)\n", reader->async_callback, hr, i, stream_flags, timestamp, sample);
> -
>        hr = IMFSourceReaderCallback_OnReadSample(reader->async_callback, hr, i, stream_flags, timestamp, sample);
> -
>        IMFSample_Release(sample);
> -
>        return hr;
> -
>    }
> - }
> -
> - return S_OK;

We already talked about this part - blocking in next_sample(), even on 
dedicated thread, is unnecessary.

What will happen is MENewSample from stream queue -> wake thread in 
dedicated per-stream queue -> process -> call OnReadSample.
Without blocking you'll have MENewSample -> process on same event 
handling thread -> call OnReadSample.

I don't see a need for additional complexity.
> - switch (index)
> - {
> -
>    case MF_SOURCE_READER_FIRST_VIDEO_STREAM:
> -
>        stream_index = reader->first_video_stream_index;
> -
>        break;
> -
>    case MF_SOURCE_READER_FIRST_AUDIO_STREAM:
> -
>        stream_index = reader->first_audio_stream_index;
> -
>        break;
> -
>    case MF_SOURCE_READER_ANY_STREAM:
> -
>        FIXME("Non-specific requests are not supported.\n");
> -
>        return E_NOTIMPL;
> -
>    default:
> -
>        stream_index = index;
> - }
> -
> - /* Can't read from deselected streams. */
> - if (FAILED(hr = source_reader_get_stream_selection(reader, stream_index, &selected)) && !selected)
> -
>    return hr;

If that's how it works, this part applies to both sync and async cases.
> - EnterCriticalSection(&stream->cs);
> - while(!stream->stream)
> - {
> -
>    SleepConditionVariableCS(&stream->sample_event, &stream->cs, INFINITE);
> - }
> - LeaveCriticalSection(&stream->cs);
> -
> - TRACE("Dispatching read sample callback for stream %p\n", stream->stream);
> - if (FAILED(hr = MFPutWorkItem(stream->read_samples_queue, &reader->read_samples_callback, (IUnknown*)stream->stream)))
> - {
> -
>    WARN("Failed to submit item hr = %#x\n", hr);
> -
>    return E_FAIL;
> - }

This blocks for indefinite amount of time, for ReadSample method that's 
supposed to be non-blocking. It's correct of course to start the source 
here,
which also could be potentially shared with sync case, but waiting for 
new samples, using 'sample_event' that's not meant for it, is wrong.

It should start source, and keep a note of number of times each stream 
was requested, later on MENewStream/MEUpdatedStream you can issue that 
many requests at once.

[PATCH v1 3/3] mfreadwrite: Implement MF_SOURCE_READER_ASYNC_CALLBACK.

[Derek Lesho]

On 2020-03-18 08:37, Nikolay Sivov wrote:
> On 3/17/20 7:48 PM, Derek Lesho wrote:
> > @@ -587,6 +594,8 @@ static ULONG WINAPI 
> > src_reader_Release(IMFSourceReader *iface)
> >                   list_remove(&ptr->entry);
> >                   heap_free(ptr);
> >               }
> > -
> >
> > +            MFUnlockWorkQueue(stream->read_samples_queue);
> >           }
> >           heap_free(reader->streams);
> >           DeleteCriticalSection(&reader->cs);
>
> Is additional queue necessary for synchronous case?

No, I can remove it in that case.
> > +    if (FAILED(hr = media_stream_get_id(state, &id)))
> > +    {
> > +        WARN("Bad stream %p, hr %#x.\n", state, hr);
> > +    }
> > -
> >
> > +    for (unsigned int i = 0; i < reader->stream_count; ++i)
> > +    {
> > +        if (id == reader->streams[i].id)
> > +        {
> > +            struct media_stream *stream = &reader->streams[i];
> > +            IMFSample *sample = NULL;
> > +            DWORD stream_flags;
> > +            LONGLONG timestamp = 0;
> > -
> >
> > +            hr = next_sample(stream, &sample, &stream_flags, FALSE);
> > +            if (sample)
> > +            {
> > +                IMFSample_GetSampleTime(sample, &timestamp);
> > +            }
> > -
> >
> > +            TRACE("Invoking read sample callback %p with (hr = %#x, 
> > stream_idx = %u, flags = %#x, timestamp %lu, sample %p)\n", 
> > reader->async_callback, hr, i, stream_flags, timestamp, sample);
> > +            hr = 
> > IMFSourceReaderCallback_OnReadSample(reader->async_callback, hr, i, 
> > stream_flags, timestamp, sample);
> > +            IMFSample_Release(sample);
> > +            return hr;
> > +        }
> > +    }
> > -
> >
> > +    return S_OK;
>
> We already talked about this part - blocking in next_sample(), even on 
> dedicated thread, is unnecessary.
>
> What will happen is MENewSample from stream queue -> wake thread in 
> dedicated per-stream queue -> process -> call OnReadSample.
> Without blocking you'll have MENewSample -> process on same event 
> handling thread -> call OnReadSample.
>
> I don't see a need for additional complexity.

This solution, in my opinion, reduces complexity, as it allows us to 
share the same logic between the asynchronous and synchronous paths.  
Additionally, what happens if a single input sample produces multiple 
output samples, do we fall back to the work queue method in that case?  
The only difference in the two paths you've laid out is that the work 
queue approach involves waking up another thread, in the same way that 
we will have to wake up the thread blocking on ReadSample.
> > +    switch (index)
> > +    {
> > +        case MF_SOURCE_READER_FIRST_VIDEO_STREAM:
> > +            stream_index = reader->first_video_stream_index;
> > +            break;
> > +        case MF_SOURCE_READER_FIRST_AUDIO_STREAM:
> > +            stream_index = reader->first_audio_stream_index;
> > +            break;
> > +        case MF_SOURCE_READER_ANY_STREAM:
> > +            FIXME("Non-specific requests are not supported.\n");
> > +            return E_NOTIMPL;
> > +        default:
> > +            stream_index = index;
> > +    }
> > -
> >
> > +    /* Can't read from deselected streams. */
> > +    if (FAILED(hr = source_reader_get_stream_selection(reader, 
> > stream_index, &selected)) && !selected)
> > +        return hr;
>
> If that's how it works, this part applies to both sync and async cases.

Sorry, typo here.
> > - EnterCriticalSection(&stream->cs);
> >
> > +    while(!stream->stream)
> > +    {
> > +        SleepConditionVariableCS(&stream->sample_event, &stream->cs, 
> > INFINITE);
> > +    }
> > +    LeaveCriticalSection(&stream->cs);
> > -
> >
> > +    TRACE("Dispatching read sample callback for stream %p\n", 
> > stream->stream);
> > +    if (FAILED(hr = MFPutWorkItem(stream->read_samples_queue, 
> > &reader->read_samples_callback, (IUnknown*)stream->stream)))
> > +    {
> > +        WARN("Failed to submit item hr = %#x\n", hr);
> > +        return E_FAIL;
> > +    }
>
> This blocks for indefinite amount of time, for ReadSample method 
> that's supposed to be non-blocking. It's correct of course to start 
> the source here,
> which also could be potentially shared with sync case, but waiting for 
> new samples, using 'sample_event' that's not meant for it, is wrong.
>
> It should start source, and keep a note of number of times each stream 
> was requested, later on MENewStream/MEUpdatedStream you can issue that 
> many requests at once.

Ack.

[Nikolay Sivov]

On 3/17/20 7:48 PM, Derek Lesho wrote:
>           LIST_FOR_EACH_ENTRY_SAFE(ptr, next, &stream->samples, struct sample, entry)
>
> @@ -755,6 +758,15 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
>           IMFMediaType *type)
>   {
>       struct source_reader *reader = impl_from_IMFSourceReader(iface);
> -
>
> DWORD equal_flags;
> -
>
> GUID major_type, target_subtype, decoder_category;
> -
>
> IMFStreamDescriptor *stream_descriptor = NULL;
> -
>
> BOOL selected;
> -
>
> IMFMediaTypeHandler *stream_type_handler = NULL;
> -
>
> IMFMediaType *in_stream_type = NULL, *potential_out_stream_type = NULL;
> -
>
> CLSID decoder_id;
> -
>
> BOOL decoder_found = FALSE;
> -
>
> IMFTransform *decoder = NULL;
>  HRESULT hr;
>
> TRACE("%p, %#x, %p, %p.\n", iface, index, reserved, type);
>
> @@ -774,14 +786,186 @@ static HRESULT WINAPI src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
>       if (index >= reader->stream_count)
>           return MF_E_INVALIDSTREAMNUMBER;
> - /* FIXME: validate passed type and current presentation state. */
> -

Second part of this comment still applies.
>   EnterCriticalSection(&reader->cs);
>
> - hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
> - if (FAILED(hr = IMFMediaType_IsEqual(type, reader->streams[index].current, &equal_flags)))
> -
>    goto done;
>
> - LeaveCriticalSection(&reader->cs);
> - if (!(equal_flags & MF_MEDIATYPE_EQUAL_MAJOR_TYPES))
> - {
> -
>    /* Not gonna happen */
> -
>    hr = MF_E_INVALIDMEDIATYPE;
> -
>    goto done;
> - }

Does it mean usually not going to happen or can't happen?
> -
> - if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> - {
> -
>    /* Similar enough not to need to refind a decoder */
> -
>    hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *)reader->streams[index].current);
> -
>    goto done;
> - }

This should be possible to test if new, yet equal, type attributes 
replace existing attributes. It's not obviously correct.

Regarding skipping decoder decoder lookup, while true, what should 
happen if you already have a decoder?
E.g. should you be able to change current type after you set it once. If 
yes, old decoder should be shut down/released first,
and such switch should be likely rejected if you're currently streaming.
> -
> - if (FAILED(hr = IMFMediaType_GetMajorType(type, &major_type)))
> -
>    goto done;
> -
> - if (FAILED(hr = IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &target_subtype)))
> -
>    goto done;
> -
> - if (FAILED(hr = IMFPresentationDescriptor_GetStreamDescriptorByIndex(reader->descriptor, index, &selected, &stream_descriptor)))
> -
>    goto done;
> -
> - if (FAILED(hr = IMFStreamDescriptor_GetMediaTypeHandler(stream_descriptor, &stream_type_handler)))
> -
>    goto done;
> -
> - /* first, check if it's a native type */
> - for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> - {
> -
>    if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
> -
>        continue;
> -
> -
>    if (FAILED(hr = IMFMediaType_IsEqual(type, in_stream_type, &equal_flags)))
> -
>        goto done;
> -
> -
>    if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> -
>    {
> -
>        if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
> -
>            goto done;
> -
>        hr = IMFMediaType_CopyAllItems(in_stream_type, (IMFAttributes *)reader->streams[index].current);
> -
>        goto done;
> -
>    }
> -
> -
>    IMFMediaType_Release(in_stream_type);
> -
>    in_stream_type = NULL;
> - }

Calls preceding this loop essentially duplicate what 
GetNativeMediaType() would do. Could it use IsMediaTypeSupported() instead?

Is it correct to discard input type attributes and use full native type?
> -
> - /* TODO: should we check if the source type is compressed? */
> -
> - if (IsEqualGUID(&major_type, &MFMediaType_Video))
> - {
> -
>    decoder_category = MFT_CATEGORY_VIDEO_DECODER;
> - }
> - else if (IsEqualGUID(&major_type, &MFMediaType_Audio))
> - {
> -
>    decoder_category = MFT_CATEGORY_AUDIO_DECODER;
> - }
> - else
> - {
> -
>    hr = MF_E_TOPO_CODEC_NOT_FOUND;
> -
>    goto done;
> - }

Yes, we probably should check that, for example in case of color 
conversion. But as understand current plan for decoders was to advertise
more types and get paired with videoconvert, so maybe for now it's not 
really necessary.
> -
> -
>
> hr = S_OK;
> -
>
> for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> -
>
> {
> -
>    GUID source_subtype;
> -
>    MFT_REGISTER_TYPE_INFO in_type, out_type;
> -
>    CLSID *decoder_ids;
> -
>    ULONG decoder_count;
> -
> -
>    if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, index, i, &in_stream_type)))
> -
>        continue;
> -
> -
>    if (FAILED(hr = IMFMediaType_GetGUID(in_stream_type, &MF_MT_SUBTYPE, &source_subtype)))
> -
>        goto done;
> -
> -
>    in_type.guidMajorType = major_type;
> -
>    in_type.guidSubtype = source_subtype;
> -
> -
>    out_type.guidMajorType = major_type;
> -
>    out_type.guidSubtype = target_subtype;
>
> -
>    if (FAILED(hr = MFTEnum(decoder_category, 0, &in_type, &out_type, NULL, &decoder_ids, &decoder_count)) || decoder_count == 0)
> -
>    {
> -
>        if (!(decoder_found))
> -
>        {
> -
>            /* see if there are other decoders for this stream */
> -
>            if (SUCCEEDED(MFTEnum(decoder_category, 0, &in_type, NULL, NULL, &decoder_ids, &decoder_count)) && decoder_count > 0)
> -
>            {
> -
>                decoder_found = TRUE;
> -
>                CoTaskMemFree(decoder_ids);
> -
>            }
> -
>        }
> -
> -
>        IMFMediaType_Release(in_stream_type);
> -
>        in_stream_type = NULL;
> -
>        continue;
> -
>    }

Is it really useful to have a second call here? It probably means "any" 
output type, which could work in some interactive scenario. Does it really
offer anything available in this case on Windows and what happens to 
current type?
> -
> -
>    if (decoder_count > 1)
> -
>        FIXME("Just using the first decoder.\n");

There is a way to sort in MFTEnum*, and it's normal to get a number of 
candidates, so I don't know what this fixme will tell us. Do you mean to
iterate over them all in case advertised types don't actually work?
> -
>    decoder_id = decoder_ids[0];
> -
>    CoTaskMemFree(decoder_ids);
> -
> -
>    if (FAILED(hr = IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, in_stream_type)))
> -
>        goto done;
> -
> -
>    break;
> - }
> -
> - if (!in_stream_type)
> - {
> -
>    ERR("Failed to find decoder matching request, source has decoder: %u.\n", decoder_found);
> -
>    hr = decoder_found ? MF_E_INVALIDREQUEST : MF_E_TOPO_CODEC_NOT_FOUND;
> -
>    goto done;
> - }
> -
> - /* we have the decoder id at this point */
> - if (FAILED(hr = CoCreateInstance(&decoder_id, NULL, CLSCTX_INPROC_SERVER, &IID_IMFTransform, (void**)&decoder)))
> -
>    goto done;
> -
> - {
> -
>    DWORD in_min, in_max, out_min, out_max;
> -
> -
>    if (FAILED(hr = IMFTransform_GetStreamLimits(decoder, &in_min, &in_max, &out_min, &out_max)))
> -
>        goto done;
> -
>    if (in_min != 1 || in_max != 1 || out_min != 1 || out_max != 1)
> -
>    {
> -
>        hr = ERROR_INTERNAL_ERROR;
> -
>        goto done;
> -
>    }
> - }

GetStreamCount() seems easier. Why ERROR_INTERNAL_ERROR? It's not a 
valid failure HRESULT.

Would it help to use IMFActivate instances instead? For example that 
would be the only way to access local transforms, but I don't know if 
reader is using them.
> -
> - if (FAILED(hr = IMFTransform_SetInputType(decoder, 0, in_stream_type, 0)))
> -
>    goto done;
> -
> - /* find the relevant output type */
> - for (unsigned int i = 0;;i++)
> - {
> -
>    if (FAILED(hr = IMFTransform_GetOutputAvailableType(decoder, 0, i, &potential_out_stream_type)))
> -
>        goto done;
> -
> -
>    if (FAILED(hr = IMFMediaType_IsEqual(type, potential_out_stream_type, &equal_flags)))
> -
>        goto done;
> -
> -
>    if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> -
>    {
> -
>        if (FAILED(hr = IMFTransform_SetOutputType(decoder, 0, potential_out_stream_type, 0)))
> -
>            goto done;
> -
> -
>        if (FAILED(hr = IMFMediaType_CopyAllItems(potential_out_stream_type, (IMFAttributes *)reader->streams[index].current)))
> -
>            goto done;
> -
> -
>        break;
> -
>    }
> -
>    IMFMediaType_Release(potential_out_stream_type);
> -
>    potential_out_stream_type = NULL;
> - }

This should handle a case when output type should be set first.
> -
> - reader->streams[index].decoder = decoder;
> - decoder = NULL;
> -
> - done:
> - LeaveCriticalSection(&reader->cs);
> - if (stream_descriptor)
> -
>    IMFStreamDescriptor_Release(stream_descriptor);
> - if (stream_type_handler)
> -
>    IMFMediaTypeHandler_Release(stream_type_handler);
> - if (in_stream_type)
> -
>    IMFMediaType_Release(in_stream_type);
> - if (decoder)
> -
>    IMFTransform_Release(decoder);
> - if (potential_out_stream_type)
> -
>    IMFMediaType_Release(potential_out_stream_type);
> return hr;
> }

[Derek Lesho]

On 2020-03-18 08:23, Nikolay Sivov wrote:
> On 3/17/20 7:48 PM, Derek Lesho wrote:
> > LIST_FOR_EACH_ENTRY_SAFE(ptr, next, &stream->samples, struct sample, 
> > entry)
> > @@ -755,6 +758,15 @@ static HRESULT WINAPI 
> > src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
> >           IMFMediaType *type)
> >   {
> >       struct source_reader *reader = impl_from_IMFSourceReader(iface);
> > +    DWORD equal_flags;
> > +    GUID major_type, target_subtype, decoder_category;
> > +    IMFStreamDescriptor *stream_descriptor = NULL;
> > +    BOOL selected;
> > +    IMFMediaTypeHandler *stream_type_handler = NULL;
> > +    IMFMediaType *in_stream_type = NULL, *potential_out_stream_type 
> > = NULL;
> > +    CLSID decoder_id;
> > +    BOOL decoder_found = FALSE;
> > +    IMFTransform *decoder = NULL;
> >       HRESULT hr;
> >         TRACE("%p, %#x, %p, %p.\n", iface, index, reserved, type);
> > @@ -774,14 +786,186 @@ static HRESULT WINAPI 
> > src_reader_SetCurrentMediaType(IMFSourceReader *iface, DWO
> >       if (index >= reader->stream_count)
> >           return MF_E_INVALIDSTREAMNUMBER;
> >   -    /* FIXME: validate passed type and current presentation state. */
> > -
>
> Second part of this comment still applies.

Ack.
> > EnterCriticalSection(&reader->cs);
> >   -    hr = IMFMediaType_CopyAllItems(type, (IMFAttributes 
> > *)reader->streams[index].current);
> > +    if (FAILED(hr = IMFMediaType_IsEqual(type, 
> > reader->streams[index].current, &equal_flags)))
> > +        goto done;
> >   -    LeaveCriticalSection(&reader->cs);
> > +    if (!(equal_flags & MF_MEDIATYPE_EQUAL_MAJOR_TYPES))
> > +    {
> > +        /* Not gonna happen */
> > +        hr = MF_E_INVALIDMEDIATYPE;
> > +        goto done;
> > +    }
>
> Does it mean usually not going to happen or can't happen?

It's just a bad comment I forgot to remove.
> > -
> >
> > +    if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> > +    {
> > +        /* Similar enough not to need to refind a decoder */
> > +        hr = IMFMediaType_CopyAllItems(type, (IMFAttributes 
> > *)reader->streams[index].current);
> > +        goto done;
> > +    }
>
> This should be possible to test if new, yet equal, type attributes 
> replace existing attributes. It's not obviously correct.

Ack.
> Regarding skipping decoder decoder lookup, while true, what should 
> happen if you already have a decoder?
> E.g. should you be able to change current type after you set it once. 
> If yes, old decoder should be shut down/released first,
> and such switch should be likely rejected if you're currently streaming.

Ah, I didn't think about that, and looking at the return values for the 
method, it looks like you need to flush the source reader before you can 
change the type.
> > -
> >
> > +    if (FAILED(hr = IMFMediaType_GetMajorType(type, &major_type)))
> > +        goto done;
> > -
> >
> > +    if (FAILED(hr = IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, 
> > &target_subtype)))
> > +        goto done;
> > -
> >
> > +    if (FAILED(hr = 
> > IMFPresentationDescriptor_GetStreamDescriptorByIndex(reader->descriptor, 
> > index, &selected, &stream_descriptor)))
> > +        goto done;
> > -
> >
> > +    if (FAILED(hr = 
> > IMFStreamDescriptor_GetMediaTypeHandler(stream_descriptor, 
> > &stream_type_handler)))
> > +        goto done;
> > -
> >
> > +    /* first, check if it's a native type */
> > +    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> > +    {
> > +        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, 
> > index, i, &in_stream_type)))
> > +            continue;
> > -
> >
> > +        if (FAILED(hr = IMFMediaType_IsEqual(type, in_stream_type, 
> > &equal_flags)))
> > +            goto done;
> > -
> >
> > +        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> > +        {
> > +            if (FAILED(hr = 
> > IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, 
> > in_stream_type)))
> > +                goto done;
> > +            hr = IMFMediaType_CopyAllItems(in_stream_type, 
> > (IMFAttributes *)reader->streams[index].current);
> > +            goto done;
> > +        }
> > -
> >
> > +        IMFMediaType_Release(in_stream_type);
> > +        in_stream_type = NULL;
> > +    }
>
> Calls preceding this loop essentially duplicate what 
> GetNativeMediaType() would do. Could it use IsMediaTypeSupported() 
> instead?
>
> Is it correct to discard input type attributes and use full native type?

Ack.
> > -
> >
> > +    /* TODO: should we check if the source type is compressed? */
> > -
> >
> > +    if (IsEqualGUID(&major_type, &MFMediaType_Video))
> > +    {
> > +        decoder_category = MFT_CATEGORY_VIDEO_DECODER;
> > +    }
> > +    else if (IsEqualGUID(&major_type, &MFMediaType_Audio))
> > +    {
> > +        decoder_category = MFT_CATEGORY_AUDIO_DECODER;
> > +    }
> > +    else
> > +    {
> > +        hr = MF_E_TOPO_CODEC_NOT_FOUND;
> > +        goto done;
> > +    }
>
> Yes, we probably should check that, for example in case of color 
> conversion. But as understand current plan for decoders was to advertise
> more types and get paired with videoconvert, so maybe for now it's not 
> really necessary.

My intention is the advertise whatever the windows variant of the 
decoder advertises, and so far I haven't seen an application set the 
output type to something the decoder doesn't support.  If that ever were 
the case, the correct solution would be to use VideoProcessorMFT.
> > -
> >
> > +    hr = S_OK;
> > +    for (unsigned int i = 0; hr != MF_E_NO_MORE_TYPES; i++)
> > +    {
> > +        GUID source_subtype;
> > +        MFT_REGISTER_TYPE_INFO in_type, out_type;
> > +        CLSID *decoder_ids;
> > +        ULONG decoder_count;
> > -
> >
> > +        if (FAILED(hr = IMFSourceReader_GetNativeMediaType(iface, 
> > index, i, &in_stream_type)))
> > +            continue;
> > -
> >
> > +        if (FAILED(hr = IMFMediaType_GetGUID(in_stream_type, 
> > &MF_MT_SUBTYPE, &source_subtype)))
> > +            goto done;
> > -
> >
> > +        in_type.guidMajorType = major_type;
> > +        in_type.guidSubtype = source_subtype;
> > -
> >
> > +        out_type.guidMajorType = major_type;
> > +        out_type.guidSubtype = target_subtype;
> >   +        if (FAILED(hr = MFTEnum(decoder_category, 0, &in_type, 
> > &out_type, NULL, &decoder_ids, &decoder_count)) || decoder_count == 0)
> > +        {
> > +            if (!(decoder_found))
> > +            {
> > +                /* see if there are other decoders for this stream */
> > +                if (SUCCEEDED(MFTEnum(decoder_category, 0, &in_type, 
> > NULL, NULL, &decoder_ids, &decoder_count)) && decoder_count > 0)
> > +                {
> > +                    decoder_found = TRUE;
> > +                    CoTaskMemFree(decoder_ids);
> > +                }
> > +            }
> > -
> >
> > +            IMFMediaType_Release(in_stream_type);
> > +            in_stream_type = NULL;
> > +            continue;
> > +        }
>
> Is it really useful to have a second call here? It probably means 
> "any" output type, which could work in some interactive scenario. Does 
> it really
> offer anything available in this case on Windows and what happens to 
> current type?

The reason we need the second call is so that we return the correct 
error code.  `MF_E_INVALIDREQUEST` when the type can't be output, and 
`MF_E_TOPO_CODEC_NOT_FOUND` when a decoder isn't found for a native 
media type.
> > -
> >
> > +        if (decoder_count > 1)
> > +            FIXME("Just using the first decoder.\n");
>
> There is a way to sort in MFTEnum*, and it's normal to get a number of 
> candidates, so I don't know what this fixme will tell us. Do you mean to
> iterate over them all in case advertised types don't actually work?

Nope, this is just me putting too many logs in.
> > +        decoder_id = decoder_ids[0];
> > +        CoTaskMemFree(decoder_ids);
> > -
> >
> > +        if (FAILED(hr = 
> > IMFMediaTypeHandler_SetCurrentMediaType(stream_type_handler, 
> > in_stream_type)))
> > +            goto done;
> > -
> >
> > +        break;
> > +    }
> > -
> >
> > +    if (!in_stream_type)
> > +    {
> > +        ERR("Failed to find decoder matching request, source has 
> > decoder: %u.\n", decoder_found);
> > +        hr = decoder_found ? MF_E_INVALIDREQUEST : 
> > MF_E_TOPO_CODEC_NOT_FOUND;
> > +        goto done;
> > +    }
> > -
> >
> > +    /* we have the decoder id at this point */
> > +    if (FAILED(hr = CoCreateInstance(&decoder_id, NULL, 
> > CLSCTX_INPROC_SERVER, &IID_IMFTransform, (void**)&decoder)))
> > +        goto done;
> > -
> >
> > +    {
> > +        DWORD in_min, in_max, out_min, out_max;
> > -
> >
> > +        if (FAILED(hr = IMFTransform_GetStreamLimits(decoder, 
> > &in_min, &in_max, &out_min, &out_max)))
> > +            goto done;
> > +        if (in_min != 1 || in_max != 1 || out_min != 1 || out_max != 1)
> > +        {
> > +            hr = ERROR_INTERNAL_ERROR;
> > +            goto done;
> > +        }
> > +    }
>
> GetStreamCount() seems easier. Why ERROR_INTERNAL_ERROR? It's not a 
> valid failure HRESULT.

Because it auto-filled when I searched INTERNAL_ERROR, whoops.  To be 
honest, I'm not sure why I added this check in the first place, it 
really shouldn't be necessary.
> Would it help to use IMFActivate instances instead? For example that 
> would be the only way to access local transforms, but I don't know if 
> reader is using them.

I'm also unsure whether the source reader can access local transforms, 
but it doesn't seem critical to me.  Do you think this could instead be 
added in a future patch?
> > -
> >
> > +    if (FAILED(hr = IMFTransform_SetInputType(decoder, 0, 
> > in_stream_type, 0)))
> > +        goto done;
> > -
> >
> > +    /* find the relevant output type */
> > +    for (unsigned int i = 0;;i++)
> > +    {
> > +        if (FAILED(hr = IMFTransform_GetOutputAvailableType(decoder, 
> > 0, i, &potential_out_stream_type)))
> > +            goto done;
> > -
> >
> > +        if (FAILED(hr = IMFMediaType_IsEqual(type, 
> > potential_out_stream_type, &equal_flags)))
> > +            goto done;
> > -
> >
> > +        if (equal_flags & MF_MEDIATYPE_EQUAL_FORMAT_TYPES)
> > +        {
> > +            if (FAILED(hr = IMFTransform_SetOutputType(decoder, 0, 
> > potential_out_stream_type, 0)))
> > +                goto done;
> > -
> >
> > +            if (FAILED(hr = 
> > IMFMediaType_CopyAllItems(potential_out_stream_type, (IMFAttributes 
> > *)reader->streams[index].current)))
> > +                goto done;
> > -
> >
> > +            break;
> > +        }
> > +        IMFMediaType_Release(potential_out_stream_type);
> > +        potential_out_stream_type = NULL;
> > +    }
>
> This should handle a case when output type should be set first.

Is this ever the case with a decoder?
> > -
> >
> > +    reader->streams[index].decoder = decoder;
> > +    decoder = NULL;
> > -
> >
> > +    done:
> > +    LeaveCriticalSection(&reader->cs);
> > +    if (stream_descriptor)
> > +        IMFStreamDescriptor_Release(stream_descriptor);
> > +    if (stream_type_handler)
> > +        IMFMediaTypeHandler_Release(stream_type_handler);
> > +    if (in_stream_type)
> > +        IMFMediaType_Release(in_stream_type);
> > +    if (decoder)
> > +        IMFTransform_Release(decoder);
> > +    if (potential_out_stream_type)
> > +        IMFMediaType_Release(potential_out_stream_type);
> >       return hr;
> >   }

[Derek Lesho]

On 2020-03-18 09:53, Derek Lesho wrote:
> Ah, I didn't think about that, and looking at the return values for 
> the method, it looks like you need to flush the source reader before 
> you can change the type.

Disregard this, it only fails in that way if there are pending sample 
requests.

[Nikolay Sivov]

On 3/18/20 5:53 PM, Derek Lesho wrote:
> > Is it really useful to have a second call here? It probably means 
> > "any" output type, which could work in some interactive scenario. 
> > Does it really
> > offer anything available in this case on Windows and what happens to 
> > current type?
>
> The reason we need the second call is so that we return the correct 
> error code.  `MF_E_INVALIDREQUEST` when the type can't be output, and 
> `MF_E_TOPO_CODEC_NOT_FOUND` when a decoder isn't found for a native 
> media type.

Let's keep it simple and return MF_E_INVALIDREQUEST, with single enum call.
> > Would it help to use IMFActivate instances instead? For example that 
> > would be the only way to access local transforms, but I don't know if 
> > reader is using them.
>
> I'm also unsure whether the source reader can access local transforms, 
> but it doesn't seem critical to me.  Do you think this could instead 
> be added in a future patch?

Definitely. I asked because they moved away from CLSID-based enumeration 
in later versions of EnumEx and Enum2, and only newer functions give 
control over returned results.
> This should handle a case when output type should be set first.
> Is this ever the case with a decoder?

I don't know, it's documented to work one way or another. It's a minor 
addition if we ever need it, for user transforms for example. If you 
verified that at least some decoders work like that, no need to change it.