[PATCH 0/1] MR2546: winegstreamer: Create extra decodebin to decode compressed stream.

[PATCH 1/1] winegstreamer: Create extra decodebin to decode compressed stream.

[Ziqing Hui]

From: Ziqing Hui

---
 dlls/winegstreamer/wg_parser.c | 168 +++++++++++++++++++++++++++------
 1 file changed, 141 insertions(+), 27 deletions(-)

diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
index 5bb824f4399..1a07ff6663e 100644
--- a/dlls/winegstreamer/wg_parser.c
+++ b/dlls/winegstreamer/wg_parser.c
@@ -57,8 +57,30 @@ typedef enum
 GST_DEBUG_CATEGORY(wine);
 #define GST_CAT_DEFAULT wine

+static const char *raw_caps[] =
+{
+    "video/x-raw",
+    "audio/x-raw",
+    "text/x-raw",
+};
+
+static const char *compressed_caps[] =
+{
+    "video/x-wmv",
+    "video/x-h264",
+    "audio/x-mpeg",
+    "audio/x-wma",
+};
+
 typedef BOOL (*init_gst_cb)(struct wg_parser *parser);

+static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user);
+static void pad_removed_cb(GstElement *element, GstPad *pad, gpointer user);
+static gboolean autoplug_continue_cb(GstElement * decodebin, GstPad *pad, GstCaps * caps, gpointer user);
+static GstAutoplugSelectResult autoplug_select_cb(GstElement *bin, GstPad *pad,
+        GstCaps *caps, GstElementFactory *fact, gpointer user);
+static void no_more_pads_cb(GstElement *element, gpointer user);
+
 struct wg_parser
 {
     init_gst_cb init_gst;
@@ -104,7 +126,7 @@ struct wg_parser_stream
     uint32_t number;

GstPad *their_src, *post_sink, *post_src, *my_sink;
-    GstElement *flip;
+    GstElement *flip, *decodebin;
     GstSegment segment;
     struct wg_format preferred_format, current_format;

@@ -112,7 +134,7 @@ struct wg_parser_stream
     GstBuffer *buffer;
     GstMapInfo map_info;

-    bool flushing, eos, enabled, has_caps, has_tags, has_buffer;
+    bool flushing, eos, enabled, has_caps, has_tags, has_buffer, no_more_pads;

uint64_t duration;
     gchar *tags[WG_PARSER_TAG_COUNT];
@@ -476,6 +498,58 @@ static NTSTATUS wg_parser_stream_notify_qos(void *args)
     return S_OK;
 }

