[PATCH v5 0/4] MR2387: Draft: winegstreamer: Use codec format in stream_props_GetMediaType.
According to the tests in 23b72ad, when we are reading a compressed stream, the type returned by `stream_props_GetMediaType()` should reflect compressed format even if we finnally output uncomressed data. For example, if we use wmvcore reader to read a WMV3 stream and output RGB24, the format information returned by `stream_props_GetMediaType()` should be WMV3, not RGB24. This patch set is marked as draft now, because PATCH 2 and PATCH 3 is affected by 3e8936a2 in MR !2258. So I'll submit a newer version of this after !2258 get merged. -- v5: winegstreamer: Use codec format in stream_props_GetMediaType. winegstreamer: Implement amt_from_wg_format_video_wmv. winegstreamer: Implement wg_format_from_caps_video_wmv. winegstreamer: Implement wg_parser_stream_get_codec_format. https://gitlab.winehq.org/wine/wine/-/merge_requests/2387
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 1 + dlls/winegstreamer/main.c | 13 +++++++++++ dlls/winegstreamer/unixlib.h | 7 ++++++ dlls/winegstreamer/wg_parser.c | 37 +++++++++++++++++++++++++++++--- 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index c59d3f1bb66..6b7d7a82848 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -82,6 +82,7 @@ uint32_t wg_parser_get_stream_count(struct wg_parser *parser); struct wg_parser_stream *wg_parser_get_stream(struct wg_parser *parser, uint32_t index); void wg_parser_stream_get_preferred_format(struct wg_parser_stream *stream, struct wg_format *format); +void wg_parser_stream_get_codec_format(struct wg_parser_stream *stream, struct wg_format *format); void wg_parser_stream_enable(struct wg_parser_stream *stream, const struct wg_format *format); void wg_parser_stream_disable(struct wg_parser_stream *stream); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 1afa51ac0aa..ba8b01497a3 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -185,6 +185,19 @@ void wg_parser_stream_get_preferred_format(struct wg_parser_stream *stream, stru WINE_UNIX_CALL(unix_wg_parser_stream_get_preferred_format, ¶ms); } +void wg_parser_stream_get_codec_format(struct wg_parser_stream *stream, struct wg_format *format) +{ + struct wg_parser_stream_get_codec_format_params params = + { + .stream = stream, + .format = format, + }; + + TRACE("stream %p, format %p.\n", stream, format); + + WINE_UNIX_CALL(unix_wg_parser_stream_get_codec_format, ¶ms); +} + void wg_parser_stream_enable(struct wg_parser_stream *stream, const struct wg_format *format) { struct wg_parser_stream_enable_params params = diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 539ccd3e12a..9b0ba090f22 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -238,6 +238,12 @@ struct wg_parser_stream_get_preferred_format_params struct wg_format *format; }; +struct wg_parser_stream_get_codec_format_params +{ + struct wg_parser_stream *stream; + struct wg_format *format; +}; + struct wg_parser_stream_enable_params { struct wg_parser_stream *stream; @@ -346,6 +352,7 @@ enum unix_funcs unix_wg_parser_get_stream, unix_wg_parser_stream_get_preferred_format, + unix_wg_parser_stream_get_codec_format, unix_wg_parser_stream_enable, unix_wg_parser_stream_disable, diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 5bb824f4399..15b0e9ceddd 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -106,7 +106,7 @@ struct wg_parser_stream GstPad *their_src, *post_sink, *post_src, *my_sink; GstElement *flip; GstSegment segment; - struct wg_format preferred_format, current_format; + struct wg_format preferred_format, current_format, codec_format; pthread_cond_t event_cond, event_empty_cond; GstBuffer *buffer; @@ -208,6 +208,14 @@ static NTSTATUS wg_parser_stream_get_preferred_format(void *args) return S_OK; } +static NTSTATUS wg_parser_stream_get_codec_format(void *args) +{ + struct wg_parser_stream_get_codec_format_params *params = args; + + *params->format = params->stream->codec_format; + return S_OK; +} + static NTSTATUS wg_parser_stream_enable(void *args) { const struct wg_parser_stream_enable_params *params = args; @@ -783,6 +791,7 @@ static struct wg_parser_stream *create_stream(struct wg_parser *parser) stream->parser = parser; stream->number = parser->stream_count; stream->current_format.major_type = WG_MAJOR_TYPE_UNKNOWN; + stream->codec_format.major_type = WG_MAJOR_TYPE_UNKNOWN; pthread_cond_init(&stream->event_cond, NULL); pthread_cond_init(&stream->event_empty_cond, NULL); @@ -826,12 +835,15 @@ static void free_stream(struct wg_parser_stream *stream) static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user) { + GstPad *decoder_src_pad, *decoder_sink_pad; + GstCaps *caps, *decoder_sink_caps; struct wg_parser *parser = user; struct wg_parser_stream *stream; - const char *name; - GstCaps *caps; + const char *name, *klass; + GstElement *decoder; int ret; + GST_LOG("parser %p, element %p, pad %p.", parser, element, pad); if (gst_pad_is_linked(pad)) @@ -843,6 +855,24 @@ static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user) if (!(stream = create_stream(parser))) goto out; + /* Get decoder format for decodebin. */ + if (!strcmp(gst_element_get_metadata(element, GST_ELEMENT_METADATA_LONGNAME), "Decoder Bin")) + { + decoder_src_pad = gst_ghost_pad_get_target(GST_GHOST_PAD(pad)); + decoder = gst_pad_get_parent_element(decoder_src_pad); + klass = gst_element_get_metadata(decoder, GST_ELEMENT_METADATA_KLASS); + if (strstr(klass, GST_ELEMENT_FACTORY_KLASS_DECODER)) + { + decoder_sink_pad = gst_element_get_static_pad(decoder, "sink"); + decoder_sink_caps = gst_pad_get_current_caps(decoder_sink_pad); + wg_format_from_caps(&stream->codec_format, decoder_sink_caps); + gst_caps_unref(decoder_sink_caps); + gst_object_unref(decoder_sink_pad); + } + gst_object_unref(decoder); + gst_object_unref(decoder_src_pad); + } + if (!strcmp(name, "video/x-raw")) { GstElement *deinterlace, *vconv, *flip, *vconv2; @@ -1797,6 +1827,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_parser_get_stream), X(wg_parser_stream_get_preferred_format), + X(wg_parser_stream_get_codec_format), X(wg_parser_stream_enable), X(wg_parser_stream_disable), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2387
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/wg_format.c | 57 ++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/dlls/winegstreamer/wg_format.c b/dlls/winegstreamer/wg_format.c index ac21b0af94f..1850972cdbc 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -232,6 +232,59 @@ static void wg_format_from_caps_video_cinepak(struct wg_format *format, const Gs format->u.video_cinepak.fps_d = fps_d; } +static void wg_format_from_caps_video_wmv(struct wg_format *format, const GstCaps *caps) +{ + const GstStructure *structure = gst_caps_get_structure(caps, 0); + gint width, height, fps_n, fps_d, wmv_version = 0; + gchar format_buffer[4] = {'W','M','V','0'}; + enum wg_wmv_video_format wmv_format; + const gchar *wmv_format_str = NULL; + + if (!gst_structure_get_int(structure, "width", &width)) + { + GST_WARNING("Missing \"width\" value."); + return; + } + if (!gst_structure_get_int(structure, "height", &height)) + { + GST_WARNING("Missing \"height\" value."); + return; + } + + if (!(wmv_format_str = gst_structure_get_string(structure, "format"))) + { + if (!gst_structure_get_int(structure, "wmvversion", &wmv_version)) + GST_WARNING("Unable to get WMV format."); + format_buffer[3] += wmv_version; + wmv_format_str = format_buffer; + } + if (!strcmp(wmv_format_str, "WMV1")) + wmv_format = WG_WMV_VIDEO_FORMAT_WMV1; + else if (!strcmp(wmv_format_str, "WMV2")) + wmv_format = WG_WMV_VIDEO_FORMAT_WMV2; + else if (!strcmp(wmv_format_str, "WMV3")) + wmv_format = WG_WMV_VIDEO_FORMAT_WMV3; + else if (!strcmp(wmv_format_str, "WMVA")) + wmv_format = WG_WMV_VIDEO_FORMAT_WMVA; + else if (!strcmp(wmv_format_str, "WVC1")) + wmv_format = WG_WMV_VIDEO_FORMAT_WVC1; + else + wmv_format = WG_WMV_VIDEO_FORMAT_UNKNOWN; + + if (!gst_structure_get_fraction(structure, "framerate", &fps_n, &fps_d)) + { + fps_n = 0; + fps_d = 1; + } + + format->major_type = WG_MAJOR_TYPE_VIDEO_WMV; + format->u.video_wmv.width = width; + format->u.video_wmv.height = height; + format->u.video_wmv.format = wmv_format; + format->u.video_wmv.fps_n = fps_n; + format->u.video_wmv.fps_d = fps_d; +} + void wg_format_from_caps(struct wg_format *format, const GstCaps *caps) { const GstStructure *structure = gst_caps_get_structure(caps, 0); @@ -261,6 +314,10 @@ void wg_format_from_caps(struct wg_format *format, const GstCaps *caps) { wg_format_from_caps_video_cinepak(format, caps); } + else if (!strcmp(name, "video/x-wmv")) + { + wg_format_from_caps_video_wmv(format, caps); + } else { gchar *str = gst_caps_to_string(caps); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2387
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/quartz_parser.c | 57 +++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 085a43b4297..f09579e07a9 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -538,6 +538,59 @@ static bool amt_from_wg_format_video_cinepak(AM_MEDIA_TYPE *mt, const struct wg_ return true; } +static bool amt_from_wg_format_video_wmv(AM_MEDIA_TYPE *mt, const struct wg_format *format) +{ + VIDEOINFO *video_format; + uint32_t frame_time; + const GUID *subtype; + + if (!(video_format = CoTaskMemAlloc(sizeof(*video_format)))) + return false; + + switch (format->u.video_wmv.format) + { + case WG_WMV_VIDEO_FORMAT_WMV1: + subtype = &MEDIASUBTYPE_WMV1; + break; + case WG_WMV_VIDEO_FORMAT_WMV2: + subtype = &MEDIASUBTYPE_WMV2; + break; + case WG_WMV_VIDEO_FORMAT_WMV3: + subtype = &MEDIASUBTYPE_WMV3; + break; + case WG_WMV_VIDEO_FORMAT_WMVA: + subtype = &MEDIASUBTYPE_WMVA; + break; + case WG_WMV_VIDEO_FORMAT_WVC1: + subtype = &MEDIASUBTYPE_WVC1; + break; + default: + WARN("Invalid WMV format %u.\n", format->u.video_wmv.format); + return false; + } + + mt->majortype = MEDIATYPE_Video; + mt->subtype = *subtype; + mt->bFixedSizeSamples = FALSE; + mt->bTemporalCompression = TRUE; + mt->lSampleSize = 0; + mt->formattype = FORMAT_VideoInfo; + mt->cbFormat = sizeof(VIDEOINFOHEADER); + mt->pbFormat = (BYTE *)video_format; + + memset(video_format, 0, sizeof(*video_format)); + SetRect(&video_format->rcSource, 0, 0, format->u.video_wmv.width, format->u.video_wmv.height); + video_format->rcTarget = video_format->rcSource; + if ((frame_time = MulDiv(10000000, format->u.video_wmv.fps_d, format->u.video_wmv.fps_n)) != -1) + video_format->AvgTimePerFrame = frame_time; + video_format->bmiHeader.biSize = sizeof(BITMAPINFOHEADER); + video_format->bmiHeader.biWidth = format->u.video_wmv.width; + video_format->bmiHeader.biHeight = format->u.video_wmv.height; + video_format->bmiHeader.biPlanes = 1; + video_format->bmiHeader.biCompression = mt->subtype.Data1; + + return true; +} bool amt_from_wg_format(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool wm) { @@ -548,7 +601,6 @@ bool amt_from_wg_format(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool case WG_MAJOR_TYPE_AUDIO_MPEG4: case WG_MAJOR_TYPE_AUDIO_WMA: case WG_MAJOR_TYPE_VIDEO_H264: - case WG_MAJOR_TYPE_VIDEO_WMV: case WG_MAJOR_TYPE_VIDEO_INDEO: FIXME("Format %u not implemented!\n", format->major_type); /* fallthrough */ @@ -566,6 +618,9 @@ bool amt_from_wg_format(AM_MEDIA_TYPE *mt, const struct wg_format *format, bool case WG_MAJOR_TYPE_VIDEO_CINEPAK: return amt_from_wg_format_video_cinepak(mt, format); + + case WG_MAJOR_TYPE_VIDEO_WMV: + return amt_from_wg_format_video_wmv(mt, format); } assert(0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2387
From: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/winegstreamer/wm_reader.c | 10 +++++++++- dlls/wmvcore/tests/wmvcore.c | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 736dbba452c..64bd4ce800b 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -544,12 +544,20 @@ static HRESULT WINAPI stream_props_GetType(IWMMediaProps *iface, GUID *major_typ static HRESULT WINAPI stream_props_GetMediaType(IWMMediaProps *iface, WM_MEDIA_TYPE *mt, DWORD *size) { struct stream_config *config = impl_from_IWMMediaProps(iface); + const struct wg_format *format; + struct wg_format codec_format; const DWORD req_size = *size; AM_MEDIA_TYPE stream_mt; TRACE("iface %p, mt %p, size %p.\n", iface, mt, size); - if (!amt_from_wg_format(&stream_mt, &config->stream->format, true)) + wg_parser_stream_get_codec_format(config->stream->wg_stream, &codec_format); + if (codec_format.major_type != WG_MAJOR_TYPE_UNKNOWN) + format = &codec_format; + else + format = &config->stream->format; + + if (!amt_from_wg_format(&stream_mt, format, true)) return E_OUTOFMEMORY; *size = sizeof(stream_mt) + stream_mt.cbFormat; diff --git a/dlls/wmvcore/tests/wmvcore.c b/dlls/wmvcore/tests/wmvcore.c index f07abb8d4bb..0cd37990b49 100644 --- a/dlls/wmvcore/tests/wmvcore.c +++ b/dlls/wmvcore/tests/wmvcore.c @@ -1549,7 +1549,7 @@ static void test_sync_reader_types(void) if (IsEqualGUID(&majortype, &MEDIATYPE_Audio)) test_stream_media_props(config, &MEDIATYPE_Audio, &MEDIASUBTYPE_MSAUDIO1, &FORMAT_WaveFormatEx, TRUE); else - test_stream_media_props(config, &MEDIATYPE_Video, &MEDIASUBTYPE_WMV1, &FORMAT_VideoInfo, TRUE); + test_stream_media_props(config, &MEDIATYPE_Video, &MEDIASUBTYPE_WMV1, &FORMAT_VideoInfo, FALSE); ref = IWMStreamConfig_Release(config); ok(!ref, "Got outstanding refcount %ld.\n", ref); @@ -3429,7 +3429,7 @@ static void test_async_reader_types(void) if (IsEqualGUID(&majortype, &MEDIATYPE_Audio)) test_stream_media_props(config, &MEDIATYPE_Audio, &MEDIASUBTYPE_MSAUDIO1, &FORMAT_WaveFormatEx, TRUE); else - test_stream_media_props(config, &MEDIATYPE_Video, &MEDIASUBTYPE_WMV1, &FORMAT_VideoInfo, TRUE); + test_stream_media_props(config, &MEDIATYPE_Video, &MEDIASUBTYPE_WMV1, &FORMAT_VideoInfo, FALSE); ref = IWMStreamConfig_Release(config); ok(!ref, "Got outstanding refcount %ld.\n", ref); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2387
@zfigura `amstream:amstream` and `quartz:avisplit` failures are fixed in V5. This patch is ready to be reviewed now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28170
The winegstreamer API seems fine, but I don't think that approach of retrieving the stream format is quite what we want. I believe the right approach to actually outputting uncompressed data is to use autoplug-continue, so we should use the same signal to find out what the uncompressed format is; that way we know the logic matches. By contrast, this patch could retrieve a compressed format that isn't the "initial" compressed format, or (theoretically?) fail to retrieve a compressed format altogether. The trick there is matching the pad from autoplug-continue with the pad from pad-added. I've given this some thought and I have two potential ideas here: (1a) Keep track of pads returned from autoplug-continue in a list, adding a reference to each one. When we get pad-added, walk the list of pads upwards until we find a pad in the autoplug list. (1b) Keep track of pads returned from autoplug-continue in a list, adding a reference to each one. Install a pad probe on each one as well. When we get pad-added, send a custom query to the downstream pad, and catch it with the pad probe. (2) Always return FALSE from autoplug-continue for recognized compressed formats. If the format is compressed and we are configured to return uncompressed data, append a second decodebin. Unsurprisingly, GStreamer is designed so that this is actually a pretty reasonable thing to do. There's a (negligible) bit of overhead from the extra bin, but the configuration of elements shouldn't actually change at all. Worth noting is that decodebin will *not* add a multiThat aside, a few extra comments on this queue if there's no parser and use-buffering isn't configured to TRUE; both of these will be the case here and that's what we want (we already have the multiqueue earlier). This also might allow us to toggle compression without recreating the entire wg_parser (by detaching the stream's decodebin), though I haven't fully thought this through and it's not like it matters that much anyway. The more I think about it, the more I like (2). (1a) seems conceptually ugly and is probably a pain to implement; (1b) is less bad but honestly (2) just seems simpler, both practically and architecturally. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28268
On Wed Mar 29 02:12:01 2023 +0000, Zebediah Figura wrote:
The winegstreamer API seems fine, but I don't think that approach of retrieving the stream format is quite what we want. I believe the right approach to actually outputting uncompressed data is to use autoplug-continue, so we should use the same signal to find out what the uncompressed format is; that way we know the logic matches. By contrast, this patch could retrieve a compressed format that isn't the "initial" compressed format, or (theoretically?) fail to retrieve a compressed format altogether. The trick there is matching the pad from autoplug-continue with the pad from pad-added. I've given this some thought and I have two potential ideas here: (1a) Keep track of pads returned from autoplug-continue in a list, adding a reference to each one. When we get pad-added, walk the list of pads upwards until we find a pad in the autoplug list. (1b) Keep track of pads returned from autoplug-continue in a list, adding a reference to each one. Install a pad probe on each one as well. When we get pad-added, send a custom query to the downstream pad, and catch it with the pad probe. (2) Always return FALSE from autoplug-continue for recognized compressed formats. If the format is compressed and we are configured to return uncompressed data, append a second decodebin. Unsurprisingly, GStreamer is designed so that this is actually a pretty reasonable thing to do. There's a (negligible) bit of overhead from the extra bin, but the configuration of elements shouldn't actually change at all. Worth noting is that decodebin will *not* add a multiThat aside, a few extra comments on this queue if there's no parser and use-buffering isn't configured to TRUE; both of these will be the case here and that's what we want (we already have the multiqueue earlier). This also might allow us to toggle compression without recreating the entire wg_parser (by detaching the stream's decodebin), though I haven't fully thought this through and it's not like it matters that much anyway. The more I think about it, the more I like (2). (1a) seems conceptually ugly and is probably a pain to implement; (1b) is less bad but honestly (2) just seems simpler, both practically and architecturally. OK, I'll try to implement approach (2).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28294
On Wed Mar 29 02:12:01 2023 +0000, Ziqing Hui wrote:
OK, I'll try to implement approach (2). We should create a extra bin for each stream, is that correct? For example, for a ASF file contains both video and audio streams, the first decodebin demux it into video and audio. Then we should create two extra decodebins, one for the video stream, and another for the audio. Is that true?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28295
We should create a extra bin for each stream, is that correct? For example, for a ASF file contains both video and audio streams, the first decodebin demux it into video and audio. Then we should create two extra decodebins, one for the video stream, and another for the audio. Is that true?
That's the idea, yes. Note that we'd only create an extra bin for a stream which we recognize the compressed type of, though. If we don't recognize the type we should return true from autoplug-continue, and not hook up an extra decodebin. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28296
On Wed Mar 29 04:04:36 2023 +0000, Zebediah Figura wrote:
We should create a extra bin for each stream, is that correct? For example, for a ASF file contains both video and audio streams, the first decodebin demux it into video and audio. Then we should create two extra decodebins, one for the video stream, and another for the audio. Is that true? That's the idea, yes. Note that we'd only create an extra bin for a stream which we recognize the compressed type of, though. If we don't recognize the type we should return true from autoplug-continue, and not hook up an extra decodebin. I've submitted MR !2546 that implements it. This MR will be updated after !2546 is merged.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2387#note_28496
participants (3)
-
Zebediah Figura (＠zfigura) -
Ziqing Hui
-
Ziqing Hui (＠zhui)