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

[Zebediah Figura (she/her)]

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
>
> 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.
>  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. 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, that
option is starting to look more attractive, especially if we want to
make winegstreamer compatible with WoW64 emulation.)
> @@ -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)
>    {