[PATCH 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> -- 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. 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> --- dlls/winegstreamer/wg_transform.c | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 65a34511284..b5439c7a639 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,6 +52,7 @@ struct wg_transform GstPad *their_sink, *their_src; GstSegment segment; GstQuery *drain_query; + GstElement *video_flip; guint input_max_length; GstAtomicQueue *input_queue; @@ -347,6 +348,39 @@ static struct wg_sample *transform_request_sample(gsize size, void *context) return InterlockedExchangePointer((void **)&transform->output_wg_sample, NULL); } +static 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; +} + +static bool wg_format_need_video_flip(const struct wg_format *format1, const struct wg_format *format2) +{ + /* Assuming all RGB formats are bottom-up, and all the others top-down. */ + return format1->major_type == WG_MAJOR_TYPE_VIDEO + && format2->major_type == WG_MAJOR_TYPE_VIDEO + && wg_video_format_is_rgb(format1->u.video.format) != wg_video_format_is_rgb(format2->u.video.format); +} + NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; @@ -377,6 +411,7 @@ NTSTATUS wg_transform_create(void *args) goto out; transform->input_max_length = 1; transform->output_plane_align = 0; + transform->video_flip = NULL; if (!(src_caps = wg_format_to_caps(&input_format))) goto out; @@ -474,6 +509,10 @@ 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 (!(transform->video_flip = create_element("videoflip", "base")) + || !transform_append_element(transform, transform->video_flip, &first, &last)) + goto out; break; case WG_MAJOR_TYPE_AUDIO_MPEG1: @@ -488,6 +527,14 @@ NTSTATUS wg_transform_create(void *args) goto out; } + if (transform->video_flip) + { + if (wg_format_need_video_flip(&input_format, &output_format)) + gst_util_set_object_arg(G_OBJECT(transform->video_flip), "method", "vertical-flip"); + else + gst_util_set_object_arg(G_OBJECT(transform->video_flip), "method", "none"); + } + if (!(transform->their_sink = gst_element_get_static_pad(first, "sink"))) goto out; if (!(transform->their_src = gst_element_get_static_pad(last, "src"))) @@ -563,6 +610,7 @@ NTSTATUS wg_transform_set_output_format(void *args) struct wg_transform_set_output_format_params *params = args; struct wg_transform *transform = params->transform; const struct wg_format *format = params->format; + struct wg_format old_output_format; GstSample *sample; GstCaps *caps; gchar *str; @@ -585,9 +633,19 @@ NTSTATUS wg_transform_set_output_format(void *args) return STATUS_UNSUCCESSFUL; } + wg_format_from_caps(&old_output_format, transform->output_caps); gst_caps_unref(transform->output_caps); transform->output_caps = caps; + if (transform->video_flip && wg_format_need_video_flip(&old_output_format, format)) + { + gchar* prop; + g_object_get(G_OBJECT(transform->video_flip), "method", &prop, NULL); + gst_util_set_object_arg(G_OBJECT(transform->video_flip), "method", + (!prop || !strcmp(prop, "none")) ? "vertical-flip" : "none"); + g_free(prop); + } + if (!gst_pad_push_event(transform->my_sink, gst_event_new_reconfigure())) { GST_ERROR("Failed to reconfigure transform %p.", transform); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2471
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=130932 Your paranoid android. === debian11 (32 bit report) === mfmediaengine: mfmediaengine.c:1300: Test failed: Unexpected 52% diff mf: transform.c:5738: Test failed: colorconv: got 82% diff
As far as I know, it's all related to the media type `MF_MT_DEFAULT_STRIDE` attribute, and it being implicitly negative for RGB formats. And for legacy media type structures, such as `VIDEOFORMAT`, I believe there is a native translation to MF media type, using MF APIs, that we should mimic. YUV formats simply cannot have a negative stride, and it usually raises `MF_E_INVALID_MEDIA_TYPE` errors. Then, I'm wondering if we can have a better integration of signed strides in the code rather than relying on a ad-hoc `videoflip` element. The `wg_transform` has a buffer pool with metadata, and should be able to convey the plane alignment and stride requirements to the GStreamer elements. Whether this can translate a negative stride to a vertically flipped frame or not, I don't know, but I think it's worth exploring. This would improve compatibility with other possible stride values, as I believe we have some known issues already with some YUV formats using a different default stride on Windows and GStreamer. In any case I'd expect the stride sign to checked when deciding whether to flip or not the buffers, and I think it also needs to be taken into account in the plane padding computation. As far as the video processor tests goes, the padding is also usually reversed with RGB formats. And yes, it's definitely related to !2159. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_27550
participants (4)
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Rémi Bernon