[PATCH v3 0/1] MR2471: winegstreamer: In video_processor, activate a videoflip converter.
The app I'm considering opens a video_processor on its own, with a NV12 format on input and a ARGB32 format on output. Tested on Windows: the samples are flipped vertically. While Wine keeps them untouched. So added a videoflip in the video processor to be activated when needed. Current activation is based on RGB vs non RGB input/output formats. Set as draft as if somehow related to MR!2159. Comments welcomed. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> -- v3: winegstreamer: In video_processor, activate a videoflip converter. https://gitlab.winehq.org/wine/wine/-/merge_requests/2471
From: Eric Pouech <eric.pouech(a)gmail.com> The app I'm considering opens a video_processor on its own, with a NV12 format on input and a ARGB32 format on output. Tested on Windows: the samples are flipped vertically. While Wine keeps them untouched. So added a videoflip in the video processor to be activated when needed. This patch depends on MR!2159. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/mf/tests/transform.c | 8 ++++++-- dlls/winegstreamer/color_convert.c | 6 ++++++ dlls/winegstreamer/video_decoder.c | 2 -- dlls/winegstreamer/wg_transform.c | 14 ++++++++++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/dlls/mf/tests/transform.c b/dlls/mf/tests/transform.c index b41f87afabe..830b67beadc 100644 --- a/dlls/mf/tests/transform.c +++ b/dlls/mf/tests/transform.c @@ -6338,8 +6338,12 @@ static void test_video_processor(void) ok(ref == 1, "Release returned %ld\n", ref); ret = check_mf_sample_collection(output_samples, &output_sample_desc, L"rgb32frame-vp.bmp"); - todo_wine - ok(ret == 0 || broken(ret == 25) /* w1064v1507 / w1064v1809 incorrectly rescale */, "got %lu%% diff\n", ret); + /* Color transformation in gstreamer doesn't exactly match Windows' implementation. + * Tolerate some variation in Wine. + */ + ok(ret == 0 || + (!strcmp(winetest_platform, "wine") && ret <= 2) || + broken(ret == 25) /* w1064v1507 / w1064v1809 incorrectly rescale */, "got %lu%% diff\n", ret); IMFCollection_Release(output_samples); output_sample = create_sample(NULL, output_info.cbSize); diff --git a/dlls/winegstreamer/color_convert.c b/dlls/winegstreamer/color_convert.c index 0eaddc687ee..30481e183f5 100644 --- a/dlls/winegstreamer/color_convert.c +++ b/dlls/winegstreamer/color_convert.c @@ -111,6 +111,12 @@ static HRESULT try_create_wg_transform(struct color_convert *impl) if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; + /* don't request vertical flipping in wg_transform */ + if (input_format.major_type == WG_MAJOR_TYPE_VIDEO && input_format.u.video.height < 0) + input_format.u.video.height = -input_format.u.video.height; + if (output_format.major_type == WG_MAJOR_TYPE_VIDEO && output_format.u.video.height < 0) + output_format.u.video.height = -output_format.u.video.height; + if (!(impl->wg_transform = wg_transform_create(&input_format, &output_format))) return E_FAIL; diff --git a/dlls/winegstreamer/video_decoder.c b/dlls/winegstreamer/video_decoder.c index 66df8173038..0087e12abe3 100644 --- a/dlls/winegstreamer/video_decoder.c +++ b/dlls/winegstreamer/video_decoder.c @@ -310,8 +310,6 @@ static HRESULT WINAPI transform_SetOutputType(IMFTransform *iface, DWORD id, IMF { mf_media_type_to_wg_format(decoder->output_type, &output_format); - output_format.u.video.width = frame_size >> 32; - output_format.u.video.height = (UINT32)frame_size; output_format.u.video.fps_d = 0; output_format.u.video.fps_n = 0; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index ccdd90361fc..6a7f27c4dba 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -347,6 +347,13 @@ static struct wg_sample *transform_request_sample(gsize size, void *context) return InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL); } +static bool wg_format_need_video_flip(const struct wg_format *format1, const struct wg_format *format2) +{ + return format1->major_type == WG_MAJOR_TYPE_VIDEO + && format2->major_type == WG_MAJOR_TYPE_VIDEO + && (format1->u.video.height > 0) != (format2->u.video.height > 0); +} + NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; @@ -475,6 +482,13 @@ NTSTATUS wg_transform_create(void *args) goto out; /* Let GStreamer choose a default number of threads. */ gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0"); + if (wg_format_need_video_flip(&input_format, &output_format)) + { + if (!(element = create_element("videoflip", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out; + gst_util_set_object_arg(G_OBJECT(element), "method", "vertical-flip"); + } break; case WG_MAJOR_TYPE_AUDIO_MPEG1: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2471
V2 pushed: - simplify some code - removed todo_wine in transform test -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29562
On Thu Mar 30 07:45:56 2023 +0000, Rémi Bernon wrote: > Yes, I think that's what I was seeing as well. There's also the same > kind of differences in some other tests between windows version. Of > course it'd be nice to be pixel perfect but I don't think it matters > very much. It's also possibly related to the very small video frame > size, or at least, it makes it more visible. I searched a bit further: - gstreamer seems to apply some smoothing (and the bars in the input image make it appear more clearly than for a "normal" image) - I'm still a bit worried about the one color large difference (see band 0 above), but again couldn't find a simple solution So keeping it as is. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29563
Rémi Bernon (@rbernon) commented about dlls/mf/tests/transform.c:
ok(ref == 1, "Release returned %ld\n", ref);
ret = check_mf_sample_collection(output_samples, &output_sample_desc, L"rgb32frame-vp.bmp"); - todo_wine - ok(ret == 0 || broken(ret == 25) /* w1064v1507 / w1064v1809 incorrectly rescale */, "got %lu%% diff\n", ret); + /* Color transformation in gstreamer doesn't exactly match Windows' implementation. + * Tolerate some variation in Wine. + */ + ok(ret == 0 || + (!strcmp(winetest_platform, "wine") && ret <= 2) || + broken(ret == 25) /* w1064v1507 / w1064v1809 incorrectly rescale */, "got %lu%% diff\n", ret);
Imho we could unconditionally relax this to accept up to 2% difference. The purpose of the test was to catch vertical flips and invalid cropping, not to be pixel perfect. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29567
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/wg_transform.c:
return InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL); }
+static bool wg_format_need_video_flip(const struct wg_format *format1, const struct wg_format *format2) +{ + return format1->major_type == WG_MAJOR_TYPE_VIDEO + && format2->major_type == WG_MAJOR_TYPE_VIDEO + && (format1->u.video.height > 0) != (format2->u.video.height > 0); +} +
What about decoding from compressed format to RGB? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29568
Rémi Bernon (@rbernon) commented about dlls/winegstreamer/color_convert.c:
if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE;
+ /* don't request vertical flipping in wg_transform */ + if (input_format.major_type == WG_MAJOR_TYPE_VIDEO && input_format.u.video.height < 0) + input_format.u.video.height = -input_format.u.video.height; + if (output_format.major_type == WG_MAJOR_TYPE_VIDEO && output_format.u.video.height < 0) + output_format.u.video.height = -output_format.u.video.height; +
I think the difference with video_processor is related to the media types attributes, rather than something unconditional like this. For instance, it is possible to force the frame orientation by explicitly specifying `MF_MT_DEFAULT_STRIDE` attribute in both cases. The tests don't do that, but maybe it should. The default for color_converter seems to be a positive `MF_MT_DEFAULT_STRIDE` value, and it even exposes it in the current media type as the test shows, outputting the same orientation as the input NV12 frame. The video_converter doesn't add a default value, but behaves as if it was negative by default for RGB, reversing the output compared to the NV12 frame input. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29569
On Tue Apr 11 09:31:11 2023 +0000, Rémi Bernon wrote:
Imho we could unconditionally relax this to accept up to 2% difference. The purpose of the test was to catch vertical flips and invalid cropping, not to be pixel perfect. I wrote it that way to maintain the fact that the test is pixel accurate on (recent) Windows. But this can be relaxed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29596
On Tue Apr 11 09:31:12 2023 +0000, Rémi Bernon wrote: > I think the difference with video_processor is related to the media > types attributes, rather than something unconditional like this. > For instance, it is possible to force the frame orientation by > explicitly specifying `MF_MT_DEFAULT_STRIDE` attribute in both cases. > The tests don't do that, but maybe it should. > The default for color_converter seems to be a positive > `MF_MT_DEFAULT_STRIDE` value, and it even exposes it in the current > media type as the test shows, outputting the same orientation as the > input NV12 frame. > The video_converter doesn't add a default value, but behaves as if it > was negative by default for RGB, reversing the output compared to the > NV12 frame input. in fact there might be two issues to take care of (covering also your next comment): - various decoders are now exposed to potentially negative heights, which some of them are not prepared for (eg. h264 or dmo) - orientation when MF_MT_DEFAULT_STRIDE isn't specified in MediaType (color_converter and videoconverter may provide behave differently) so I wonder which design is better: - hide completly the flip information to decoders (passing abs(height)) and handle in wg_transform only - expose the flip request to decoders in case they are able to handle it? (and fallback to flip in wg_transform if they don't handle it) looks like we maybe need some more tests for various stride values -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29597
On Tue Apr 11 13:25:29 2023 +0000, eric pouech wrote:
I wrote it that way to maintain the fact that the test is pixel accurate on (recent) Windows. But this can be relaxed. I think something like `todo_wine_if(ret <= 2)` would be more readable if you want to keep the idea that we're aiming for pixel perfect result.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29638
On Tue Apr 11 13:26:36 2023 +0000, eric pouech wrote:
in fact there might be two issues to take care of (covering also your next comment): - various decoders are now exposed to potentially negative heights, which some of them are not prepared for (eg. h264 or dmo) - orientation when MF_MT_DEFAULT_STRIDE isn't specified in MediaType (color_converter and videoconverter may provide behave differently) so I wonder which design is better: - hide completly the flip information to decoders (passing abs(height)) and handle in wg_transform only - expose the flip request to decoders in case they are able to handle it? (and fallback to flip in wg_transform if they don't handle it) looks like we maybe need some more tests for various stride values Fwiw I don't think native allows negative `MF_MT_DEFAULT_STRIDE` for anything else than RGB formats, and we shouldn't either. H264 decoder doesn't support RGB format output, but WMV decoder does and I think we should check how it behaves wrt frame orientation (eventually adding a test).
We should probably keep the flipping decision to wg_transform, almost like it's done. Actually more like it was done initially as you'll need to keep a `struct wg_format input_format` and `GstElement *videoflip` in the `struct wg_transform` to use them in `wg_transform_set_output_format`. I'd use a helper like this, and simply compare `wg_format_is_flipped(input) != wg_format_is_flipped(output)` to decide which value to set the videoflip method to: ```C static bool wg_format_is_flipped(const struct wg_format *format) { return format->major_type == WG_MAJOR_TYPE_VIDEO && format->u.video.height < 0; } ``` Then, regarding how `video.height` is set, this is purely a matter of media type translation. I think it's even visible when you test the MF conversion functions to translate a `IMFMediaType` to the various legacy representation like `AM_MEDIA_TYPE`, through `IMFMediaType_GetRepresentation` for instance. We don't have these conversions functions, at the moment and we probably don't need them for this, but the `wg_format` translation should probably follow them. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_29639
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Rémi Bernon