+static bool no_more_pads(struct wg_parser *parser)
+{
+    unsigned int i;
+
+    for (i = 0; i < parser->stream_count; ++i)
+    {
+        if (!parser->streams[i]->no_more_pads)
+            return FALSE;
+    }
+
+    return parser->no_more_pads;
+}
+
+static struct wg_parser_stream *get_stream_by_decodebin(struct wg_parser *parser, GstElement *decodebin)
+{
+    unsigned int i;
+
+    for (i = 0; i < parser->stream_count; ++i)
+    {
+        if (decodebin == parser->streams[i]->decodebin)
+            return parser->streams[i];
+    }
+
+    return NULL;
+}
+
+static gboolean autoplug_continue_cb(GstElement * decodebin, GstPad *pad, GstCaps * caps, gpointer user)
+{
+    struct wg_parser *parser = user;
+    const char *caps_name;
+    unsigned int i;
+
+    caps_name = gst_structure_get_name(gst_caps_get_structure(caps, 0));
+
+    for (i = 0; i < ARRAY_SIZE(raw_caps); ++i)
+    {
+        if (!strcmp(caps_name, raw_caps[i]))
+            return FALSE;
+    }
+
+    if (decodebin == parser->decodebin)
+    {
+        for (i = 0; i < ARRAY_SIZE(compressed_caps); ++i)
+        {
+            if (!strcmp(caps_name, compressed_caps[i]))
+                return FALSE;
+        }
+    }
+
+    return TRUE;
+}
+
 static GstAutoplugSelectResult autoplug_select_cb(GstElement *bin, GstPad *pad,
         GstCaps *caps, GstElementFactory *fact, gpointer user)
 {
@@ -505,11 +579,15 @@ static GstAutoplugSelectResult autoplug_select_cb(GstElement *bin, GstPad *pad,
 static void no_more_pads_cb(GstElement *element, gpointer user)
 {
     struct wg_parser *parser = user;
+    struct wg_parser_stream *stream;

-    GST_DEBUG("parser %p.", parser);
+    GST_DEBUG("parser %p, element %p", parser, element);

pthread_mutex_lock(&parser->mutex);
-    parser->no_more_pads = true;
+    if ((stream = get_stream_by_decodebin(parser, element)) != NULL)
+        stream->no_more_pads = true;
+    else
+        parser->no_more_pads = true;
     pthread_mutex_unlock(&parser->mutex);
     pthread_cond_signal(&parser->init_cond);
 }
@@ -766,6 +844,29 @@ GstElement *create_element(const char *name, const char *plugin_set)
     return element;
 }

+GstElement *create_decodebin(struct wg_parser *parser)
+{
+    GstElement *decodebin;
+
+    if (!(decodebin = create_element("decodebin", "base")))
+        return NULL;
+    gst_bin_add(GST_BIN(parser->container), decodebin);
+
+    if (parser->unlimited_buffering)
+    {
+        g_object_set(decodebin, "max-size-buffers", G_MAXUINT, NULL);
+        g_object_set(decodebin, "max-size-time", G_MAXUINT64, NULL);
+        g_object_set(decodebin, "max-size-bytes", G_MAXUINT, NULL);
+    }
+    g_signal_connect(decodebin, "pad-added", G_CALLBACK(pad_added_cb), parser);
+    g_signal_connect(decodebin, "pad-removed", G_CALLBACK(pad_removed_cb), parser);
+    g_signal_connect(decodebin, "autoplug-continue", G_CALLBACK(autoplug_continue_cb), parser);
+    g_signal_connect(decodebin, "autoplug-select", G_CALLBACK(autoplug_select_cb), parser);
+    g_signal_connect(decodebin, "no-more-pads", G_CALLBACK(no_more_pads_cb), parser);
+
+    return decodebin;
+}
+
 static struct wg_parser_stream *create_stream(struct wg_parser *parser)
 {
     struct wg_parser_stream *stream, **new_array;
@@ -782,6 +883,7 @@ static struct wg_parser_stream *create_stream(struct wg_parser *parser)

stream->parser = parser;
     stream->number = parser->stream_count;
+    stream->no_more_pads = true;
     stream->current_format.major_type = WG_MAJOR_TYPE_UNKNOWN;
     pthread_cond_init(&stream->event_cond, NULL);
     pthread_cond_init(&stream->event_empty_cond, NULL);
@@ -826,11 +928,12 @@ static void free_stream(struct wg_parser_stream *stream)

static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user)
 {
+    struct wg_parser_stream *stream = NULL;
+    GstPad *stream_decodebin_sink;
     struct wg_parser *parser = user;
-    struct wg_parser_stream *stream;
     const char *name;
     GstCaps *caps;
-    int ret;
+    int ret, i;

GST_LOG("parser %p, element %p, pad %p.", parser, element, pad);

@@ -840,8 +943,35 @@ static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user)
     caps = gst_pad_query_caps(pad, NULL);
     name = gst_structure_get_name(gst_caps_get_structure(caps, 0));

-    if (!(stream = create_stream(parser)))
-        goto out;
+    if (!(stream = get_stream_by_decodebin(parser, element)))
+    {
+        if (!(stream = create_stream(parser)))
+            goto out;
+    }
+
+    if (element == parser->decodebin)
+    {
+        /* Create stream decodebin to decode compressed stream. */
+        for (i = 0; i < ARRAY_SIZE(compressed_caps); ++i)
+        {
+            if (!strcmp(name, compressed_caps[i]))
+            {
+                stream->decodebin = create_decodebin(parser);
+
+                pthread_mutex_lock(&parser->mutex);
+                stream->no_more_pads = false;
+                pthread_mutex_unlock(&parser->mutex);
+                gst_element_sync_state_with_parent(stream->decodebin);
+
+                stream_decodebin_sink = gst_element_get_static_pad(stream->decodebin, "sink");
+                if ((ret = gst_pad_link(pad, stream_decodebin_sink)) < 0)
+                    GST_ERROR("Failed to link pads, error %d.", ret);
+                gst_object_unref(stream_decodebin_sink);
+
+                goto out;
+            }
+        }
+    }

if (!strcmp(name, "video/x-raw"))
     {
@@ -1398,7 +1528,7 @@ static NTSTATUS wg_parser_connect(void *args)

pthread_mutex_lock(&parser->mutex);

-    while (!parser->no_more_pads && !parser->error)
+    while (!no_more_pads(parser) && !parser->error)
         pthread_cond_wait(&parser->init_cond, &parser->mutex);
     if (parser->error)
     {
@@ -1567,28 +1697,12 @@ static NTSTATUS wg_parser_disconnect(void *args)

static BOOL decodebin_parser_init_gst(struct wg_parser *parser)
 {
-    GstElement *element;
     int ret;

-    if (!(element = create_element("decodebin", "base")))
+    if (!(parser->decodebin = create_decodebin(parser)))
         return FALSE;

-    gst_bin_add(GST_BIN(parser->container), element);
-    parser->decodebin = element;
-
-    if (parser->unlimited_buffering)
-    {
-        g_object_set(parser->decodebin, "max-size-buffers", G_MAXUINT, NULL);
-        g_object_set(parser->decodebin, "max-size-time", G_MAXUINT64, NULL);
-        g_object_set(parser->decodebin, "max-size-bytes", G_MAXUINT, NULL);
-    }
-
-    g_signal_connect(element, "pad-added", G_CALLBACK(pad_added_cb), parser);
-    g_signal_connect(element, "pad-removed", G_CALLBACK(pad_removed_cb), parser);
-    g_signal_connect(element, "autoplug-select", G_CALLBACK(autoplug_select_cb), parser);
-    g_signal_connect(element, "no-more-pads", G_CALLBACK(no_more_pads_cb), parser);
-
-    parser->their_sink = gst_element_get_static_pad(element, "sink");
+    parser->their_sink = gst_element_get_static_pad(parser->decodebin, "sink");

pthread_mutex_lock(&parser->mutex);
     parser->no_more_pads = false;

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c:
> - return NULL;
>
> +}
> -
>
> +static gboolean autoplug_continue_cb(GstElement * decodebin, GstPad *pad, GstCaps * caps, gpointer user)
> +{
> - struct wg_parser *parser = user;
> - const char *caps_name;
> - unsigned int i;
> -
> - caps_name = gst_structure_get_name(gst_caps_get_structure(caps, 0));
> -
> - for (i = 0; i < ARRAY_SIZE(raw_caps); ++i)
> - {
> -
>    if (!strcmp(caps_name, raw_caps[i]))
> -
>        return FALSE;
> - }

We shouldn't need this part; decodebin will detect that the caps are raw and stop anyway.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c:
> -
> - caps_name = gst_structure_get_name(gst_caps_get_structure(caps, 0));
> -
> - for (i = 0; i < ARRAY_SIZE(raw_caps); ++i)
> - {
> -
>    if (!strcmp(caps_name, raw_caps[i]))
> -
>        return FALSE;
> - }
> -
> - if (decodebin == parser->decodebin)
> - {
> -
>    for (i = 0; i < ARRAY_SIZE(compressed_caps); ++i)
> -
>    {
> -
>        if (!strcmp(caps_name, compressed_caps[i]))
> -
>            return FALSE;
> -
>    }

I don't think we quite want to do it this way, rather we should probably just test if we can convert it to a wg_format. We'll need to do that anyway. Then we can just set a flag on the stream rather than later checking the array again.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c:
> -
> - if (parser->unlimited_buffering)
> - {
> -
>    g_object_set(decodebin, "max-size-buffers", G_MAXUINT, NULL);
> -
>    g_object_set(decodebin, "max-size-time", G_MAXUINT64, NULL);
> -
>    g_object_set(decodebin, "max-size-bytes", G_MAXUINT, NULL);
> - }
> - g_signal_connect(decodebin, "pad-added", G_CALLBACK(pad_added_cb), parser);
> - g_signal_connect(decodebin, "pad-removed", G_CALLBACK(pad_removed_cb), parser);
> - g_signal_connect(decodebin, "autoplug-continue", G_CALLBACK(autoplug_continue_cb), parser);
> - g_signal_connect(decodebin, "autoplug-select", G_CALLBACK(autoplug_select_cb), parser);
> - g_signal_connect(decodebin, "no-more-pads", G_CALLBACK(no_more_pads_cb), parser);
> -
> - return decodebin;
>
> +}
> -

I'm not sure I like using the same callbacks for both decodebins. pad-added does share a lot of code for postprocessing, but that could easily be a helper function. pad-removed isn't accounted for in this patch (and should be) and I think it needs to do something completely different for compressed and uncompressed elements. no-more-pads is small but can still afford to be duplicated.

Using separate callbacks for everything (except autoplug-select) strikes me as a bit architecturally clearer. It also has the benefit that get_stream_by_decodebin() is no longer needed.

[Ziqing Hui (＠zhui)]

On Fri Mar 31 19:35:56 2023 +0000, Zebediah Figura wrote:
> I don't think we quite want to do it this way, rather we should probably
> just test if we can convert it to a wg_format. We'll need to do that
> anyway. Then we can just set a flag on the stream rather than later
> checking the array again.

It seems that we can't set a flag to stream in autoplug_continue_cb(), because stream is created in pad_added_cb(), we can get the stream in autoplug_continue_cb() I think.

[Ziqing Hui (＠zhui)]

> pad-removed isn't accounted for in this patch (and should be) and I think it needs to do something completely different for compressed and uncompressed elements.

I'm not sure what else to do in extra decodebin's pad-removed, expect for the existing unlink stuff.

[Ziqing Hui (＠zhui)]

On Mon Apr  3 02:06:54 2023 +0000, Ziqing Hui wrote:
> It seems that we can't set a flag to stream in autoplug_continue_cb(),
> because stream is created in pad_added_cb(), we can not get the stream
> in autoplug_continue_cb() I think.
> And yeah, I do agree we should convert it to wg_format.

I try to convert caps to wg_format in autoplug_continue_cb(), then run wmvcore tests. However, Gst complains about:
`ERROR             audio-info audio-info.c:302:gst_audio_info_from_caps: no rate property given`

I think the caps given in autoplug_continue_cb() is not complete, we cannot convert it to wg_format (without changing the existing wg_format_from_caps). So we should test the caps name in autoplug_continue_cb().

[Ziqing Hui (＠zhui)]

On Mon Apr  3 03:45:50 2023 +0000, Ziqing Hui wrote:
> I try to convert caps to wg_format in autoplug_continue_cb(), then run
> wmvcore tests. However, Gst complains:
> `ERROR             audio-info audio-info.c:302:gst_audio_info_from_caps:
> no rate property given`
> I think the caps given in autoplug_continue_cb() is not complete, we
> cannot convert it to wg_format (without changing the existing
> wg_format_from_caps). So we should test the caps name in autoplug_continue_cb().

I found that `gst_audio_info_from_caps error` also happens in pad_added_cb(). The caps from the pad given in pad_added_cb() is also missing rate property.