[PATCH v4 0/4] MR2159: winegstreamer: Stride and RGB formats, part 2/2.
From: Zebediah Figura <zfigura(a)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) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2159
From: Zebediah Figura <zfigura(a)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 = -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2159
From: Zebediah Figura <zfigura(a)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) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2159
From: Zebediah Figura <zfigura(a)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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2159
On Thu Apr 6 17:12:01 2023 +0000, Zebediah Figura wrote:
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. It is not a hypothetical issue.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2159#note_29195
It is not a hypothetical issue.
In this case it is, unless you can point to a specific program that will be broken by this patch? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2159#note_29196
On Thu Apr 6 17:45:30 2023 +0000, Zebediah Figura wrote:
It is not a hypothetical issue. In this case it is, unless you can point to a specific program that will be broken by this patch? I don't think this is how it works. If you want to merge this, you have to make a case that it goes in the right direction. I think it's not, because Windows doesn't work like that.
I don't remember or note every game I debug, but I know that I've found enough evidence that this is going to be a problem sooner or later. Whether debugging is made easier never has been an argument for merging a change, before compatibility. Anyway, I see that you've dropped the controversial change, so I'll happily unblock this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2159#note_29208
I don't think this is how it works. If you want to merge this, you have to make a case that it goes in the right direction. I think it's not, because Windows doesn't work like that.
Simply "Windows doesn't work like that" is not a sufficient argument against a patch, there are dozens, probably hundreds of places in Wine where we don't follow Windows for one reason or another. I believe there are good reasons here.
I don't remember or note every game I debug, but I know that I've found enough evidence that this is going to be a problem sooner or later. Whether debugging is made easier never has been an argument for merging a change, before compatibility.
The point is that it's not always perfectly practical to implement everything correctly. When it's not, and no known application depends on a feature, you have to decide whether to do it anyway. Besides the actual difficulty going into implementing and maintaining the feature, another factor to take into account when deciding is to ask "how hard will it be to find out that an application fails if I don't implement this?" This is a great argument, for example, for things like not bothering with E_POINTER checks in every conceivable place (because if an application really passes NULL and you don't catch that, you'll just crash, and it takes 0 debugging effort to find out why you crashed.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2159#note_29217
participants (3)
-
Rémi Bernon -
Zebediah Figura
-
Zebediah Figura (＠zfigura)