[PATCH v2 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.

--
  v2: winegstreamer: Only add the capsfilter for avdec_h264.

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

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

[Brendan McGrath]

From: Brendan McGrath

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.
---
 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);

[Elizabeth Figura (＠zfigura)]

The original commit looks rather suspicious to me, and this looks more so. Are we working around a GStreamer bug here? Are we sure it's a bug? Have we reported it upstream?

[Brendan McGrath (＠redmcg)]

On Thu Aug  7 02:09:26 2025 +0000, Elizabeth Figura wrote:
> The original commit looks rather suspicious to me, and this looks more
> so. Are we working around a GStreamer bug here? Are we sure it's a bug?
> Have we reported it upstream?

I haven't finished my investigation yet; so I'm currently not 100% sure where the bug is. But this is certainly a work-around for what I suspect is a GStreamer bug.

[Elizabeth Figura (＠zfigura)]

On Thu Aug  7 02:09:26 2025 +0000, Brendan McGrath wrote:
> I haven't finished my investigation yet; so I'm currently not 100% sure
> where the bug is. But this is certainly a work-around for what I suspect
> is a GStreamer bug.

I see. While the workaround may make sense, I don't think we should commit workarounds until we fully understand the bug.

Of course, in a sense we're only limiting an existing workaround, so it's probably still a good idea...

[Brendan McGrath (＠redmcg)]

On Thu Aug  7 03:03:32 2025 +0000, Elizabeth Figura wrote:
> I see. While the workaround may make sense, I don't think we should
> commit workarounds until we fully understand the bug.
> Of course, in a sense we're only limiting an existing workaround, so
> it's probably still a good idea...

I think I've got to the bottom of this one now. It appears to be the result of a bug in `h264parse`.

Our test only provides twos buffers before calling drain. They are:
1. an AU; and
2. an SPS

The drain creates a `SEGMENT_DONE` event which causes `h264parse` to call `gst_h264_parse_update_src_caps`.

In there it calls `gst_h264_parse_make_codec_data` to create the `codec_data` (which is required for AVC) but it fails due to the missing PPS (which has not yet been sent):
```
h264parse gsth264parse.c:1666:gst_h264_parse_make_codec_data:<h264parse1> constructing codec_data: num_sps=1, num_pps=0
```

But it never handles this failure correctly, as it then only checks for the absence of SPS (gsth264parse.c:2154 @ tag 1.24.2):
```
  caps = NULL;
  if (G_UNLIKELY (!sps)) {
    caps = gst_caps_copy (sink_caps);
  } else {
```

Simply modifying that if statement to also check for the absence of pps fixes the issue.

I'll report this upstream, but I suspect our test case has hit rather an edge case here.

[Brendan McGrath (＠redmcg)]

On Thu Aug  7 05:03:43 2025 +0000, Brendan McGrath wrote:
> I think I've got to the bottom of this one now. It appears to be the
> result of a bug in `h264parse`.
> Our test only provides two buffers before calling drain. They are:
> - an AU; and
> - an SPS
>
> The drain creates a `SEGMENT_DONE` event which causes `h264parse` to
> call `gst_h264_parse_update_src_caps`.
> In there it calls `gst_h264_parse_make_codec_data` to create the
> `codec_data` (which is required for AVC) but it fails due to the missing
> PPS (which has not yet been sent):
> h264parse
> gsth264parse.c:1666:gst_h264_parse_make_codec_data:<h264parse1>
> constructing codec_data: num_sps=1, num_pps=0
>
> But it never handles this failure correctly, as it then only checks for
> the absence of SPS (gsth264parse.c:2154 @ tag 1.24.2, or [main branch](https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/g...)):
>   caps = NULL;
>   if (G_UNLIKELY (!sps)) {
>     caps = gst_caps_copy (sink_caps);
>   } else {
>
> Simply modifying that if statement to also check for the absence of pps
> fixes the issue.
> I'll report this upstream, but I suspect our test case has hit rather an
> edge case here.

Oops. I was about to report this upstream and found it has already been fixed with:
[ffed473](https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/ffed473992e9a66d...)

[Elizabeth Figura (＠zfigura)]

On Fri Aug  8 04:06:56 2025 +0000, Brendan McGrath wrote:
> Oops. I was about to report this upstream and found it has already been
> fixed with:
> [ffed473](https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/ffed473992e9a66d...)

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

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.