[PATCH v2 0/1] MR8719: winegstreamer: Only add the capsfilter for avdec_h264.
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
From: Brendan McGrath <bmcgrath(a)codeweavers.com> 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8719
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112279
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112280
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... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112281
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112294
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: 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, 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...)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112418
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8719#note_112636
participants (3)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Elizabeth Figura (＠zfigura)