Re: [PATCH 5/6] winegstreamer: Implement IMFMediaSource::Start.

[Zebediah Figura]

On 9/29/20 5:08 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho
>
> dlls/mfplat/tests/mfplat.c        |   8 +-
>  dlls/winegstreamer/media_source.c | 316 +++++++++++++++++++++++++++++-
>  2 files changed, 316 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c
> index 15d5bcba3d6..bffce2bc114 100644
> --- a/dlls/mfplat/tests/mfplat.c
> +++ b/dlls/mfplat/tests/mfplat.c
> @@ -605,10 +605,7 @@ todo_wine
>  var.vt = VT_EMPTY;
>  hr = IMFMediaSource_Start(mediasource, descriptor, &GUID_NULL, &var);
>
> -todo_wine
>      ok(hr == S_OK, "Failed to start media source, hr %#x.\n", hr);
> -
>
> if (FAILED(hr))
> -
>    goto skip_source_tests;
>
> get_event((IMFMediaEventGenerator *)mediasource, MENewStream, &var);
>    ok(var.vt == VT_UNKNOWN, "Unexpected value type %u from MENewStream event.\n", var.vt);
>
> @@ -626,10 +623,13 @@ todo_wine
>          hr = IMFMediaStream_RequestSample(video_stream, NULL);
>          if (i == sample_count)
>              break;
> +todo_wine
>          ok(hr == S_OK, "Failed to request sample %u, hr %#x.\n", i + 1, hr);
>          if (hr != S_OK)
>              break;
>      }
> -
>
> if (FAILED(hr))
> -
>    goto skip_source_tests;
>
> for (i = 0; i < sample_count; ++i)
>    {
>
> @@ -667,11 +667,11 @@ todo_wine
>  hr = IMFMediaStream_RequestSample(video_stream, NULL);
>  ok(hr == MF_E_END_OF_STREAM, "Unexpected hr %#x.\n", hr);
> -
>
> IMFMediaStream_Release(video_stream);
>
> get_event((IMFMediaEventGenerator *)mediasource, MEEndOfPresentation, NULL);
>
> skip_source_tests:
> - IMFMediaStream_Release(video_stream);
> IMFMediaTypeHandler_Release(handler);
> IMFPresentationDescriptor_Release(descriptor);
>
> diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c
> index 9585b0101ba..581d30ced8f 100644
> --- a/dlls/winegstreamer/media_source.c
> +++ b/dlls/winegstreamer/media_source.c
> @@ -55,14 +55,39 @@ struct media_stream
>      {
>          STREAM_INACTIVE,
>          STREAM_SHUTDOWN,
> -
>    STREAM_RUNNING,
> } state;
>    DWORD stream_id;
> - BOOL eos;
>
> +};
> -
>
> +enum source_async_op
> +{
> - SOURCE_ASYNC_START,
>
> +};
> -
>
> +struct source_async_command
> +{
> - IUnknown IUnknown_iface;
> - LONG refcount;
> - enum source_async_op op;
> - union
> - {
> -
>    struct
> -
>    {
> -
>        IMFPresentationDescriptor *descriptor;
> -
>        GUID format;
> -
>        PROPVARIANT position;
> -
>    } start;
> - } u;
>
> };
>
> struct media_source
>  {
>      IMFMediaSource IMFMediaSource_iface;
> - IMFAsyncCallback async_commands_callback;
> LONG ref;
> - DWORD async_commands_queue;
> IMFMediaEventQueue *event_queue;
> IMFByteStream *byte_stream;
> struct media_stream **streams;
>
> @@ -76,6 +101,7 @@ struct media_source
>      {
>          SOURCE_OPENING,
>          SOURCE_STOPPED,
> -
>    SOURCE_RUNNING,
>    SOURCE_SHUTDOWN,
> } state;
>    HANDLE no_more_pads_event;
>
> @@ -91,7 +117,266 @@ static inline struct media_source *impl_from_IMFMediaSource(IMFMediaSource *ifac
>      return CONTAINING_RECORD(iface, struct media_source, IMFMediaSource_iface);
>  }
>
> -static GstFlowReturn bytestream_wrapper_pull(GstPad *pad, GstObject *parent, guint64 ofs, guint len,
> +static inline struct media_source *impl_from_async_commands_callback_IMFAsyncCallback(IMFAsyncCallback *iface)
> +{
> - return CONTAINING_RECORD(iface, struct media_source, async_commands_callback);
>
> +}
> -
>
> +static inline struct source_async_command *impl_from_async_command_IUnknown(IUnknown *iface)
> +{
> - return CONTAINING_RECORD(iface, struct source_async_command, IUnknown_iface);
>
> +}
> -
>
> +static HRESULT WINAPI source_async_command_QueryInterface(IUnknown *iface, REFIID riid, void **obj)
> +{
> - if (IsEqualIID(riid, &IID_IUnknown))
> - {
> -
>    *obj = iface;
> -
>    IUnknown_AddRef(iface);
> -
>    return S_OK;
> - }
> -
> - WARN("Unsupported interface %s.\n", debugstr_guid(riid));
> - *obj = NULL;
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI source_async_command_AddRef(IUnknown *iface)
> +{
> - struct source_async_command *command = impl_from_async_command_IUnknown(iface);
> - return InterlockedIncrement(&command->refcount);
>
> +}
> -
>
> +static ULONG WINAPI source_async_command_Release(IUnknown *iface)
> +{
> - struct source_async_command *command = impl_from_async_command_IUnknown(iface);
> - ULONG refcount = InterlockedDecrement(&command->refcount);
> -
> - if (!refcount)
> - {
> -
>    if (command->op == SOURCE_ASYNC_START)
> -
>        PropVariantClear(&command->u.start.position);
> -
>    heap_free(command);
> - }
> -
> - return refcount;
>
> +}
> -
>
> +static const IUnknownVtbl source_async_command_vtbl =
> +{
> - source_async_command_QueryInterface,
> - source_async_command_AddRef,
> - source_async_command_Release,
>
> +};
> -
>
> +static HRESULT source_create_async_op(enum source_async_op op, struct source_async_command **ret)
> +{
> - struct source_async_command *command;
> -
> - if (!(command = heap_alloc_zero(sizeof(*command))))
> -
>    return E_OUTOFMEMORY;
> -
> - command->IUnknown_iface.lpVtbl = &source_async_command_vtbl;
> - command->op = op;
> -
> - *ret = command;
> -
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI 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 HRESULT WINAPI callback_GetParameters(IMFAsyncCallback *iface,
> -
>    DWORD *flags, DWORD *queue)
>
> +{
> - return E_NOTIMPL;
>
> +}
> -
>
> +static ULONG WINAPI source_async_commands_callback_AddRef(IMFAsyncCallback *iface)
> +{
> - struct media_source *source = impl_from_async_commands_callback_IMFAsyncCallback(iface);
> - return IMFMediaSource_AddRef(&source->IMFMediaSource_iface);
>
> +}
> -
>
> +static ULONG WINAPI source_async_commands_callback_Release(IMFAsyncCallback *iface)
> +{
> - struct media_source *source = impl_from_async_commands_callback_IMFAsyncCallback(iface);
> - return IMFMediaSource_Release(&source->IMFMediaSource_iface);
>
> +}
> -
>
> +static IMFStreamDescriptor *stream_descriptor_from_id(IMFPresentationDescriptor *pres_desc, DWORD id, BOOL *selected)
> +{
> - ULONG sd_count;
> - IMFStreamDescriptor *ret;
> -
> - if (FAILED(IMFPresentationDescriptor_GetStreamDescriptorCount(pres_desc, &sd_count)))
> -
>    return NULL;
> -
> - for (unsigned int i = 0; i < sd_count; i++)

Misplaced variable initializer.
> - {
> -
>    DWORD stream_id;
> -
> -
>    if (FAILED(IMFPresentationDescriptor_GetStreamDescriptorByIndex(pres_desc, i, selected, &ret)))
> -
>        return NULL;
> -
> -
>    if (SUCCEEDED(IMFStreamDescriptor_GetStreamIdentifier(ret, &stream_id)) && stream_id == id)
> -
>        return ret;
> -
> -
>    IMFStreamDescriptor_Release(ret);
> - }
> - return NULL;
>
> +}
> -
>
> +static HRESULT start_pipeline(struct media_source *source, struct source_async_command *command)
> +{
> - PROPVARIANT *position = &command->u.start.position;
> - BOOL seek_message = source->state != SOURCE_STOPPED && position->vt != VT_EMPTY;
> -
> - gst_element_set_state(source->container, GST_STATE_PAUSED);
> - assert(gst_element_get_state(source->container, NULL, NULL, -1) == GST_STATE_CHANGE_SUCCESS);

It's bad practice to put code with side-effects inside of assert(); it
may be a macro which evaluates to nothing (i.e. if NDEBUG is defined).
> -
> - /* seek to beginning on stop->play */
> - if (source->state == SOURCE_STOPPED && position->vt == VT_EMPTY)
> - {
> -
>    position->vt = VT_I8;
> -
>    position->u.hVal.QuadPart = 0;
> - }
> -
> - for (unsigned int i = 0; i < source->stream_count; i++)

Misplaced variable initializer.
> - {
> -
>    struct media_stream *stream;
> -
>    IMFStreamDescriptor *sd;
> -
>    IMFMediaTypeHandler *mth;
> -
>    IMFMediaType *current_mt;
> -
>    GstCaps *current_caps;
> -
>    GstCaps *prev_caps;
> -
>    DWORD stream_id;
> -
>    BOOL was_active;
> -
>    BOOL selected;
> -
>    BOOL changed_caps;
> -
> -
>    stream = source->streams[i];
> -
> -
>    IMFStreamDescriptor_GetStreamIdentifier(stream->descriptor, &stream_id);
> -
> -
>    sd = stream_descriptor_from_id(command->u.start.descriptor, stream_id, &selected);
> -
>    IMFStreamDescriptor_Release(sd);
> -
> -
>    was_active = stream->state != STREAM_INACTIVE;
> -
> -
>    stream->state = selected ? STREAM_RUNNING : STREAM_INACTIVE;
> -
> -
>    if (selected)
> -
>    {
> -
>        IMFStreamDescriptor_GetMediaTypeHandler(stream->descriptor, &mth);
> -
>        IMFMediaTypeHandler_GetCurrentMediaType(mth, &current_mt);
> -
>        current_caps = caps_from_mf_media_type(current_mt);
> -
>        g_object_get(stream->appsink, "caps", &prev_caps, NULL);
> -
>        changed_caps = !gst_caps_is_equal(prev_caps, current_caps);

Why is this a local variable?
> -
> -
>        if (changed_caps)
> -
>        {
> -
>            GstEvent *reconfigure_event = gst_event_new_reconfigure();
> -
>            g_object_set(stream->appsink, "caps", current_caps, NULL);
> -
>            gst_pad_push_event(gst_element_get_static_pad(stream->appsink, "sink"), reconfigure_event);
> -
>        }
> -
> -
>        gst_caps_unref(current_caps);
> -
>        gst_caps_unref(prev_caps);
> -
>        IMFMediaType_Release(current_mt);
> -
>        IMFMediaTypeHandler_Release(mth);
> -
>    }
> -
> -
>    if (position->vt != VT_EMPTY)
> -
>    {
> -
>        GstEvent *seek_event = gst_event_new_seek(1.0, GST_FORMAT_TIME, GST_SEEK_FLAG_FLUSH,
> -
>                GST_SEEK_TYPE_SET, position->u.hVal.QuadPart / 100, GST_SEEK_TYPE_NONE, 0);
> -
>        GstMessage *reset_time;
> -
> -
>        gst_pad_push_event(stream->my_sink, seek_event);
> -
>        if ((reset_time = gst_bus_poll(source->bus, GST_MESSAGE_RESET_TIME, -1)))
> -
>            gst_message_unref(reset_time);

You really don't want to call gst_bus_poll(), and why do you need to do
this anyway?
> -
> -
>        stream->eos = FALSE;
> -
>    }
> -
> -
>    if (selected)
> -
>    {
> -
>        TRACE("Stream %u (%p) selected\n", i, stream);
> -
>        IMFMediaEventQueue_QueueEventParamUnk(source->event_queue,
> -
>        was_active ? MEUpdatedStream : MENewStream, &GUID_NULL,
> -
>        S_OK, (IUnknown*) &stream->IMFMediaStream_iface);
> -
> -
>        IMFMediaEventQueue_QueueEventParamVar(stream->event_queue,
> -
>            seek_message ? MEStreamSeeked : MEStreamStarted, &GUID_NULL, S_OK, position);
> -
>    }
> - }
> -
> - IMFMediaEventQueue_QueueEventParamVar(source->event_queue,
> -
>    seek_message ? MESourceSeeked : MESourceStarted,
> -
>    &GUID_NULL, S_OK, position);
> -
> - source->state = SOURCE_RUNNING;
> -
> - gst_element_set_state(source->container, GST_STATE_PLAYING);
> - gst_element_get_state(source->container, NULL, NULL, -1);
> -
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFAsyncResult *result)
> +{
> - struct media_source *source = impl_from_async_commands_callback_IMFAsyncCallback(iface);
> - struct source_async_command *command;
> - IUnknown *state;
> - HRESULT hr;
> -
> - if (source->state == SOURCE_SHUTDOWN)
> -
>    return S_OK;
> -
> - if (FAILED(hr = IMFAsyncResult_GetState(result, &state)))
> -
>    return hr;
> -
> - command = impl_from_async_command_IUnknown(state);
> - switch (command->op)
> - {
> -
>    case SOURCE_ASYNC_START:
> -
>        start_pipeline(source, command);
> -
>        break;
> -
>    default:
> -
>        ;

"break" may be a little more idiomatic, but I would just omit the
"default" branch.
> - }
> -
> - IUnknown_Release(state);
> -
> - return S_OK;
>
> +}
> -
>
> +static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl =
> +{
> - callback_QueryInterface,
> - source_async_commands_callback_AddRef,
> - source_async_commands_callback_Release,
> - callback_GetParameters,
> - source_async_commands_Invoke,
>
> +};
> -
>
> +GstFlowReturn bytestream_wrapper_pull(GstPad *pad, GstObject *parent, guint64 ofs, guint len,
>          GstBuffer **buf)
>  {
>      struct media_source *source = gst_pad_get_element_private(pad);
> @@ -236,6 +521,8 @@ GstBusSyncReply bus_watch(GstBus *bus, GstMessage *message, gpointer user)
>              g_error_free(err);
>              g_free(dbg_info);
>              break;
> -
>    case GST_MESSAGE_RESET_TIME:
> -
>        return GST_BUS_PASS;
>    default:
>        break;
> }
>
> @@ -669,16 +956,30 @@ static HRESULT WINAPI media_source_CreatePresentationDescriptor(IMFMediaSource *
>  }
>
> static HRESULT WINAPI media_source_Start(IMFMediaSource *iface, IMFPresentationDescriptor *descriptor,
> -
>                                 const GUID *time_format, const PROPVARIANT *start_position)
> -
>                                 const GUID *time_format, const PROPVARIANT *position)
>
> {
>      struct media_source *source = impl_from_IMFMediaSource(iface);
> - struct source_async_command *command;
> - HRESULT hr;
> - FIXME("(%p)->(%p, %p, %p): stub\n", source, descriptor, time_format, start_position);
> -
>
> TRACE("(%p)->(%p, %p, %p)\n", source, descriptor, time_format, position);
>
> if (source->state == SOURCE_SHUTDOWN)
>     return MF_E_SHUTDOWN;
> - return E_NOTIMPL;
> - if (!(IsEqualIID(time_format, &GUID_NULL)))
> -
>    return MF_E_UNSUPPORTED_TIME_FORMAT;
> -
> - if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_START, &command)))
> - {
> -
>    command->u.start.descriptor = descriptor;
> -
>    command->u.start.format = *time_format;
> -
>    PropVariantCopy(&command->u.start.position, position);
> -
> -
>    hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, &command->IUnknown_iface);
> - }
> -
> - return hr;
>
> }
>
> static HRESULT WINAPI media_source_Stop(IMFMediaSource *iface)
> @@ -759,6 +1060,9 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
>      if (source->no_more_pads_event)
>          CloseHandle(source->no_more_pads_event);
> - if (source->async_commands_queue)
> -
>    MFUnlockWorkQueue(source->async_commands_queue);
> - return S_OK;
>
> }
>
> @@ -839,6 +1143,7 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_
>          return E_OUTOFMEMORY;
>  object->IMFMediaSource_iface.lpVtbl = &IMFMediaSource_vtbl;
> - object->async_commands_callback.lpVtbl = &source_async_commands_callback_vtbl;
> object->ref = 1;
> object->byte_stream = bytestream;
> IMFByteStream_AddRef(bytestream);
>
> @@ -847,6 +1152,9 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_
>      if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
>          goto fail;
> - if (FAILED(hr = MFAllocateWorkQueue(&object->async_commands_queue)))
> -
>    goto fail;
> - object->container = gst_bin_new(NULL);
>    object->bus = gst_bus_new();
>    gst_bus_set_sync_handler(object->bus, mf_src_bus_watch_wrapper, object, NULL);