[PATCH v2 4/5] winegstreamer: Implement IMFMediaSource::Start.
Derek Lesho
dlesho at codeweavers.com
Wed Oct 7 15:19:02 CDT 2020
On 10/7/20 1:53 PM, Zebediah Figura wrote:
> On 10/7/20 9:58 AM, Derek Lesho wrote:
>> On 10/6/20 9:58 PM, Zebediah Figura wrote:
>>> On 10/6/20 10:59 AM, Derek Lesho wrote:
>>>> + 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);
>>>> + GstSample *preroll;
>>>> +
>>>> + gst_pad_push_event(stream->my_sink, seek_event);
>>>> +
>>>> + /* this works because the pre-seek preroll is already removed by media_source_constructor */
>>>> + g_signal_emit_by_name(stream->appsink, "pull-preroll", &preroll);
>>>> + if (preroll)
>>>> + gst_sample_unref(preroll);
>>> What is the point of doing this?
>> To wait for the flush to complete. What is the point of waiting for the
>> flush to complete? To ensure we don't send any MEMediaSample events
>> from before the seek after MESourceSeeked.
> I don't think this is necessary; I don't think a flush can be
> asynchronous, nor a seek.
Interesting, so gst_pad_push_event blocks until the seek event is
handled, but I'm not sure if this is before or after the flush start
event has been propogated downstream, as what we're really waiting for
is appsink to receive that and invalidate all buffered samples. I'll
look into this and if just waiting for gst_pad_push_event to return
suffices, I'll remove the hack.
>
>>>> +
>>>> + 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);
>>> And if this method is asynchronous, do you need this
>>> gst_element_get_state() call?
>> Probably not, but not because the method is asynchronous, but rather
>> because it shouldn't affect how we interact with the appsink/s.
>>>> @@ -432,6 +717,8 @@ static HRESULT media_stream_connect_to_sink(struct media_stream *stream)
>>>> if (gst_pad_link(stream->their_src, stream->my_sink) != GST_PAD_LINK_OK)
>>>> return E_FAIL;
>>>>
>>>> + g_object_set(stream->appsink, "caps", source_caps, NULL);
>>>> +
>>> Again, what's the point of this?
>> To see whether the caps have changed since the prior selection in
>> start_pipeline. If they have changed, we must send a reconfigure event.
> This might need clarification in the code, then; it looks redundant at
> first glance.
>
> It may even be better to remove it. I have my doubts that it will make a
> difference in terms of performance; in particular I suspect that the
> caps GStreamer generates and the caps generated by
> caps_from_mf_media_type() will have subtle differences.
>
That's true, I guess I'll remove it.
>>>> + 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);
>>>> + }
>>> There are likely mfplat pecularities I'm not aware of here, but does
>>> this need to be asynchronous? I know that the documentation says "this
>>> method is asynchronous", but it's not immediately obvious to me that
>>> there isn't already a level of asynchronicity between this call and any
>>> of its effects.
>> Well, I think that making the media source calls synchronous in their
>> own thread simplifies things since you don't have to worry about
>> blocking or synchronization with other calls. For example,
>> ::RequestSample, if synchronous, would have to have two paths, one for
>> popping signals from the appsink and one for issuing the request into
>> some dedicated request queue.
> I don't think I understand this; can you please explain in more detail?
Yeah, in my first implementation, I setup a new-sample callback with the
appsink, as well as a queue using a doubly linked list for sample
requests. When ::RequestSample was called, if the appsink had any
buffered samples, I would immediately dispatch the sample with the token
sent in. Otherwise, I would insert a sample request into the queue and
return. When the new-sample callback was run, I'd pop the sample from
the queue and send the event. In IMFMediaSource::Start. I still have
the code for this lying around on my github [1], it was quite an
unnecessary mess.
>
>> This was actually the implementation I
>> had at first, but then I implemented the async reader callback with the
>> same system, and Nikolay instead insisted that I use a command queue,
>> which reduces complexity a lot. Then when adding seeking support, I
>> switched to this approach for the media source. If nothing else, this
>> system matches other media foundation components better
>> (IMFMediaSession, IMFSourceReader), which reduces overhead for the reader.
> Even if RequestSample() needs to be asynchronous, is there a reason that
> Start() needs to be asynchronous? Thread safety is obviously important,
> but that can be achieved with simple locking.
One thing I want to avoid here is blocking too much during a function
which is documented to be asynchronous. We have to wait for the media
streams to finish up responding to requested samples in this function,
which may take a non trivial amount of time. I don't think it's
unreasonable to expect there to be applications which calls
IMFMediaSource methods from their interactive thread, resulting in our
blocking causing a reduction in responsiveness.
>
>>>> +
>>>> + return hr;
>>>> }
>>>>
>>>> static HRESULT WINAPI media_source_Stop(IMFMediaSource *iface)
>>>> @@ -772,6 +1073,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;
>>>> }
>>>>
>>>> @@ -852,6 +1156,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);
>>>> @@ -860,6 +1165,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);
>>>> diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c
>>>> index 2e8b0978648..9aa17ad00ab 100644
>>>> --- a/dlls/winegstreamer/mfplat.c
>>>> +++ b/dlls/winegstreamer/mfplat.c
>>>> @@ -601,3 +601,152 @@ IMFMediaType *mf_media_type_from_caps(const GstCaps *caps)
>>>>
>>>> return media_type;
>>>> }
>>>> +
>>>> +GstCaps *caps_from_mf_media_type(IMFMediaType *type)
>>>> +{
>>>> + GUID major_type;
>>>> + GUID subtype;
>>>> + GstCaps *output = NULL;
>>>> +
>>>> + if (FAILED(IMFMediaType_GetMajorType(type, &major_type)))
>>>> + return NULL;
>>>> + if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &subtype)))
>>>> + return NULL;
>>>> +
>>>> + if (IsEqualGUID(&major_type, &MFMediaType_Video))
>>>> + {
>>>> + UINT64 frame_rate = 0, frame_size = 0;
>>>> + DWORD width, height, framerate_num, framerate_den;
>>>> + UINT32 unused;
>>>> +
>>>> + if (FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size)))
>>>> + return NULL;
>>>> + width = frame_size >> 32;
>>>> + height = frame_size;
>>>> + if (FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_RATE, &frame_rate)))
>>>> + {
>>>> + frame_rate = TRUE;
>>>> + framerate_num = 0;
>>>> + framerate_den = 1;
>>>> + }
>>>> + else
>>>> + {
>>>> + framerate_num = frame_rate >> 32;
>>>> + framerate_den = frame_rate;
>>>> + }
>>>> +
>>>> + /* Check if type is uncompressed */
>>>> + if (SUCCEEDED(MFCalculateImageSize(&subtype, 100, 100, &unused)))
>>> Early return could save a level of indentation.
>> Yes but in future patches there will be else statements for handling
>> compressed types. Even if we don't end up using this for the media
>> source, it will be necessary for games which manually use decoder MFTs.
>>> I also feel like there's an easier way to do this check. Maybe something
>>> like:
>>>
>>> for (i = 0; i < ARRAY_SIZE(uncompressed_video_formats); i++)
>>> {
>>> ...
>>> }
>>>
>>> if (format == GST_VIDEO_FORMAT_UNKNOWN
>>> && !memcmp(&subtype, &MFVideoFormat_Base.Data2, ...))
>>> format = gst_video_format_from_fourcc(subtype.Data1);
>>>
>>> if (format == GST_VIDEO_FORMAT_UNKNOWN)
>>> {
>>> FIXME("Unrecognized subtype %s.\n", debugstr_guid(&subtype));
>>> return NULL;
>>> }
>> That's certainly a possibility, but using MFCalculateImageSize is
>> probably the best way to concisely determine whether we want to take the
>> uncompressed video format route. With your solution, I guess we'd just
>> add the handling for those formats in `if (format ==
>> GST_VIDEO_FORMAT_UNKNOWN)`. Either way is fine, which path should I take?
> I don't see any reason we need to check whether it's uncompressed in
> general; both "an uncompressed format we can't handle" and "a compressed
> format" yield the same result.
Right now they do, but my point is that in future patches the structure
is more like
if (uncompressed) { ... } else if (format == h264) { ... } else if
(format == wmv)
>
>>>> + FIXME("Unrecognized subtype %s\n", debugstr_guid(&subtype));
>>>> + return NULL;
>>>> + }
>>>> +
>>>> +
>>>> + if (frame_size)
>>>> + {
>>>> + gst_caps_set_simple(output, "width", G_TYPE_INT, width, NULL);
>>>> + gst_caps_set_simple(output, "height", G_TYPE_INT, height, NULL);
>>>> + }
>>>> + if (frame_rate)
>>> Why do you need to check this is nonzero? Can the frame rate really be 0/0?
>> To be honest, I don't remember exactly which circumstance led to this
>> being necessary. I assume it was something with the IMFMediaType MK11
>> generates, but it does seem very strange so I'll have to check again.
>>> For that matter, do we need to give GStreamer the framerate at all? It
>>> should only ever matter to sink elements (or those that respect a clock).
>> Why risk seeing what happens when you don't translate this cap, are
>> there any benefits? Maybe a decoder would need it need it to handle
>> some time-based seek?
>>
> In fact, translating the framerate in quartz has caused problems before,
> as applications didn't always provide it, or provided the wrong one, and
> thus the caps would not match; as a result we always remove it. On the
> other hand, I haven't seen any problems arise from clearing this field.
> In practice I don't think it should ever be necessary, because the
> parser provides the framerate.
My feeling is that such a workaround should be put in place where it
causes problems, not in a general conversion function which may be used
(as in later patches) for other purposes where the cap is necessary.
For what it's worth, the chance of a media foundation application
feeding some bogus framerate is slim, as they usually just search the
media type handler for a desired type anyway, and we're supposed to fail
on SetCurrentMediaType if the media type they enter is unsupported.
>
1:
https://github.com/Guy1524/wine/blob/mfplat/dlls/winegstreamer/media_source.c#L98
More information about the wine-devel
mailing list