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

[Derek Lesho]

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."

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.

>   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.
> , 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?
>
>> @@ -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)
>>       {
>>
>