Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698
-- v8: quartz/tests: Test that avi_decompressor_source_qc_Notify does not deadlock if called from a foreign thread during IMemInput_Receive. quartz: Allow concurrent calls to AVI decoder qc_Notify and Receive. msvfw32/tests: Test that Cinepak rejects unsupported output types. iccvid: Reject unsupported output types. quartz/tests: Add Cinepak test to avi splitter. winegstreamer: Make AVI splitter use end of previous frame if the current frame doesn't have a timestamp. winegstreamer: Implement AM_MEDIA_TYPE to wg_format converter for Cinepak video.
From: Alfred Agrell floating@muncher.se
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698 --- dlls/winegstreamer/quartz_parser.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 125e5c00c0f..69c618e213e 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -977,6 +977,30 @@ static bool amt_to_wg_format_video(const AM_MEDIA_TYPE *mt, struct wg_format *fo return false; }
+static bool amt_to_wg_format_video_cinepak(const AM_MEDIA_TYPE *mt, struct wg_format *format) +{ + const VIDEOINFOHEADER *video_format = (const VIDEOINFOHEADER *)mt->pbFormat; + + if (!IsEqualGUID(&mt->formattype, &FORMAT_VideoInfo)) + { + FIXME("Unknown format type %s.\n", debugstr_guid(&mt->formattype)); + return false; + } + if (mt->cbFormat < sizeof(VIDEOINFOHEADER) || !mt->pbFormat) + { + ERR("Unexpected format size %lu.\n", mt->cbFormat); + return false; + } + + format->major_type = WG_MAJOR_TYPE_VIDEO_CINEPAK; + format->u.video.width = video_format->bmiHeader.biWidth; + format->u.video.height = video_format->bmiHeader.biHeight; + format->u.video.fps_n = 10000000; + format->u.video.fps_d = video_format->AvgTimePerFrame; + + return true; +} + static bool amt_to_wg_format_video_wmv(const AM_MEDIA_TYPE *mt, struct wg_format *format) { const VIDEOINFOHEADER *video_format = (const VIDEOINFOHEADER *)mt->pbFormat; @@ -1051,6 +1075,8 @@ bool amt_to_wg_format(const AM_MEDIA_TYPE *mt, struct wg_format *format)
if (IsEqualGUID(&mt->majortype, &MEDIATYPE_Video)) { + if (IsEqualGUID(&mt->subtype, &MEDIASUBTYPE_CVID)) + return amt_to_wg_format_video_cinepak(mt, format); if (IsEqualGUID(&mt->subtype, &MEDIASUBTYPE_WMV1) || IsEqualGUID(&mt->subtype, &MEDIASUBTYPE_WMV2) || IsEqualGUID(&mt->subtype, &MEDIASUBTYPE_WMVA)
From: Alfred Agrell floating@muncher.se
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698 --- dlls/winegstreamer/quartz_parser.c | 36 +++++++++++++++++++----------- 1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 69c618e213e..c42f5124f80 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -100,6 +100,9 @@ struct parser_source bool need_segment;
bool eos; + + bool interpolate_timestamps; + UINT64 prev_end_pts; };
static inline struct parser *impl_from_strmbase_filter(struct strmbase_filter *iface) @@ -1179,30 +1182,34 @@ static HRESULT send_sample(struct parser_source *pin, IMediaSample *sample, return S_OK; }
- if (buffer->has_pts) + if (buffer->has_pts || (pin->interpolate_timestamps && pin->prev_end_pts != 0)) { - REFERENCE_TIME start_pts = buffer->pts; + UINT64 start_pts = (buffer->has_pts ? buffer->pts : pin->prev_end_pts); + REFERENCE_TIME start_reftime = start_pts;
if (offset) - start_pts += scale_uint64(offset, 10000000, bytes_per_second); - start_pts -= pin->seek.llCurrent; - start_pts *= pin->seek.dRate; + start_reftime += scale_uint64(offset, 10000000, bytes_per_second); + start_reftime -= pin->seek.llCurrent; + start_reftime *= pin->seek.dRate;
if (buffer->has_duration) { - REFERENCE_TIME end_pts = buffer->pts + buffer->duration; + UINT64 end_pts = start_pts + buffer->duration; + REFERENCE_TIME end_reftime = end_pts;
+ pin->prev_end_pts = end_pts; if (offset + size < buffer->size) - end_pts = buffer->pts + scale_uint64(offset + size, 10000000, bytes_per_second); - end_pts -= pin->seek.llCurrent; - end_pts *= pin->seek.dRate; + end_reftime = end_reftime + scale_uint64(offset + size, 10000000, bytes_per_second); + end_reftime -= pin->seek.llCurrent; + end_reftime *= pin->seek.dRate;
- IMediaSample_SetTime(sample, &start_pts, &end_pts); - IMediaSample_SetMediaTime(sample, &start_pts, &end_pts); + IMediaSample_SetTime(sample, &start_reftime, &end_reftime); + IMediaSample_SetMediaTime(sample, &start_reftime, &end_reftime); } else { - IMediaSample_SetTime(sample, &start_pts, NULL); + pin->prev_end_pts = 0; + IMediaSample_SetTime(sample, &start_reftime, NULL); IMediaSample_SetMediaTime(sample, NULL, NULL); } } @@ -2317,6 +2324,7 @@ static const struct strmbase_sink_ops avi_splitter_sink_ops = static BOOL avi_splitter_filter_init_gst(struct parser *filter) { wg_parser_t parser = filter->wg_parser; + struct parser_source *src; uint32_t i, stream_count; WCHAR source_name[20];
@@ -2324,8 +2332,10 @@ static BOOL avi_splitter_filter_init_gst(struct parser *filter) for (i = 0; i < stream_count; ++i) { swprintf(source_name, ARRAY_SIZE(source_name), L"Stream %02u", i); - if (!create_pin(filter, wg_parser_get_stream(parser, i), source_name)) + src = create_pin(filter, wg_parser_get_stream(parser, i), source_name); + if (!src) return FALSE; + src->interpolate_timestamps = TRUE; }
return TRUE;
From: Alfred Agrell floating@muncher.se
--- dlls/quartz/tests/avisplit.c | 11 ++++++++--- dlls/quartz/tests/rsrc.rc | 4 ++++ dlls/quartz/tests/test_cinepak.avi | Bin 0 -> 6702 bytes 3 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 dlls/quartz/tests/test_cinepak.avi
diff --git a/dlls/quartz/tests/avisplit.c b/dlls/quartz/tests/avisplit.c index 25e313aa4b4..f7fadb606c4 100644 --- a/dlls/quartz/tests/avisplit.c +++ b/dlls/quartz/tests/avisplit.c @@ -1677,9 +1677,9 @@ static void test_seeking(void) ok(ret, "Failed to delete file, error %lu.\n", GetLastError()); }
-static void test_streaming(void) +static void test_streaming(const WCHAR *resname) { - const WCHAR *filename = load_resource(L"test.avi"); + const WCHAR *filename = load_resource(resname); IBaseFilter *filter = create_avi_splitter(); IFilterGraph2 *graph = connect_input(filter, filename); struct testfilter testsink; @@ -1691,6 +1691,8 @@ static void test_streaming(void) ULONG ref; DWORD ret;
+ winetest_push_context("File %ls", resname); + testfilter_init(&testsink); IFilterGraph2_AddFilter(graph, &testsink.filter.IBaseFilter_iface, L"sink"); IFilterGraph2_QueryInterface(graph, &IID_IMediaControl, (void **)&control); @@ -1761,6 +1763,8 @@ static void test_streaming(void) ok(!ref, "Got outstanding refcount %ld.\n", ref); ret = DeleteFileW(filename); ok(ret, "Failed to delete file, error %lu.\n", GetLastError()); + + winetest_pop_context(); }
START_TEST(avisplit) @@ -1787,7 +1791,8 @@ START_TEST(avisplit) test_unconnected_filter_state(); test_connect_pin(); test_seeking(); - test_streaming(); + test_streaming(L"test.avi"); + test_streaming(L"test_cinepak.avi");
CoUninitialize(); } diff --git a/dlls/quartz/tests/rsrc.rc b/dlls/quartz/tests/rsrc.rc index a16edacf1d1..a15520d21ac 100644 --- a/dlls/quartz/tests/rsrc.rc +++ b/dlls/quartz/tests/rsrc.rc @@ -39,3 +39,7 @@ test.wav RCDATA "test.wav" /* ffmpeg -f lavfi -i smptebars -f lavfi -i "sine=frequency=1000" -t 2.04 -r 25 -f mpeg -vcodec mpeg1video -vf scale=64x48 -acodec mp2 -ar 32k -ab 96k test2.mpg */ /* @makedep: test2.mpg */ test2.mpg RCDATA "test2.mpg" + +/* ffmpeg -f lavfi -i smptebars -t 5 -r 1 -f avi -vcodec cinepak -pix_fmt rgb24 -vf scale=32x24 test_cinepak.avi */ +/* @makedep: test_cinepak.avi */ +test_cinepak.avi RCDATA "test_cinepak.avi" diff --git a/dlls/quartz/tests/test_cinepak.avi b/dlls/quartz/tests/test_cinepak.avi new file mode 100644 index 0000000000000000000000000000000000000000..006ee289b37ff396103fae36050b8ba6bf8ce5be GIT binary patch literal 6702 zcmeHMO-vI(6n@(-rG+kR3qt9(vTgZoD703j{3PHHjg<<>Pc;z}357tyU%)iR7*Y!f zfqFK?iw6)i#ux}EB1BFKoIDyYCYl(355xmOWPQ`6hV<yk$b9Y0y!Ynqo0%^=yPG#H zKCidH2yi;!6aBu{Hjf^lH#pMY8S3lx06^WDba>Q>0)p27B*`K$vIRtJ4F|Tv4T<%R z2N=CE(vKw;E7TVp?ZWD?ABiK!l4C~|01_hZBVu&CzNZKcq=;C^cnsFDSU9IdTxf4> zFyPqiL5I^B>`L^ZI%R+|KpCJ6PzERilmW^BWq>k38K4YM1}FpnHv^&Jk>PWPmPqms zA{LTYP-4~~E)zSk#dXpqg5(-lbQ}r}^y9yGC(+>u=_KcasU+|<dYgQ$-nKN9{hgtn z^2*W^6{Rkx69`*7$|FPxT`FaOGVq51@`SjK`+s04)JL+iB-;z1h~gvG01OX`J26)S zh$1i(7K_bh=dMh(jAy(qzG}ag{h4Wi^AhuXd1Xl<Jw!a^GwY0goo-=aX670H<r_&H zPfbrviz@|%g3z?xj;J}6oT|dKbw6TrV|jk=-Yd=H=>3Ve-zyOB>-Q0oWK84KS$irK zF#GJ{?BeW84X4e}@BSJX92nf#`0+rRS1DhtFD|i?r^HjT>r#`6N`r&l-CJ9ek;zE< znqU%4E!z{5k#S+oU+b^cRxd9v;XFMaxBJ7{6tx1R{Yr<s<frp;{l(Lc;*Q3Srd)Gb znagGVNP>mK(Yuj|WmT3ZC&q8{>$1$^VA4L3C8@2oGvGPNaYVD^nXN_KEz_t`^NHx; zurw#lk+%_%1);F8X<L>X>Y6LH9$9V=1OnCW`RIJ~&DYhn)inkzdTWM|EcYI+tpMIt z1QW}~hr@E83t^1SnCS9If^g|zB-tX`L|d^IRQLlYz>~4DF$pr0GLxz_RT>j#;<ECy z@>6qDbNL)TNA)*7)zt<=W>Q*ml_tH&#My<cd;tSiR`59%o7r4mF7*PWq!bkBk15ou zQgwEo)(KXorsTNGYRphNw5(WGrc~$@I-OhR*3>v^9Bc{;<es$S5|BbW#O_I*{Q4&! z0-}gV3djlT0??9m68xK2^aXFanh$1&2O&UCLdaf=pM=byOx79KHxu-|1icjZEb;#h DyVUok
literal 0 HcmV?d00001
From: Alfred Agrell floating@muncher.se
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698 --- dlls/iccvid/iccvid.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/dlls/iccvid/iccvid.c b/dlls/iccvid/iccvid.c index a1cfdc1ee21..56a882af659 100644 --- a/dlls/iccvid/iccvid.c +++ b/dlls/iccvid/iccvid.c @@ -797,26 +797,21 @@ static LRESULT ICCVID_DecompressQuery( ICCVID_Info *info, LPBITMAPINFO in, LPBIT if( in->bmiHeader.biWidth != out->bmiHeader.biWidth ) return ICERR_BADFORMAT;
- switch( out->bmiHeader.biBitCount ) + switch( out->bmiHeader.biCompression ) { - case 16: - if ( out->bmiHeader.biCompression == BI_BITFIELDS ) - { - if ( !ICCVID_CheckMask(out->bmiColors, 0x7C00, 0x03E0, 0x001F) && - !ICCVID_CheckMask(out->bmiColors, 0xF800, 0x07E0, 0x001F) ) - { - TRACE("unsupported output bit field(s) for 16-bit colors\n"); - return ICERR_BADFORMAT; - } - } + case BI_RGB: + if ( out->bmiHeader.biBitCount == 16 || out->bmiHeader.biBitCount == 24 || out->bmiHeader.biBitCount == 32 ) + return ICERR_OK; break; - case 24: - case 32: + case BI_BITFIELDS: + if ( out->bmiHeader.biBitCount == 16 && ICCVID_CheckMask(out->bmiColors, 0x7C00, 0x03E0, 0x001F) ) + return ICERR_OK; + if ( out->bmiHeader.biBitCount == 16 && ICCVID_CheckMask(out->bmiColors, 0xF800, 0x07E0, 0x001F) ) + return ICERR_OK; break; - default: - TRACE("unsupported output bitcount = %d\n", out->bmiHeader.biBitCount ); - return ICERR_BADFORMAT; } + TRACE("unsupported output format\n"); + return ICERR_BADFORMAT; }
return ICERR_OK;
From: Alfred Agrell floating@muncher.se
--- dlls/msvfw32/tests/msvfw.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/msvfw32/tests/msvfw.c b/dlls/msvfw32/tests/msvfw.c index b22b466f595..2a7d376917e 100644 --- a/dlls/msvfw32/tests/msvfw.c +++ b/dlls/msvfw32/tests/msvfw.c @@ -190,6 +190,13 @@ static void test_Locate(void) if (h) ok(ICClose(h) == ICERR_OK,"ICClose failed\n"); bo.biHeight = - bo.biHeight;
+ bo.biCompression = mmioFOURCC('U','Y','V','Y'); + bo.biBitCount = bi.biBitCount = 16; + h = ICLocate(ICTYPE_VIDEO, 0, &bi, &bo, ICMODE_DECOMPRESS); + ok(h == 0, "cvid->UYVY succeeded\n"); + if (h) ok(ICClose(h) == ICERR_OK,"ICClose failed\n"); + bo.biCompression = BI_RGB; + bo.biBitCount = bi.biBitCount = 32; h = ICLocate(ICTYPE_VIDEO, 0, &bi, &bo, ICMODE_DECOMPRESS); ok(h != 0, "cvid->RGB32 failed\n");
From: Alfred Agrell floating@muncher.se
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698 --- dlls/quartz/avidec.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/dlls/quartz/avidec.c b/dlls/quartz/avidec.c index f494efdf1e8..89b3e998b73 100644 --- a/dlls/quartz/avidec.c +++ b/dlls/quartz/avidec.c @@ -482,12 +482,15 @@ static HRESULT WINAPI avi_decompressor_source_qc_Notify(IQualityControl *iface, TRACE("filter %p, sender %p, type %#x, proportion %ld, late %s, timestamp %s.\n", filter, sender, q.Type, q.Proportion, debugstr_time(q.Late), debugstr_time(q.TimeStamp));
- EnterCriticalSection(&filter->filter.stream_cs); - if (q.Late > 0) - filter->late = q.Late + q.TimeStamp; - else - filter->late = -1; - LeaveCriticalSection(&filter->filter.stream_cs); + /* Submarine Titans calls qc_Notify from a foreign thread while inside sink_Receive */ + if (TryEnterCriticalSection(&filter->filter.stream_cs)) + { + if (q.Late > 0) + filter->late = q.Late + q.TimeStamp; + else + filter->late = -1; + LeaveCriticalSection(&filter->filter.stream_cs); + } return S_OK; }
From: Alfred Agrell floating@muncher.se
--- dlls/quartz/tests/avidec.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/dlls/quartz/tests/avidec.c b/dlls/quartz/tests/avidec.c index ac3a9abf2b1..78d94d95a6f 100644 --- a/dlls/quartz/tests/avidec.c +++ b/dlls/quartz/tests/avidec.c @@ -873,6 +873,20 @@ static HRESULT testsink_connect(struct strmbase_sink *iface, IPin *peer, const A return S_OK; }
+static DWORD WINAPI call_qc_notify(void *ptr) +{ + struct testfilter *filter = ptr; + IQualityControl *qc; + Quality q = { Famine, 2000, -10000000, 10000000 }; + + IPin_QueryInterface(filter->sink.pin.peer, &IID_IQualityControl, (void**)&qc); + /* don't worry too much about what it returns, just check that it doesn't deadlock */ + IQualityControl_Notify(qc, &filter->filter.IBaseFilter_iface, q); + IQualityControl_Release(qc); + + return 0; +} + static HRESULT WINAPI testsink_Receive(struct strmbase_sink *iface, IMediaSample *sample) { struct testfilter *filter = impl_from_strmbase_filter(iface->pin.filter); @@ -920,6 +934,13 @@ static HRESULT WINAPI testsink_Receive(struct strmbase_sink *iface, IMediaSample hr = IMediaSample_IsSyncPoint(sample); todo_wine_if (testmode == 5 || testmode == 6) ok(hr == S_OK, "Got hr %#lx.\n", hr);
+ if (testmode == 7) + { + HANDLE h = CreateThread(NULL, 0, call_qc_notify, filter, 0, NULL); + ok(WaitForSingleObject(h, 1000) == WAIT_OBJECT_0, "Unexpected deadlock.\n"); + CloseHandle(h); + } + return S_OK; }
@@ -1129,6 +1150,15 @@ static void test_sample_processing(IMediaControl *control, IMemInputPin *input, ok(hr == S_OK, "Got hr %#lx.\n", hr); todo_wine ok(!sink->got_sample, "Got %u calls to Receive().\n", sink->got_sample);
+ testmode = 7; + hr = IMediaSample_SetSyncPoint(sample, TRUE); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + sink->got_sample = 0; + hr = IMemInputPin_Receive(input, sample); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + ok(sink->got_sample == 1, "Got %u calls to Receive().\n", sink->got_sample); + sink->got_sample = 0; + hr = IMediaControl_Stop(control); ok(hr == S_OK, "Got hr %#lx.\n", hr);
@@ -1174,7 +1204,7 @@ static void test_streaming_events(IMediaControl *control, IPin *sink, ok(!testsink->got_eos, "Got %u calls to IPin::EndOfStream().\n", testsink->got_eos); hr = IPin_EndOfStream(sink); ok(hr == S_OK, "Got hr %#lx.\n", hr); - todo_wine ok(!testsink->got_sample, "Got %u calls to Receive().\n", testsink->got_sample); + ok(!testsink->got_sample, "Got %u calls to Receive().\n", testsink->got_sample); ok(testsink->got_eos == 1, "Got %u calls to IPin::EndOfStream().\n", testsink->got_eos); testsink->got_eos = 0;
@@ -1185,7 +1215,7 @@ static void test_streaming_events(IMediaControl *control, IPin *sink, testmode = 0; hr = IMemInputPin_Receive(input, sample); ok(hr == S_OK, "Got hr %#lx.\n", hr); - todo_wine ok(testsink->got_sample == 1, "Got %u calls to Receive().\n", testsink->got_sample); + ok(testsink->got_sample == 1, "Got %u calls to Receive().\n", testsink->got_sample); testsink->got_sample = 0;
ok(!testsink->got_begin_flush, "Got %u calls to IPin::BeginFlush().\n", testsink->got_begin_flush);
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=146276
Your paranoid android.
=== w864 (32 bit report) ===
quartz: systemclock.c:223: Test failed: Event should be signaled. systemclock.c:226: Test failed: Got hr 0.
On Thu Jun 13 19:06:57 2024 +0000, Alfred Agrell wrote:
Then let's switch commit 6 to the simplest possible implementation that satisfies the game, to minimize the amount of work to undo when implementing the correct fix.
I don't think TryEnterCriticalSection() is the right solution, no. I wanted to figure out whether avidec is supposed to do any QC at all, or if v7 was the right solution, or if I could somehow find some way to prove something more interesting about native behaviour (which I unfortunately couldn't find a way to do).
I did some testing with native, and while our implementation isn't correct (in particular, native does not use ICDECOMPRESS_HURRYUP), it does look like native will drop samples when Late is high enough.
So we do need to keep that logic, I think. I would advocate either:
* revert to v7, with the separate lock,
* just get rid of the lock and rely on TSO guaranteed by Windows.
I don't have a preference.
On Wed Jun 19 01:57:47 2024 +0000, Elizabeth Figura wrote:
I don't think TryEnterCriticalSection() is the right solution, no. I wanted to figure out whether avidec is supposed to do any QC at all, or if v7 was the right solution, or if I could somehow find some way to prove something more interesting about native behaviour (which I unfortunately couldn't find a way to do). I did some testing with native, and while our implementation isn't correct (in particular, native does not use ICDECOMPRESS_HURRYUP), it does look like native will drop samples when Late is high enough. So we do need to keep that logic, I think. I would advocate either:
- revert to v7, with the separate lock,
- just get rid of the lock and rely on TSO guaranteed by Windows.
I don't have a preference.
That variable is 64bit, accesses are not atomic if compiled as 32bit. (And even if it was 32bit, data races are UB per the C spec; even if MS promises it's safe, I don't trust MinGW, Clang and TSan to make the same promises.)
A third option would be dropping the lock before calling IMemInput_Receive, but that's difficult with strmbase's current architecture.
A fourth option would be to access that variable with Interlocked functions, but the most convenient write operation (InterlockedExchange64) doesn't exist in Wine's headers. Wouldn't be hard to add, but that's a little out of scope for this MR, isn't it?
So I'd say using that extra lock is the best option here.
-
Alfred Agrell -
Alfred Agrell (＠Alcaro)
-
Elizabeth Figura (＠zfigura)
-
Marvin