[PATCH 1/5] winegstreamer: Activate source pad in push mode if it isn't activated in pull mode.

[Zebediah Figura (she/her)]

On 3/11/21 2:29 PM, Derek Lesho wrote:
> On 3/11/21 3:15 PM, Zebediah Figura (she/her) wrote:
> 
>> On 3/10/21 1:33 PM, Derek Lesho wrote:
>>> Since our source pad is not part of any element, gstreamer won't end
>>> up activating it
>>> directly through the state transition.  Instead, if the downstream
>>> element doesn't
>>> activate the source pad into pull mode during the transition to the
>>> READY state,
>>> we activate our pad in push mode.
>>>
>>> Signed-off-by: Derek Lesho <dlesho at codeweavers.com>
>>> ---
>>>   dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/dlls/winegstreamer/wg_parser.c
>>> b/dlls/winegstreamer/wg_parser.c
>>> index b8dd75d5fff..67f65264a8d 100644
>>> --- a/dlls/winegstreamer/wg_parser.c
>>> +++ b/dlls/winegstreamer/wg_parser.c
>>> @@ -58,7 +58,7 @@ struct wg_parser
>>>       pthread_mutex_t mutex;
>>>         pthread_cond_t init_cond;
>>> -    bool no_more_pads, has_duration, error;
>>> +    bool no_more_pads, has_duration, error, pull_mode;
>>>         pthread_cond_t read_cond, read_done_cond;
>>>       struct
>>> @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad
>>> *pad, GstObject *parent, GstPadMode
>>>       GST_DEBUG("%s source pad for parser %p in %s mode.",
>>>               activate ? "Activating" : "Deactivating", parser,
>>> gst_pad_mode_get_name(mode));
>>>   +    parser->pull_mode = false;
>>> +
>> I think this statement is redundant—the field will already be
>> initialized to zero, and I don't think we should be activated in
>> pull-mode directly after being activated in push-mode.
> Makes sense.
>>
>>>       switch (mode)
>>>       {
>>>           case GST_PAD_MODE_PULL:
>>> +            parser->pull_mode = activate;
>>>               return TRUE;
>>>           case GST_PAD_MODE_PUSH:
>>>               return activate_push(pad, activate);
>>> @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct
>>> wg_parser *parser)
>>>       pthread_mutex_unlock(&parser->mutex);
>>>         gst_element_set_state(parser->container, GST_STATE_NULL);
>>> +    if (!parser->pull_mode)
>>> +        gst_pad_set_active(parser->my_src, 0);
>>>       gst_pad_unlink(parser->my_src, parser->their_sink);
>>>       gst_object_unref(parser->my_src);
>>>       gst_object_unref(parser->their_sink);
>>> @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct
>>> wg_parser *parser)
>>>       }
>>>         gst_element_set_state(parser->container, GST_STATE_PAUSED);
>>> +    if (!parser->pull_mode)
>>> +        gst_pad_set_active(parser->my_src, 1);
>>>       ret = gst_element_get_state(parser->container, NULL, NULL, -1);
>>>       if (ret == GST_STATE_CHANGE_FAILURE)
>>>       {
>> I think this and the following should happen after the
>> gst_element_get_state() call, right? We shouldn't spawn a thread if
>> state change fails.
>>
>> In theory that might risk a deadlock—i.e. GStreamer doesn't make
>> guarantees that NULL->READY->PAUSED can happen without any buffers being
>> pushed—but I don't know if it happens in practice.
> 
> Yeah, it does deadlock and I think Gstreamer specifies that the PAUSED
> state indicates that pipeline is prerolled.
> 
> https://gstreamer.freedesktop.org/documentation/plugin-development/basics/states.html?gi-language=c
> 
> 
> "GST_STATE_PAUSED is ... At this point the pipeline is 'prerolled' and
> ready to render data immediately."

That's assuming that the sink prerolls—which most sinks do, but we don't.

> 
> For what it's worth, it isn't guaranteed (although in practice it seems
> to be the case) that even the transition from NULL to READY is
> completely synchronous.  The most correct code would involve first
> transitioning to READY, checking the result, activating the pad, then
> continuing to transition to PAUSED.

Unfortunately it's not quite that simple. Pads are activated on
READY->PAUSED, not on NULL->READY, and upstream pads have to be
activated *after* downstream ones.

> 
>>   The really correct
>> solution to that deadlock is to create our own element (or, actually,
>> use appsrc—now that winegstreamer is split into PE and Unix parts
> Yeah, I still believe GstAppSrc is the cleaner solution, as the
> interface is quite simple and well documented.  But functionally, the
> manual pad mode should work just as well, it's up to you.

I don't remember what discussion we had about appsrc in the first place
(if we had any?), but I think my argument was that it wasn't really a
*better* fit than using getrange callbacks directly, i.e. it wasn't
saving us much work (at best it would have saved us the need to
separately handle push mode, but it was better to simply not implement
push mode in the media source).

But that's less true now, given the way that read requests are handled
at the Unix library boundary, and the combined backend interface for
DirectShow and Media Foundation, and I think appsrc may be a better fit
now. The "need-data"/"push-data" model looks like a better fit, that
might allow us to avoid the current awkward callback marshalling, and
we'd be able to get rid of push-mode handling completely.

>> , that
>> option is starting to look more attractive, especially if we want to
>> make winegstreamer compatible with WoW64 emulation.)
> I'm not sure I see how this is relevant to the WoW64 emulation, if you
> don't mind, could you explain this?

One reason that appsrc is undesirable (which I probably didn't mention
earlier) is that it always takes buffers allocated by the "app", as it
were, rather than by the element calling gst_pad_get_range(). That's
fine if the downstream element didn't allocate its own buffer, but if it
did (and oggdemux is one major example of an element that does) appsrc
will do an extra blit.

That should be avoided if possible. I'm not particularly concerned about
latency—I don't think it should be a bottleneck, as long as the machine
has enough cores—but I am more concerned about CPU usage, and cases
where we might be CPU limited and *don't* have enough cores. Yes, it's
probably premature optimization, but I'm a lot warier about removing an
optimization already present than I am about introducing a new one.

Currently we fill read buffers allocated by the Unix side—either already
allocated by a GStreamer element, or allocated by us using
gst_buffer_new_and_alloc(). That's not compatible with WoW64 emulation
(aka 32-on-64-bit support), as it requires the 32-bit PE side to write
into a 64-bit pointer. Instead we would need to write into a buffer
allocated from the 32-bit side, and pass that to the Unix side. But this
also means that we have to do a blit.

[Note that we can in theory avoid the blit by wrapping the
caller-allocated buffer in a custom GstBuffer class and passing that
down to the Unix side. We still have to do a blit if reading into a
downstream-allocated GstBuffer, but we can at least avoid it otherwise.]

>>
>>> @@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct
>>> wg_parser *parser)
>>>       }
>>>         gst_element_set_state(parser->container, GST_STATE_PAUSED);
>>> +    if (!parser->pull_mode)
>>> +        gst_pad_set_active(parser->my_src, 1);
>>>       ret = gst_element_get_state(parser->container, NULL, NULL, -1);
>>>       if (ret == GST_STATE_CHANGE_FAILURE)
>>>       {
>>> @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct
>>> wg_parser *parser)
>>>         gst_pad_set_active(stream->my_sink, 1);
>>>       gst_element_set_state(parser->container, GST_STATE_PAUSED);
>>> +    if (!parser->pull_mode)
>>> +        gst_pad_set_active(parser->my_src, 1);
>>>       ret = gst_element_get_state(parser->container, NULL, NULL, -1);
>>>       if (ret == GST_STATE_CHANGE_FAILURE)
>>>       {
>>> @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct
>>> wg_parser *parser)
>>>         gst_pad_set_active(stream->my_sink, 1);
>>>       gst_element_set_state(parser->container, GST_STATE_PAUSED);
>>> +    if (!parser->pull_mode)
>>> +        gst_pad_set_active(parser->my_src, 1);
>>>       ret = gst_element_get_state(parser->container, NULL, NULL, -1);
>>>       if (ret == GST_STATE_CHANGE_FAILURE)
>>>       {
>>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210311/1269259f/attachment.sig>