[PATCH v3 0/1] MR8719: winegstreamer: Only add the capsfilter for avdec_h264.

[Brendan McGrath (＠redmcg)]

avdec_h264 will fail when combined with h264parse and a drain request
is made during caps neg.

To fix this, a capsfilter was added to force a stream-format of
byte-stream for all h.264 decoders. However, this introduced a new
issue with the vtdec element, as it only supports the avc stream-format.

Therefore, this patch looks to address both problems by only introducing
the capsfilter when the avdec_h264 element is used.

It also removes the stream-format and alignment requirements from
the parsed caps, applying them only to the capsfilter.

--
  v3: winegstreamer: Only add the capsfilter for avdec_h264.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8719

[PATCH v3 1/1] winegstreamer: Only add the capsfilter for avdec_h264.

[Brendan McGrath]

From: Brendan McGrath <bmcgrath(a)codeweavers.com>

h264parse currently has a bug where it will send an avc caps without
codec_data when it has received an SPS but not PPS.

This bug currently causes avdec_h264 to fail when a drain request
is made during caps neg.

As a workaround, a capsfilter was added to force a stream-format of
byte-stream for all h.264 decoders. However, this introduced a new
issue with the vtdec element, as it only supports the avc stream-format.

Therefore, this patch looks to address both problems by only introducing
the capsfilter when the avdec_h264 element is used.

It also removes the stream-format and alignment requirements from
the parsed caps, applying them only to the capsfilter.
---
 dlls/winegstreamer/wg_transform.c | 53 +++++++++++++++++++------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
index 9ca3368ecb9..56f23988b87 100644
--- a/dlls/winegstreamer/wg_transform.c
+++ b/dlls/winegstreamer/wg_transform.c
@@ -495,15 +495,7 @@ static GstCaps *transform_get_parsed_caps(GstCaps *caps, const char *media_type)
     else if (gst_structure_get_int(structure, "wmvversion", &value))
         gst_caps_set_simple(parsed_caps, "parsed", G_TYPE_BOOLEAN, true, "wmvversion", G_TYPE_INT, value, NULL);
     else
-    {
-        if (!strcmp(media_type, "video/x-h264"))
-        {
-            gst_caps_set_simple(parsed_caps, "stream-format", G_TYPE_STRING, "byte-stream", NULL);
-            gst_caps_set_simple(parsed_caps, "alignment", G_TYPE_STRING, "au", NULL);
-        }
-
         gst_caps_set_simple(parsed_caps, "parsed", G_TYPE_BOOLEAN, true, NULL);
-    }
 
     return parsed_caps;
 }
@@ -512,7 +504,9 @@ static bool transform_create_decoder_elements(struct wg_transform *transform,
         const gchar *input_mime, const gchar *output_mime, GstElement **first, GstElement **last)
 {
     GstCaps *parsed_caps = NULL, *sink_caps = NULL;
-    GstElement *element;
+    GstElement *element, *capsfilter;
+    const char *shortname = NULL;
+    GstElementFactory *factory;
     bool ret = false;
     char *str;
 
@@ -535,15 +529,6 @@ static bool transform_create_decoder_elements(struct wg_transform *transform,
 
     if (element)
     {
-        if (!(element = create_element("capsfilter", "good")) ||
-                !append_element(transform->container, element, first, last))
-            goto done;
-        if ((str = gst_caps_to_string(parsed_caps)))
-        {
-            gst_util_set_object_arg(G_OBJECT(element), "caps", str);
-            free(str);
-        }
-
         /* We try to intercept buffers produced by the parser, so if we push a large buffer into the
          * parser, it won't push everything into the decoder all in one go.
          */
@@ -558,8 +543,36 @@ static bool transform_create_decoder_elements(struct wg_transform *transform,
         parsed_caps = gst_caps_ref(transform->input_caps);
     }
 
-    if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, parsed_caps, sink_caps))
-            || !append_element(transform->container, element, first, last))
+    if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, parsed_caps, sink_caps)))
+        goto done;
+
+    /* We need to insert a capsfilter for avdec_h264 in order for it to use a byte-stream
+     * stream-format. Otherwise, when a drain request is made, the caps is fixated to AVC
+     * but no codec_data is provided to the decoder. This results in libav rejecting every
+     * packet it receives.
+     */
+    if ((factory = gst_element_get_factory(element)) &&
+            (shortname = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(factory))) &&
+            !strcmp(shortname, "avdec_h264"))
+    {
+        if (!(capsfilter = create_element("capsfilter", "good")) ||
+                !append_element(transform->container, capsfilter, first, last))
+        {
+            g_object_unref(element);
+            goto done;
+        }
+
+        gst_caps_set_simple(parsed_caps, "stream-format", G_TYPE_STRING, "byte-stream", NULL);
+        gst_caps_set_simple(parsed_caps, "alignment", G_TYPE_STRING, "au", NULL);
+
+        if ((str = gst_caps_to_string(parsed_caps)))
+        {
+            gst_util_set_object_arg(G_OBJECT(capsfilter), "caps", str);
+            free(str);
+        }
+    }
+
+    if (!append_element(transform->container, element, first, last))
         goto done;
 
     set_max_threads(element);
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8719

[Brendan McGrath (＠redmcg)]

> Erf. That bug certainly looks like it could affect more than just avdec :-/

You're right actually. Currently, with `nvh264dec` (which uses `h264decoder`), we get the following warning:
```
WARN             h264decoder gsth264decoder.c:1472:gst_h264_decoder_set_format:<nvh264dec1> avc format without codec data, assuming nal length size is 4
```

and the test passes because its assumption about the nal length is correct.

But with [ffed473](https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/ffed473992e9a66d...) in place, the test fails.

Do you think it's worth changing to opt-out rather than opt-in (i.e. explicitly exclude `vtdec` as opposed to explicitly include `avdec_h264`)?

Although, I'll point out that this bug seems to affect only this test case. We've had `h264parse` in Proton for almost 12 months and not needed this workaround.
> Anyway, the workaround seems reasonable to me, but we should alter the comment to make clear that it's a workaround for a GStreamer bug.

Done.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112644

[Elizabeth Figura (＠zfigura)]

> Do you think it's worth changing to opt-out rather than opt-in (i.e. explicitly exclude `vtdec` as opposed to explicitly include `avdec_h264`)?
>
> Although, I'll point out that this bug seems to affect only this test case. We've had `h264parse` in Proton for almost 12 months and not needed this workaround.

Yeah, that makes me more nervous actually. If just targeting avdec_h264 seems to be enough in practice then let's just limit it to that for now.
> > Anyway, the workaround seems reasonable to me, but we should alter the comment to make clear that it's a workaround for a GStreamer bug.
>
> Done.

Thanks, could you please also edit the code comment?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112646