[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@codeweavers.com
-- v3: winegstreamer: In video_processor, activate a videoflip converter.
From: Eric Pouech eric.pouech@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@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:
V2 pushed: - simplify some code - removed todo_wine in transform test
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.
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.
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?
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.
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.
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
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.
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.
-
Eric Pouech -
eric pouech (＠epo)
-
Rémi Bernon (＠rbernon)