[PATCH v2 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> -- v2: 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. 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/color_convert.c | 6 ++++++ dlls/winegstreamer/video_decoder.c | 5 ----- dlls/winegstreamer/wg_transform.c | 31 ++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 5 deletions(-) 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..617c39c5eee 100644 --- a/dlls/winegstreamer/video_decoder.c +++ b/dlls/winegstreamer/video_decoder.c @@ -310,11 +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; - if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN || !wg_transform_set_output_format(decoder->wg_transform, &output_format)) { diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 65a34511284..dfa4d993998 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -53,6 +53,9 @@ struct wg_transform GstSegment segment; GstQuery *drain_query; + GstElement *video_flip; + struct wg_format format_input; + guint input_max_length; GstAtomicQueue *input_queue; @@ -347,6 +350,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; @@ -474,6 +484,11 @@ 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 (!(element = create_element("videoflip", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out; + transform->video_flip = element; + transform->format_input = input_format; break; case WG_MAJOR_TYPE_AUDIO_MPEG1: @@ -501,6 +516,14 @@ NTSTATUS wg_transform_create(void *args) if (!gst_pad_set_active(transform->my_src, 1)) 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"); + } + gst_element_set_state(transform->container, GST_STATE_PAUSED); if (!gst_element_get_state(transform->container, NULL, NULL, -1)) goto out; @@ -585,6 +608,14 @@ NTSTATUS wg_transform_set_output_format(void *args) return STATUS_UNSUCCESSFUL; } + if (transform->video_flip) + { + if (wg_format_need_video_flip(&transform->format_input, 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"); + } + gst_caps_unref(transform->output_caps); transform->output_caps = caps; -- 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=131006 Your paranoid android. === debian11 (32 bit report) === dinput: device8.c:418: Test failed: 0x800: WaitForSingleObject returned 0x102 device8.c:423: Test failed: 0x800: got count 0
On Fri Mar 24 09:25:44 2023 +0000, Rémi Bernon wrote: > 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. some update to this draft: - you're right about the trigger based on stride sign; now using that - patch is now based on MR!2159 as it already does pushing the sign information to wg_transform the color_video test still passes and the videoproc test (mf/tests/transform.c:6320) almost passes: - the good news: the % of error went down from 85% to 2%, - but I fear that something still needs care here: digging a bit further on the 2% distance: out of 96*96*3 bytes compared, 5834 have an error which is more or equal to 2 (and some greater than that), which looks suspicious from what should pure rounding discrepancy. Didn't look further. however, I didn't find a simple way to trigger flip video in gstreamer. This doesn't mean there isn't. So, I kept using the explicit videoflip element in the pipeline. What remains to be done: - simplify it from not storing as much information in struct wg_transform - theorically, we should pass the stride value if defined by caller (apart from the sign) to wg_transform.c. - analyze discrepancy on transform.c:videoproc test. Further work on this patch is conditionned on how things settle down on 2159. So: ``` WaitForSingleEvent(MR!2159, INFINITE) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_27762
On Wed Mar 22 18:58:16 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` 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 ``` V2 should be ok (but disabled pipeline because of dep to MR2159)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_27763
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.
I'm afraid videoflip is the only way to do this. Negative stride is a convention used by mfplat and dshow; gstreamer doesn't use it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_27814
but I fear that something still needs care here: digging a bit further on the 2% distance: out of 96*96*3 bytes compared, 5834 have an error which is more or equal to 2 (and some greater than that), which looks suspicious from what should pure rounding discrepancy. Didn't look further.
Fwiw I don't think such low differences matter very much. I think it's likely some diff in color sampling, and I think windows does a bad job at it with a lot of color bleeding. There's already some differences between windows versions in some tests. Also the test produces a .bmp file that can be used to visually compare the results (for YUV images, the .bmp is only a representation of the data, which is appended to the the .bmp data). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_28253
> Fwiw I don't think such low differences matter very much. I think it's likely some diff in color sampling, and I think windows does a bad job at it with a lot of color bleeding. There's already some differences between windows versions in some tests. I just want to be sure of that... Digging just a bit more shows two interesting effets: - it seems that gstreamer applies some smoothing effet in color transition: the expected output has clear transitions between bands while gstreamer is smoother - (ascii dump of rgb colors, one char <= one different rgb color) - expect: ``` line 58: $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$ line 59: $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$ line 60: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% line 60: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% ``` - gstreamer: ``` line 58: ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;< line 59: =================================================================================> line 60: ?????????????????????????????????????????????????????????????????????????????????@ line 61: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAB ``` - note also the elements on end of line (also smoothing with vertical band of test sample) that's strange as yuv stores its information on a 2x2 rectangle, and all the bands in source are expected to be aligned on this 2x2 thingie, so either there's a misalignment of 1 somewhere (but would be on both directions), or winegstreamer adds some interpolation in the process looking at the diff of colors (picking same pixel in the middle of each band) shows: ``` band 0: cdcdcd <> ff7bff band 1: ff0000 <> fd0000 band 2: 0000fe <> 0000fd band 3: fe00ff <> fc00fd band 4: 01ff00 <> 00fa00 band 5: ffff00 <> fdfc00 band 6: 00ffff <> 00fbfb band 7: ffffff <> fdfdfd ``` diff:s for band 1 to 7 are very small, but band 0 is really more questionnable. do these ring any bell? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_28300
On Wed Mar 29 09:40:55 2023 +0000, eric pouech wrote: > > Fwiw I don't think such low differences matter very much. I think it's > likely some diff in color sampling, and I think windows does a bad job > at it with a lot of color bleeding. There's already some differences > between windows versions in some tests. > I just want to be sure of that... > Digging just a bit more shows two interesting effets: > - it seems that gstreamer applies some smoothing effet in color > transition: the expected output has > clear transitions between bands while gstreamer is smoother > - (ascii dump of rgb colors, one char <= one different rgb color) > - expect: > ``` > line 58: $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$ > line 59: $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$ > line 60: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% > line 60: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% > ``` > - gstreamer: > ``` > line 58: ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;< > line 59: =================================================================================> > line 60: ?????????????????????????????????????????????????????????????????????????????????@ > line 61: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAB > ``` > - note also the elements on end of line (also smoothing with vertical > band of test sample) > that's strange as yuv stores its information on a 2x2 rectangle, and all > the bands in source are expected to be aligned on this 2x2 thingie, so > either there's a misalignment of 1 somewhere (but would be on both > directions), or winegstreamer adds some interpolation in the process > looking at the diff of colors (picking same pixel in the middle of each > band) shows: > ``` > band 0: cdcdcd <> ff7bff > band 1: ff0000 <> fd0000 > band 2: 0000fe <> 0000fd > band 3: fe00ff <> fc00fd > band 4: 01ff00 <> 00fa00 > band 5: ffff00 <> fdfc00 > band 6: 00ffff <> 00fbfb > band 7: ffffff <> fdfdfd > ``` > diff:s for band 1 to 7 are very small, but band 0 is really more questionnable. > do these ring any bell? 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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2471#note_28404
participants (5)
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Rémi Bernon
-
Zebediah Figura (＠zfigura)