[PATCH 0/2] MR5454: winegstreamer: Ensure stream format change notification on size change in h264 decoder.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index aabe618972d..a71d0efb7d3 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -938,6 +938,7 @@ NTSTATUS wg_transform_read_data(void *args) format->u.video.height += format->u.video.padding.top; format->u.video.padding.bottom = align.padding_bottom; format->u.video.height += format->u.video.padding.bottom; + transform->output_format = *format; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5454
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/mf/tests/h264data.bin | Bin 3465 -> 8659 bytes dlls/mf/tests/resource.rc | 2 +- dlls/mf/tests/transform.c | 51 +++++++++++++++++++++++++++++- dlls/winegstreamer/wg_transform.c | 23 ++++++++++++++ 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/dlls/mf/tests/h264data.bin b/dlls/mf/tests/h264data.bin index a97ed63f2056e7f7b668bb96251c0a50690f1893..f552f277928e7b01e79a42cd29b1662c36a6a9f2 100644 GIT binary patch delta 436 zcmeB_zU(|9lH1V0Lc!QD)zC7{a$<@uqw&T`Wjvc5SWmD62`P5vRh}yxb7uO8_(=T! zcWr5a+5-j#21d>RAi+5AWR_z+1MkfVMr^4aO$==NCmS-`*2g$y8~sYZrXHlbw9)v; zv)93s8|U}E=`?#^Ju9Q1owNPOiyLRetyX!J{O|~9bILf%+7kG1o~)M$_s;AJ#=v!w z{quD6W#{opy4GH{%3J(qsXW8uf}Dv9Um8r?nQnUI!*g(a)db?yr*r!Ka;aOd)*gYTY* ziR?>yAS};#DOO<;&~207Fw2WK%QLX+A~`g%1nkhsyVw*NGbSr?igC8N3hGGwXJFTv z{F|+e@#f(a)PEP9MOlNYkvO|IkQp8Sfvf$_%VYz`?f?+2?iP{kS!oyqsvxhH$GNekaZ zGTR;;2sb7RaykH2?P3F~;+ogyDvI9%nHyk341gi&IC~EylsWW)f!wpf_u%Artg6Vi J%>NAuY5)qAo7DgS delta 72 zcmccY+$lXFlH0)2R3RxbHPytxU}B0cqtV7mWjt)g|NmWE8Zh}DyDMY)<Q_#S>8Y-w bIuidG*mW2f7#KMNfCS_GZCQ@>47(a)i1bY&KK diff --git a/dlls/mf/tests/resource.rc b/dlls/mf/tests/resource.rc index d9595c68980..ab1fb7ecbb0 100644 --- a/dlls/mf/tests/resource.rc +++ b/dlls/mf/tests/resource.rc @@ -55,7 +55,7 @@ mp3encdata.bin RCDATA mp3encdata.bin mp3decdata.bin RCDATA mp3decdata.bin /* Generated with: - * gst-launch-1.0 videotestsrc num-buffers=120 pattern=smpte100 ! \ + * gst-launch-1.0 videotestsrc num-buffers=360 pattern=smpte100 ! \ * video/x-raw,format=I420,width=84,height=82,framerate=30000/1001 ! \ * videoflip method=clockwise ! videoconvert ! \ * x264enc ! filesink location=dlls/mf/tests/h264data.bin diff --git a/dlls/mf/tests/transform.c b/dlls/mf/tests/transform.c index e43b1627789..6a4653cccee 100644 --- a/dlls/mf/tests/transform.c +++ b/dlls/mf/tests/transform.c @@ -4379,7 +4379,7 @@ static void test_h264_decoder(void) ok(hr == S_OK, "ProcessMessage returned %#lx\n", hr); } ok(i == 2, "got %lu iterations\n", i); - ok(h264_encoded_data_len == 2425, "got h264_encoded_data_len %lu\n", h264_encoded_data_len); + ok(h264_encoded_data_len == 7619, "got h264_encoded_data_len %lu\n", h264_encoded_data_len); ok(hr == MF_E_TRANSFORM_STREAM_CHANGE, "ProcessOutput returned %#lx\n", hr); ok(output_status == MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE, "got output[0].dwStatus %#lx\n", output_status); hr = IMFSample_GetTotalLength(output_sample, &length); @@ -8368,6 +8368,7 @@ static void test_h264_with_dxgi_manager(void) .attributes = output_sample_attributes, .sample_time = 333667, .sample_duration = 333667, .buffer_count = 1, .buffers = &output_buffer_desc_nv12, + .todo_time = TRUE, /* _SetInputType() / _SetOutputType() type resets the output sample timestamp on Windows. */ }; IMFDXGIDeviceManager *manager = NULL; @@ -8522,6 +8523,54 @@ static void test_h264_with_dxgi_manager(void) ok(info.dwFlags == (MFT_OUTPUT_STREAM_WHOLE_SAMPLES | MFT_OUTPUT_STREAM_SINGLE_SAMPLE_PER_BUFFER | MFT_OUTPUT_STREAM_FIXED_SAMPLE_SIZE | MFT_OUTPUT_STREAM_PROVIDES_SAMPLES), "got %#lx.\n", info.dwFlags); + hr = get_next_h264_output_sample(transform, &input_sample, NULL, output, &data, &data_len); + ok(hr == S_OK, "got %#lx\n", hr); + ok(output[0].dwStatus == 0, "got %#lx.\n", status); + sample = output[0].pSample; + IMFSample_Release(sample); + + /* Set input and output type trying to change output frame size (results in MF_E_TRANSFORM_STREAM_CHANGE with + * frame size reset. */ + hr = MFCreateMediaType(&type); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetGUID(type, &MF_MT_MAJOR_TYPE, &MFMediaType_Video); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetGUID(type, &MF_MT_SUBTYPE, &MFVideoFormat_H264); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetUINT64(type, &MF_MT_FRAME_SIZE, 1088 | (1920ull << 32)); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFTransform_SetInputType(transform, 0, type, 0); + ok(hr == S_OK, "got %#lx\n", hr); + IMFMediaType_Release(type); + hr = IMFTransform_GetOutputAvailableType(transform, 0, 0, &type); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetGUID(type, &MF_MT_MAJOR_TYPE, &MFMediaType_Video); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetGUID(type, &MF_MT_SUBTYPE, &MFVideoFormat_NV12); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFMediaType_SetUINT64(type, &MF_MT_FRAME_SIZE, ((UINT64)1920) << 32 | 1088); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IMFTransform_SetOutputType(transform, 0, type, 0); + ok(hr == S_OK, "got %#lx\n", hr); + IMFMediaType_Release(type); + + hr = get_next_h264_output_sample(transform, &input_sample, NULL, output, &data, &data_len); + ok(hr == MF_E_TRANSFORM_STREAM_CHANGE, "got %#lx\n", hr); + ok(!output[0].pSample, "got %p.\n", output[0].pSample); + + /* Need to set output type with matching size now, or that hangs on Windows. */ + hr = IMFTransform_GetOutputAvailableType(transform, 0, 0, &type); + ok(hr == S_OK, "got %#lx\n", hr); + IMFMediaType_GetUINT64(type, &MF_MT_FRAME_SIZE, &frame_size); + ok(hr == S_OK, "got %#lx\n", hr); + width = frame_size >> 32; + height = frame_size & 0xffffffff; + ok(width == aligned_width, "got %u.\n", width); + ok(height == aligned_height, "got %u.\n", height); + hr = IMFTransform_SetOutputType(transform, 0, type, 0); + ok(hr == S_OK, "got %#lx\n", hr); + IMFMediaType_Release(type); + hr = get_next_h264_output_sample(transform, &input_sample, NULL, output, &data, &data_len); ok(hr == S_OK, "got %#lx\n", hr); ok(output[0].dwStatus == 0, "got %#lx.\n", status); diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index a71d0efb7d3..1ed5593900c 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -610,6 +610,7 @@ NTSTATUS wg_transform_set_output_format(void *args) struct wg_transform_set_output_format_params *params = args; struct wg_transform *transform = get_transform(params->transform); const struct wg_format *format = params->format; + bool frame_size_changed = 0; GstSample *sample; GstCaps *caps; @@ -618,6 +619,12 @@ NTSTATUS wg_transform_set_output_format(void *args) GST_ERROR("Failed to convert format %p to caps.", format); return STATUS_UNSUCCESSFUL; } + + if (format->major_type == WG_MAJOR_TYPE_VIDEO) + { + frame_size_changed = format->u.video.width != transform->output_format.u.video.width + || format->u.video.height != transform->output_format.u.video.height; + } transform->output_format = *format; GST_INFO("transform %p output caps %"GST_PTR_FORMAT, transform, caps); @@ -625,6 +632,22 @@ NTSTATUS wg_transform_set_output_format(void *args) if (transform_output_caps_is_compatible(transform, caps)) { gst_caps_unref(caps); + if (frame_size_changed) + { + GST_TRACE("Caps are compatible but frame size changed.\n"); + if (!gst_pad_peer_query(transform->my_src, transform->drain_query)) + { + GST_ERROR("Failed to drain transform %p.", transform); + return STATUS_UNSUCCESSFUL; + } + if (!(sample = transform->output_sample)) + sample = gst_atomic_queue_peek(transform->output_queue); + + if (sample) + GST_MINI_OBJECT_FLAG_SET(sample, GST_SAMPLE_FLAG_WG_CAPS_CHANGED); + else + transform->output_caps_changed = 1; + } return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5454
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=144629 Your paranoid android. === w7u_el (32 bit report) === mf: mf.c:7227: Test failed: got caps 0x5
Fixes garbled videos in The Finals. When cycling the video and restarting it from the beginning the game sets input and output types with the frame size not matching the video stream size. The transform doesn't (and should not) perform any scaling and the frame size is essentially ignored when comparing gstreamer caps, so wg_transform_set_output_format() doesn't do anything. That results in the disagreement between h264 decoder part in video_decoder.c and wg_transform on the frame size, so the smaller sample from wg_transform is copied to the larger one h264 decoder (wrongly) expects and results in garbled sample. Windows (probably quite expectedly) changes stream format back in that case which results in MF_E_TRANSFORM_STREAM_CHANGE when getting next sample. The output sample frame size is only possible to change with SetOutputType if SetInputType is called before that (otherwise that just fails, we have a test for that). Windows also resets auto timestamp in this case (at least with input sample provided without timestamp), that is left todo in test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_66870
This looks okay but I'm surprised we keep the transform alive when SetInputType is called. Is this the same H264 stream that the game plays? What happens if we destroy the transform on SetInputType? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67010
The game just loops video, when it ends it sets the same samples from start (it sets timestamps on input samples and restarts that from 0). I guess recreating the transform in SetInputType might work too, but why? Gstreamer pipeline creation is known to take a long time, I am almost sure it will introduce noticable hitching when looping video over. I can imagine that in some other cases (when something about input stream actually change) we might need to actually introduce transform recreation in SetInputType, but probably even in that case we would like to do that only if that is actually needed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67048
There could be some buffers from the previous stream remaining in the pipeline if drain / ProcessOutput wasn't called before changing the input type for instance, what needs to be done with the remaining buffers is not clear to me. I don't think creating this specific pipeline takes time, it's more of an issue in decodebin. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67150
The time to create wg_transform takes ~10ms here in test (and ~1ms to destroy). I doubt it is neglectable so we would want to recreate wg_transform when it can be easily avoided, but if you think it is maybe also worth testing in an actual game where there is also some side load. I am attaching the patch on top of this patchset which introduces wg_transform recreation instead of what this patch is doing and prints those times. We currently discard intermediate buffers on (actual) output format change (iirc this is wrong as per tests and to match Windows on format change we'd need to convert those buffers somehow but it is unrelated here). Here we do not have to discard due to internal reasons because format doesn't actually change, frame size is essentially ignored and all the effect of trying to set it is getting the stream change message, all the already decoded samples are in correct format and size. If we'll actually need to discard the buffers it looks trivial to drain transform when setting input type (I am not sure we need to, that'd need more precise tests but maybe that is a separate aspect / change anyway)? [test.patch](/uploads/cfb829919120ea4f7a920ea736ccd2d5/test.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67278
But this only works with H264 codec, which doesn't really care about the input format, as the encoded data carries the actual frame size information. I don't think we want to skip the transform re-creation on SetInputType, even if it's expensive [^1]. It might very well be called for something else than the frame size, or, for other codecs, required anyway to reset the decoder to a different frame size. [^1]: As far as I know, and unlike the media source initialization, nothing requires it to be quick. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67291
But, as far as I can see: - if any other codec will actually be changing frame size, it is already handled in wg_transform_set_output_format() without recreating the transform, it just should not consider caps / formats with different size compatible (and there is transform flag for that); - if any other actual change requires transform recreation, then it can be checked in SetInputType, but it seems not directly related to what I am trying to fix here, that will probably need some case / tests to confirm what actually supposed to happen, maybe SetInputType will just fail? Regarding requiring it to be quick, I guess this case I am concerning here is exactly the example, when the game wants just to loop the video (and calling probably unneeded SetInputType / SetOutputType)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5454#note_67295
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon