-- v3: winegstreamer: Output BGRx video from the media source. winegstreamer: Move flipping based on RGB to the frontends. winegstreamer: Translate the MF_MT_DEFAULT_STRIDE attribute to flipped video in mf_media_type_to_wg_format(). winegstreamer: Set the MF_MT_DEFAULT_STRIDE attribute in mf_media_type_from_wg_format(). winegstreamer: Initialize media source video types from a wg_video_format array.
From: Zebediah Figura zfigura@codeweavers.com
The mf_media_type_from_wg_format function will use the video format to calculate stride. --- dlls/winegstreamer/media_source.c | 43 +++++++++++++++++++------------ 1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 542189b28f5..dda7b6df18a 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -866,15 +866,15 @@ static HRESULT media_stream_init_desc(struct media_stream *stream)
if (format.major_type == WG_MAJOR_TYPE_VIDEO) { - /* These are the most common native output types of decoders: - https://docs.microsoft.com/en-us/windows/win32/medfound/mft-decoder-expose-o... */ - static const GUID *const video_types[] = + /* Try to prefer YUV formats over RGB ones. Most decoders output in the + * YUV color space, and it's generally much less expensive for + * videoconvert to do YUV -> YUV transformations. */ + static const enum wg_video_format video_formats[] = { - &MFVideoFormat_NV12, - &MFVideoFormat_YV12, - &MFVideoFormat_YUY2, - &MFVideoFormat_IYUV, - &MFVideoFormat_I420, + WG_VIDEO_FORMAT_NV12, + WG_VIDEO_FORMAT_YV12, + WG_VIDEO_FORMAT_YUY2, + WG_VIDEO_FORMAT_I420, };
IMFMediaType *base_type = mf_media_type_from_wg_format(&format); @@ -891,21 +891,32 @@ static HRESULT media_stream_init_desc(struct media_stream *stream) stream_types[0] = base_type; type_count = 1;
- for (i = 0; i < ARRAY_SIZE(video_types); i++) + for (i = 0; i < ARRAY_SIZE(video_formats); ++i) { + struct wg_format new_format = format; IMFMediaType *new_type;
- if (IsEqualGUID(&base_subtype, video_types[i])) - continue; + new_format.u.video.format = video_formats[i];
- if (FAILED(hr = MFCreateMediaType(&new_type))) + if (!(new_type = mf_media_type_from_wg_format(&new_format))) + { + hr = E_OUTOFMEMORY; goto done; + } stream_types[type_count++] = new_type;
- if (FAILED(hr = IMFMediaType_CopyAllItems(base_type, (IMFAttributes *) new_type))) - goto done; - if (FAILED(hr = IMFMediaType_SetGUID(new_type, &MF_MT_SUBTYPE, video_types[i]))) - goto done; + if (video_formats[i] == WG_VIDEO_FORMAT_I420) + { + IMFMediaType *iyuv_type; + + if (FAILED(hr = MFCreateMediaType(&iyuv_type))) + goto done; + if (FAILED(hr = IMFMediaType_CopyAllItems(iyuv_type, (IMFAttributes *)iyuv_type))) + goto done; + if (FAILED(hr = IMFMediaType_SetGUID(iyuv_type, &MF_MT_SUBTYPE, &MFVideoFormat_IYUV))) + goto done; + stream_types[type_count++] = iyuv_type; + } } } else if (format.major_type == WG_MAJOR_TYPE_AUDIO)
From: Zebediah Figura zfigura@codeweavers.com
--- dlls/mfreadwrite/tests/mfplat.c | 6 +-- dlls/winegstreamer/gst_private.h | 4 ++ dlls/winegstreamer/main.c | 64 ++++++++++++++++++++++++++++++++ dlls/winegstreamer/mfplat.c | 7 ++++ 4 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index 83091b59380..a6938b8004f 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -821,7 +821,7 @@ static void test_source_reader(const char *filename, bool video) (unsigned int)(framesize >> 32), (unsigned int)framesize);
hr = IMFMediaType_GetUINT32(mediatype, &MF_MT_DEFAULT_STRIDE, &stride); - ok(hr == MF_E_ATTRIBUTENOTFOUND, "Unexpected hr %#lx.\n", hr); + todo_wine ok(hr == MF_E_ATTRIBUTENOTFOUND, "Unexpected hr %#lx.\n", hr);
IMFMediaType_Release(mediatype);
@@ -845,8 +845,8 @@ static void test_source_reader(const char *filename, bool video) ok(IsEqualGUID(&subtype, &MFVideoFormat_NV12), "Got subtype %s.\n", debugstr_guid(&subtype));
hr = IMFMediaType_GetUINT32(mediatype, &MF_MT_DEFAULT_STRIDE, &stride); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - todo_wine ok(stride == 160, "Got stride %u.\n", stride); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(stride == 160, "Got stride %u.\n", stride);
IMFMediaType_Release(mediatype);
diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 5b4c01a3cd0..4323cacde4d 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -144,6 +144,10 @@ HRESULT wg_transform_read_quartz(struct wg_transform *transform, struct wg_sampl
HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj);
+unsigned int wg_format_get_stride(const struct wg_format *format); + +bool wg_video_format_is_rgb(enum wg_video_format format); + HRESULT aac_decoder_create(REFIID riid, void **ret); HRESULT h264_decoder_create(REFIID riid, void **ret); HRESULT video_processor_create(REFIID riid, void **ret); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 1afa51ac0aa..ce59baaab3b 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -413,6 +413,70 @@ bool wg_transform_set_output_format(struct wg_transform *transform, struct wg_fo return !WINE_UNIX_CALL(unix_wg_transform_set_output_format, ¶ms); }
+#define ALIGN(n, alignment) (((n) + (alignment) - 1) & ~((alignment) - 1)) + +unsigned int wg_format_get_stride(const struct wg_format *format) +{ + const unsigned int width = format->u.video.width; + + switch (format->u.video.format) + { + case WG_VIDEO_FORMAT_AYUV: + return width * 4; + + case WG_VIDEO_FORMAT_BGRA: + case WG_VIDEO_FORMAT_BGRx: + return width * 4; + + case WG_VIDEO_FORMAT_BGR: + return ALIGN(width * 3, 4); + + case WG_VIDEO_FORMAT_UYVY: + case WG_VIDEO_FORMAT_YUY2: + case WG_VIDEO_FORMAT_YVYU: + return ALIGN(width * 2, 4); + + case WG_VIDEO_FORMAT_RGB15: + case WG_VIDEO_FORMAT_RGB16: + return ALIGN(width * 2, 4); + + case WG_VIDEO_FORMAT_I420: + case WG_VIDEO_FORMAT_NV12: + case WG_VIDEO_FORMAT_YV12: + return ALIGN(width, 4); /* Y plane */ + + case WG_VIDEO_FORMAT_UNKNOWN: + FIXME("Cannot calculate stride for unknown video format.\n"); + } + + return 0; +} + +bool wg_video_format_is_rgb(enum wg_video_format format) +{ + switch (format) + { + case WG_VIDEO_FORMAT_BGRA: + case WG_VIDEO_FORMAT_BGRx: + case WG_VIDEO_FORMAT_BGR: + case WG_VIDEO_FORMAT_RGB15: + case WG_VIDEO_FORMAT_RGB16: + return true; + + case WG_VIDEO_FORMAT_AYUV: + case WG_VIDEO_FORMAT_I420: + case WG_VIDEO_FORMAT_NV12: + case WG_VIDEO_FORMAT_UYVY: + case WG_VIDEO_FORMAT_YUY2: + case WG_VIDEO_FORMAT_YV12: + case WG_VIDEO_FORMAT_YVYU: + case WG_VIDEO_FORMAT_UNKNOWN: + break; + } + + return false; +} + BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, void *reserved) { if (reason == DLL_PROCESS_ATTACH) diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 4cd095fb82e..257ffb381a8 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -511,6 +511,7 @@ static IMFMediaType *mf_media_type_from_wg_format_video(const struct wg_format * { if (format->u.video.format == video_formats[i].format) { + unsigned int stride = wg_format_get_stride(format); int32_t height = abs(format->u.video.height); int32_t width = format->u.video.width;
@@ -526,6 +527,12 @@ static IMFMediaType *mf_media_type_from_wg_format_video(const struct wg_format * IMFMediaType_SetUINT32(type, &MF_MT_ALL_SAMPLES_INDEPENDENT, TRUE); IMFMediaType_SetUINT32(type, &MF_MT_VIDEO_ROTATION, MFVideoRotationFormat_0);
+ if (wg_video_format_is_rgb(format->u.video.format)) + stride = -stride; + if (format->u.video.height < 0) + stride = -stride; + IMFMediaType_SetUINT32(type, &MF_MT_DEFAULT_STRIDE, stride); + if (!IsRectEmpty(&format->u.video.padding)) { MFVideoArea aperture =
From: Zebediah Figura zfigura@codeweavers.com
--- dlls/winegstreamer/mfplat.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 257ffb381a8..af164614b43 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -699,7 +699,7 @@ static void mf_media_type_to_wg_format_video(IMFMediaType *type, const GUID *sub { UINT64 frame_rate, frame_size; MFVideoArea aperture; - UINT32 size; + UINT32 size, stride;
if (FAILED(IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size))) { @@ -729,6 +729,14 @@ static void mf_media_type_to_wg_format_video(IMFMediaType *type, const GUID *sub }
format->u.video.format = mf_video_format_to_wg(subtype); + + if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_DEFAULT_STRIDE, &stride))) + { + if (wg_video_format_is_rgb(format->u.video.format)) + format->u.video.height = -format->u.video.height; + if ((int)stride < 0) + format->u.video.height = -format->u.video.height; + } }
static void mf_media_type_to_wg_format_audio_wma(IMFMediaType *type, const GUID *subtype, struct wg_format *format)
From: Zebediah Figura zfigura@codeweavers.com
Give the backend a more simple and self-consistent API.
This commit also changes behaviour, by virtue of *not* changing some frontends. In specific, this commit does not modify the AVI splitter, which is known to output top-down RGB samples on Windows if the original video uses them (this is trivial to test by modifying test.avi in quartz to use "bgra" instead of "yuv420p"). It also does not modify the Media Foundation color converter DMO, whose tests imply that the MF_MT_DEFAULT_STRIDE attribute is always positive. --- dlls/winegstreamer/mfplat.c | 8 ++++---- dlls/winegstreamer/quartz_parser.c | 9 ++++++++- dlls/winegstreamer/unixlib.h | 2 ++ dlls/winegstreamer/wg_parser.c | 21 --------------------- dlls/winegstreamer/wm_reader.c | 9 ++++++++- dlls/winegstreamer/wmv_decoder.c | 4 ++-- 6 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index af164614b43..9cf98ec9dc4 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -527,8 +527,6 @@ static IMFMediaType *mf_media_type_from_wg_format_video(const struct wg_format * IMFMediaType_SetUINT32(type, &MF_MT_ALL_SAMPLES_INDEPENDENT, TRUE); IMFMediaType_SetUINT32(type, &MF_MT_VIDEO_ROTATION, MFVideoRotationFormat_0);
- if (wg_video_format_is_rgb(format->u.video.format)) - stride = -stride; if (format->u.video.height < 0) stride = -stride; IMFMediaType_SetUINT32(type, &MF_MT_DEFAULT_STRIDE, stride); @@ -732,11 +730,13 @@ static void mf_media_type_to_wg_format_video(IMFMediaType *type, const GUID *sub
if (SUCCEEDED(IMFMediaType_GetUINT32(type, &MF_MT_DEFAULT_STRIDE, &stride))) { - if (wg_video_format_is_rgb(format->u.video.format)) - format->u.video.height = -format->u.video.height; if ((int)stride < 0) format->u.video.height = -format->u.video.height; } + else if (wg_video_format_is_rgb(format->u.video.format)) + { + format->u.video.height = -format->u.video.height; + } }
static void mf_media_type_to_wg_format_audio_wma(IMFMediaType *type, const GUID *subtype, struct wg_format *format) diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index c12e9ee3397..987c64d7d40 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -485,7 +485,7 @@ static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format *
if (wm) { - SetRect(&video_format->rcSource, 0, 0, format->u.video.width, format->u.video.height); + SetRect(&video_format->rcSource, 0, 0, format->u.video.width, abs(format->u.video.height)); video_format->rcTarget = video_format->rcSource; } if ((frame_time = MulDiv(10000000, format->u.video.fps_d, format->u.video.fps_n)) != -1) @@ -493,6 +493,8 @@ static bool amt_from_wg_format_video(AM_MEDIA_TYPE *mt, const struct wg_format * video_format->bmiHeader.biSize = sizeof(BITMAPINFOHEADER); video_format->bmiHeader.biWidth = format->u.video.width; video_format->bmiHeader.biHeight = format->u.video.height; + if (wg_video_format_is_rgb(format->u.video.format)) + video_format->bmiHeader.biHeight = -format->u.video.height; video_format->bmiHeader.biPlanes = 1; video_format->bmiHeader.biBitCount = wg_video_format_get_depth(format->u.video.format); video_format->bmiHeader.biCompression = wg_video_format_get_compression(format->u.video.format); @@ -733,6 +735,8 @@ static bool amt_to_wg_format_video(const AM_MEDIA_TYPE *mt, struct wg_format *fo if (IsEqualGUID(&mt->subtype, format_map[i].subtype)) { format->u.video.format = format_map[i].format; + if (wg_video_format_is_rgb(format->u.video.format)) + format->u.video.height = -format->u.video.height; return true; } } @@ -1360,6 +1364,9 @@ static HRESULT decodebin_parser_source_get_media_type(struct parser_source *pin, if (format.major_type == WG_MAJOR_TYPE_VIDEO && index < ARRAY_SIZE(video_formats)) { format.u.video.format = video_formats[index]; + /* Downstream filters probably expect RGB video to be bottom-up. */ + if (format.u.video.height > 0 && wg_video_format_is_rgb(video_formats[index])) + format.u.video.height = -format.u.video.height; if (!amt_from_wg_format(mt, &format, false)) return E_OUTOFMEMORY; return S_OK; diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 19629d12fd0..18138431b20 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -110,6 +110,8 @@ struct wg_format WG_VIDEO_FORMAT_YV12, WG_VIDEO_FORMAT_YVYU, } format; + /* Positive height indicates top-down video; negative height + * indicates bottom-up video. */ int32_t width, height; uint32_t fps_n, fps_d; RECT padding; diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 5bb824f4399..a8da149e7be 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -226,27 +226,6 @@ static NTSTATUS wg_parser_stream_enable(void *args) { bool flip = (format->u.video.height < 0);
- switch (format->u.video.format) - { - case WG_VIDEO_FORMAT_BGRA: - case WG_VIDEO_FORMAT_BGRx: - case WG_VIDEO_FORMAT_BGR: - case WG_VIDEO_FORMAT_RGB15: - case WG_VIDEO_FORMAT_RGB16: - flip = !flip; - break; - - case WG_VIDEO_FORMAT_AYUV: - case WG_VIDEO_FORMAT_I420: - case WG_VIDEO_FORMAT_NV12: - case WG_VIDEO_FORMAT_UYVY: - case WG_VIDEO_FORMAT_YUY2: - case WG_VIDEO_FORMAT_YV12: - case WG_VIDEO_FORMAT_YVYU: - case WG_VIDEO_FORMAT_UNKNOWN: - break; - } - gst_util_set_object_arg(G_OBJECT(stream->flip), "method", flip ? "vertical-flip" : "none"); }
diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 736dbba452c..2e0badba5db 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -1508,6 +1508,10 @@ static HRESULT init_stream(struct wm_reader *reader, QWORD file_size) * Shadowgrounds provides wmv3 video and assumes that the initial * video type will be BGR. */ stream->format.u.video.format = WG_VIDEO_FORMAT_BGR; + + /* API consumers expect RGB video to be bottom-up. */ + if (stream->format.u.video.height > 0) + stream->format.u.video.height = -stream->format.u.video.height; } wg_parser_stream_enable(stream->wg_stream, &stream->format); } @@ -1919,6 +1923,9 @@ static HRESULT WINAPI reader_GetOutputFormat(IWMSyncReader2 *iface, return NS_E_INVALID_OUTPUT_FORMAT; } format.u.video.format = video_formats[index]; + /* API consumers expect RGB video to be bottom-up. */ + if (format.u.video.height > 0 && wg_video_format_is_rgb(format.u.video.format)) + format.u.video.height = -format.u.video.height; break;
case WG_MAJOR_TYPE_AUDIO: @@ -2211,7 +2218,7 @@ static HRESULT WINAPI reader_SetOutputProps(IWMSyncReader2 *iface, DWORD output, hr = NS_E_INVALID_OUTPUT_FORMAT; else if (pref_format.u.video.width != format.u.video.width) hr = NS_E_INVALID_OUTPUT_FORMAT; - else if (pref_format.u.video.height != format.u.video.height) + else if (abs(pref_format.u.video.height) != abs(format.u.video.height)) hr = NS_E_INVALID_OUTPUT_FORMAT; break;
diff --git a/dlls/winegstreamer/wmv_decoder.c b/dlls/winegstreamer/wmv_decoder.c index 473fabab867..874cc1f311f 100644 --- a/dlls/winegstreamer/wmv_decoder.c +++ b/dlls/winegstreamer/wmv_decoder.c @@ -437,7 +437,7 @@ static HRESULT WINAPI media_object_GetOutputType(IMediaObject *iface, DWORD inde return DMO_E_TYPE_NOT_SET;
width = decoder->input_format.u.video_wmv.width; - height = decoder->input_format.u.video_wmv.height; + height = abs(decoder->input_format.u.video_wmv.height); subtype = wmv_decoder_output_types[type_index].subtype; if (FAILED(hr = MFCalculateImageSize(subtype, width, height, &image_size))) { @@ -591,7 +591,7 @@ static HRESULT WINAPI media_object_GetOutputSizeInfo(IMediaObject *iface, DWORD return DMO_E_TYPE_NOT_SET;
if (FAILED(hr = MFCalculateImageSize(&decoder->output_subtype, - decoder->output_format.u.video.width, decoder->output_format.u.video.height, (UINT32 *)size))) + decoder->output_format.u.video.width, abs(decoder->output_format.u.video.height), (UINT32 *)size))) { FIXME("Failed to get image size of subtype %s.\n", debugstr_guid(&decoder->output_subtype)); return hr;
From: Zebediah Figura zfigura@codeweavers.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53903 --- dlls/mfreadwrite/tests/mfplat.c | 23 ++++++++++------------- dlls/winegstreamer/media_source.c | 3 ++- 2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/dlls/mfreadwrite/tests/mfplat.c b/dlls/mfreadwrite/tests/mfplat.c index a6938b8004f..b5b234fd11f 100644 --- a/dlls/mfreadwrite/tests/mfplat.c +++ b/dlls/mfreadwrite/tests/mfplat.c @@ -859,24 +859,21 @@ static void test_source_reader(const char *filename, bool video) hr = IMFMediaType_SetGUID(mediatype, &MF_MT_SUBTYPE, &MFVideoFormat_RGB32); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); hr = IMFSourceReader_SetCurrentMediaType(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, NULL, mediatype); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); IMFMediaType_Release(mediatype);
- if (hr == S_OK) - { - hr = IMFSourceReader_GetCurrentMediaType(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, &mediatype); - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = IMFSourceReader_GetCurrentMediaType(reader, MF_SOURCE_READER_FIRST_VIDEO_STREAM, &mediatype); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
- hr = IMFMediaType_GetGUID(mediatype, &MF_MT_SUBTYPE, &subtype); - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - ok(IsEqualGUID(&subtype, &MFVideoFormat_RGB32), "Got subtype %s.\n", debugstr_guid(&subtype)); + hr = IMFMediaType_GetGUID(mediatype, &MF_MT_SUBTYPE, &subtype); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(IsEqualGUID(&subtype, &MFVideoFormat_RGB32), "Got subtype %s.\n", debugstr_guid(&subtype));
- hr = IMFMediaType_GetUINT32(mediatype, &MF_MT_DEFAULT_STRIDE, &stride); - todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - todo_wine ok(stride == 160 * 4, "Got stride %u.\n", stride); + hr = IMFMediaType_GetUINT32(mediatype, &MF_MT_DEFAULT_STRIDE, &stride); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(stride == 160 * 4, "Got stride %u.\n", stride);
- IMFMediaType_Release(mediatype); - } + IMFMediaType_Release(mediatype); } else { diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index dda7b6df18a..b1cc51552fc 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -856,7 +856,7 @@ static HRESULT new_media_stream(struct media_source *source, static HRESULT media_stream_init_desc(struct media_stream *stream) { IMFMediaTypeHandler *type_handler = NULL; - IMFMediaType *stream_types[6]; + IMFMediaType *stream_types[7]; struct wg_format format; DWORD type_count = 0; unsigned int i; @@ -875,6 +875,7 @@ static HRESULT media_stream_init_desc(struct media_stream *stream) WG_VIDEO_FORMAT_YV12, WG_VIDEO_FORMAT_YUY2, WG_VIDEO_FORMAT_I420, + WG_VIDEO_FORMAT_BGRx, };
IMFMediaType *base_type = mf_media_type_from_wg_format(&format);
v3: Fix mfplat stride calculation in patch 4/5, which fixes the remaining tests.
I think outputting decompressed formats out of the media source always has been a hack and I'm not convinced we should add more formats there.
I think outputting decompressed formats out of the media source always has been a hack and I'm not convinced we should add more formats there.
I've already spent plenty of time arguing that this is not a hack and is in fact the correct approach, and I'd rather not have to repeat that argument yet again. Suffice it to say I still stand by that position.
On Tue Feb 21 20:19:53 2023 +0000, Zebediah Figura wrote:
I think outputting decompressed formats out of the media source always
has been a hack and I'm not convinced we should add more formats there. I've already spent plenty of time arguing that this is not a hack and is in fact the correct approach, and I'd rather not have to repeat that argument yet again. Suffice it to say I still stand by that position.
Native media source doesn't do that, so I don't think it's the right way.
We've seen several applications with a lot of assumptions on MF behavior already, and several other with similar expectations on the DShow graph, expecting decoder filters to be present for instance.
Native media source doesn't do that, so I don't think it's the right way.
We've seen several applications with a lot of assumptions on MF behavior already, and several other with similar expectations on the DShow graph, expecting decoder filters to be present for instance.
Again, I've already given my counterarguments to this, and I do not want to have to repeat them.
The outstanding gitlab test failure is bug 54064, and not related to this patch.
Rémi, I see you've effectively NAK'd this patch. I don't know if that's intentional.
If it is, I will point out a few things:
* How many formats the media source outputs is orthogonal to whether it outputs any at all. Even if the arguments against ever outputting decompressed video held weight—which I believe they do not—none of them are arguments against adding additional formats, as patch 5/5.
* Any application that wants a specific format (in this case, BGRx) is pretty much by definition autoplugging to that format. While applications *can* access the underlying media source from the source reader, it is vanishingly unlikely that they will depend on its output format. The *only* attested or sensible way for applications to depend on receiving uncompressed video is that they try to manually plug graphs.
* At least some native decoders do not output RGB formats anyway, meaning that the only way to get RGB out of a source reader is the way it's done in 81b09cdee, with MF_SOURCE_READER_ENABLE_VIDEO_PROCESSING. This means that in order to implement this in a way closer to native, it would not be sufficient to separate the demuxer and decoder; rather we would need to manually decode YUV to RGB in the media source. Besides adding more overhead to the pipeline, as well as more complexity should it need to be debugged, this will require much more work to implement (and for only particularly unlikely theoretical benefit).
How many formats the media source outputs is orthogonal to whether it outputs any at all. Even if the arguments against ever outputting decompressed video held weight—which I believe they do not—none of them are arguments against adding additional formats, as patch 5/5.
Any format that we add to the media source direct output, has an effect on the topology that will be built. Currently, the topology loader has to insert a video processor or color converter element [*] in the topology, to convert from YUV to a non-YUV format if such format is requested as output.
Although we don't output compressed formats from the media source, this is still closer to how native does it than having the non-YUV format supported directly by the media source.
Any application that wants a specific format (in this case, BGRx) is pretty much by definition autoplugging to that format. While applications *can* access the underlying media source from the source reader, it is vanishingly unlikely that they will depend on its output format. The *only* attested or sensible way for applications to depend on receiving uncompressed video is that they try to manually plug graphs.
Sure, I'm not really trying to make a case for outputting compressed formats here, just saying that I don't think that short-cutting the media foundation topologies is the right way forward. I've seen many games using autoplugging and then at the same time have some expectations about the resulting graph and looking up specific elements in it.
At least some native decoders do not output RGB formats anyway, meaning that the only way to get RGB out of a source reader is the way it's done in 81b09cdee, with MF_SOURCE_READER_ENABLE_VIDEO_PROCESSING. This means that in order to implement this in a way closer to native, it would not be sufficient to separate the demuxer and decoder; rather we would need to manually decode YUV to RGB in the media source. Besides adding more overhead to the pipeline, as well as more complexity should it need to be debugged, this will require much more work to implement (and for only particularly unlikely theoretical benefit).
We don't autoplug decoder elements, because we don't have to for now, but we add converters when needed, and they are pretty much all the time there already, for audio conversion, or when RGB output is required. It has some overhead, yes, but it is mitigated by the transform zero-copy support. In any case I think this is the way forward for compatibility, as I don't think we can have both (short cutting topologies by doing everything in the media source, and supporting topologies similar to native).
[*] Note that these elements somehow have a different behavior with RGB output orientation. I suspect something with internal media type conversion and stride default value. Independently from the comments above I'd expect this MR to fix it too. See https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/mf/tests/transform.c#... and https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/mf/tests/transform.c#... for instance (currently video processor is reversed on Wine, hence the todo_wine, color converter is fine).
How many formats the media source outputs is orthogonal to whether it outputs any at all. Even if the arguments against ever outputting decompressed video held weight—which I believe they do not—none of them are arguments against adding additional formats, as patch 5/5.
Any format that we add to the media source direct output, has an effect on the topology that will be built. Currently, the topology loader has to insert a video processor or color converter element [*] in the topology, to convert from YUV to a non-YUV format if such format is requested as output.
Although we don't output compressed formats from the media source, this is still closer to how native does it than having the non-YUV format supported directly by the media source.
I see the point.
Sure, I'm not really trying to make a case for outputting compressed formats here, just saying that I don't think that short-cutting the media foundation topologies is the right way forward. I've seen many games using autoplugging and then at the same time have some expectations about the resulting graph and looking up specific elements in it.
Wait, so they... expect the presence (or absence) of a whatever-mfplat-calls-videoconvert? What kind of assumptions are they making?
At least some native decoders do not output RGB formats anyway, meaning that the only way to get RGB out of a source reader is the way it's done in 81b09cdee, with MF_SOURCE_READER_ENABLE_VIDEO_PROCESSING. This means that in order to implement this in a way closer to native, it would not be sufficient to separate the demuxer and decoder; rather we would need to manually decode YUV to RGB in the media source. Besides adding more overhead to the pipeline, as well as more complexity should it need to be debugged, this will require much more work to implement (and for only particularly unlikely theoretical benefit).
We don't autoplug decoder elements, because we don't have to for now, but we add converters when needed, and they are pretty much all the time there already, for audio conversion, or when RGB output is required. It has some overhead, yes, but it is mitigated by the transform zero-copy support. In any case I think this is the way forward for compatibility, as I don't think we can have both (short cutting topologies by doing everything in the media source, and supporting topologies similar to native).
It's not the blits I'm worried about; it's everything else: threading overhead, general winegstreamer glue overhead, and the need to decode to YUV and then convert to RGB when the decoder could potentially decode straight to RGB (or at least choose a nicer intermediate format—videoconvert has logic for this). I understand the desire to match native if only to fix theoretical applications, but I think it's being applied too zealously, when there's a distinct cost, and it's not that hard to undo later.
In the specific case of RGB it's worth pointing out an additional fact: the YUV to RGB conversion is done inside the source reader, and I see no evidence that it's possible to retrieve a pointer to that transform (if it even is a transform), so I don't see any way for applications to depend on its presence.
Wait, so they... expect the presence (or absence) of a whatever-mfplat-calls-videoconvert? What kind of assumptions are they making?
They look for a supposed-to-be-here converter for instance, to plug some custom sample grabber after, or before it. Fwiw this is not even specific to MF, I've seen games doing that with DirectShow graphs too, looking for specific decoder elements.
It's not the blits I'm worried about; it's everything else: threading overhead, general winegstreamer glue overhead, and the need to decode to YUV and then convert to RGB when the decoder could potentially decode straight to RGB (or at least choose a nicer intermediate format—videoconvert has logic for this). I understand the desire to match native if only to fix theoretical applications, but I think it's being applied too zealously, when there's a distinct cost, and it's not that hard to undo later.
Overall I don't expect this to require any more thread. The `wg_transform` doesn't usually create any thread, especially when it's used for video conversion. The `videoconvert` may, internally, create some, but that's not in our control.
In the specific case of RGB it's worth pointing out an additional fact: the YUV to RGB conversion is done inside the source reader, and I see no evidence that it's possible to retrieve a pointer to that transform (if it even is a transform), so I don't see any way for applications to depend on its presence.
There's plenty of ways to access it, `IMFSourceReader` isn't even a very commonly used API AFAIK. Most Unreal Engine games use the `IMFMediaSession` which directly notifies the client of created topologies, (Unity Engine often uses the `IMFMediaEngine` which is more isolated).
Wait, so they... expect the presence (or absence) of a whatever-mfplat-calls-videoconvert? What kind of assumptions are they making?
They look for a supposed-to-be-here converter for instance, to plug some custom sample grabber after, or before it. Fwiw this is not even specific to MF, I've seen games doing that with DirectShow graphs too, looking for specific decoder elements.
Okay, that's pretty broken, but not as unreasonable as it could have been...
It's not the blits I'm worried about; it's everything else: threading overhead, general winegstreamer glue overhead, and the need to decode to YUV and then convert to RGB when the decoder could potentially decode straight to RGB (or at least choose a nicer intermediate format—videoconvert has logic for this). I understand the desire to match native if only to fix theoretical applications, but I think it's being applied too zealously, when there's a distinct cost, and it's not that hard to undo later.
Overall I don't expect this to require any more thread. The `wg_transform` doesn't usually create any thread, especially when it's used for video conversion. The `videoconvert` may, internally, create some, but that's not in our control.
Right, threading isn't a concern here, though the rest of it still applies.
In the specific case of RGB it's worth pointing out an additional fact: the YUV to RGB conversion is done inside the source reader, and I see no evidence that it's possible to retrieve a pointer to that transform (if it even is a transform), so I don't see any way for applications to depend on its presence.
There's plenty of ways to access it, `IMFSourceReader` isn't even a very commonly used API AFAIK. Most Unreal Engine games use the `IMFMediaSession` which directly notifies the client of created topologies, (Unity Engine often uses the `IMFMediaEngine` which is more isolated).
I mean that there's no way to retrieve the transform from the source reader. The application from bug 53903 is indeed using a source reader, so other APIs aren't relevant here.
I mean that there's no way to retrieve the transform from the source reader. The application from bug 53903 is indeed using a source reader, so other APIs aren't relevant here.
I think you can with IMFSourceReaderEx::GetTransformForStream(), unfortunately.
I mean that there's no way to retrieve the transform from the source reader. The application from bug 53903 is indeed using a source reader, so other APIs aren't relevant here.
I think you can with IMFSourceReaderEx::GetTransformForStream(), unfortunately.
Hmm, do we know that that retrieves the YUV -> RGB transform? Or just the decoder transform?
On Tue Mar 28 21:30:39 2023 +0000, Zebediah Figura wrote:
I mean that there's no way to retrieve the transform from the source
reader. The application from bug 53903 is indeed using a source reader, so other APIs aren't relevant here.
I think you can with IMFSourceReaderEx::GetTransformForStream(), unfortunately.
Hmm, do we know that that retrieves the YUV -> RGB transform? Or just the decoder transform?
I haven't tried. Docs say that decoder is index 0, so I assumed whole chain is accessible.
AFAIU, the point under debate only concerns the 5th change in this serie. If my understanding is right: - could we at least move forward with the 4 first changes? - I'd like to move forward with an (updated) version of https://gitlab.winehq.org/wine/wine/-/merge_requests/2471. - (if of interest I can take care of rebundling the two MR into a single one)
AFAIU, the point under debate only concerns the 5th change in this serie. If my understanding is right:
- could we at least move forward with the 4 first changes?
I can, although I was hoping that the last patch wouldn't just languish forever.
I still don't think blocking it and demanding a more complex solution be used is useful, not when the more complex solution will require an order of magnitude more development work, will make pipelines harder to debug, may add extra overhead to pipelines, and is only to fix an issue that is not only hypothetical, but even if it does appear, will by nature not be difficult to debug.
-
eric pouech (＠epo) -
Nikolay Sivov (＠nsivov)
-
Rémi Bernon (＠rbernon)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)