Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56698
-- v6: 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: 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/wg_parser.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 431a8b74cb2..35e155224ba 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -117,6 +117,7 @@ struct wg_parser_stream
bool flushing, eos, enabled, has_tags, has_buffer, no_more_pads;
+ uint64_t prev_presentation_end; uint64_t duration; gchar *tags[WG_PARSER_TAG_COUNT]; }; @@ -371,6 +372,14 @@ static NTSTATUS wg_parser_stream_get_buffer(void *args) wg_buffer->pts = GST_BUFFER_PTS(buffer) / 100; if ((wg_buffer->has_duration = GST_BUFFER_DURATION_IS_VALID(buffer))) wg_buffer->duration = GST_BUFFER_DURATION(buffer) / 100; + if (wg_buffer->has_duration && !wg_buffer->has_pts && stream->prev_presentation_end) + { + wg_buffer->has_pts = true; + wg_buffer->pts = stream->prev_presentation_end; + stream->prev_presentation_end = wg_buffer->pts + wg_buffer->duration; + } + if (wg_buffer->has_duration && wg_buffer->has_pts) + stream->prev_presentation_end = wg_buffer->pts + wg_buffer->duration; wg_buffer->discontinuity = GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DISCONT); wg_buffer->preroll = GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_LIVE); wg_buffer->delta = GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT);
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 | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/dlls/quartz/avidec.c b/dlls/quartz/avidec.c index f494efdf1e8..847fb77caff 100644 --- a/dlls/quartz/avidec.c +++ b/dlls/quartz/avidec.c @@ -48,6 +48,8 @@ struct avi_decompressor
HIC hvid; BITMAPINFOHEADER* pBihIn; + + CRITICAL_SECTION late_cs; REFERENCE_TIME late; };
@@ -77,7 +79,9 @@ static HRESULT avi_decompressor_sink_query_accept(struct strmbase_pin *iface, co static HRESULT avi_decompressor_sink_end_flush(struct strmbase_sink *iface) { struct avi_decompressor *filter = impl_from_strmbase_filter(iface->pin.filter); + EnterCriticalSection(&filter->late_cs); filter->late = -1; + LeaveCriticalSection(&filter->late_cs); if (filter->source.pin.peer) return IPin_EndFlush(filter->source.pin.peer); return S_OK; @@ -167,8 +171,10 @@ static HRESULT WINAPI avi_decompressor_sink_Receive(struct strmbase_sink *iface, if (IMediaSample_IsSyncPoint(pSample) != S_OK) flags |= ICDECOMPRESS_NOTKEYFRAME; hr = IMediaSample_GetTime(pSample, &tStart, &tStop); + EnterCriticalSection(&This->late_cs); if (hr == S_OK && AVIDec_DropSample(This, tStart)) flags |= ICDECOMPRESS_HURRYUP; + LeaveCriticalSection(&This->late_cs);
res = ICDecompress(This->hvid, flags, This->pBihIn, pbSrcStream, &source_format->bmiHeader, pbDstStream); if (res != ICERR_OK) @@ -482,12 +488,12 @@ 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); + EnterCriticalSection(&filter->late_cs); if (q.Late > 0) filter->late = q.Late + q.TimeStamp; else filter->late = -1; - LeaveCriticalSection(&filter->filter.stream_cs); + LeaveCriticalSection(&filter->late_cs); return S_OK; }
@@ -532,6 +538,9 @@ static void avi_decompressor_destroy(struct strmbase_filter *iface) IPin_Disconnect(filter->source.pin.peer); IPin_Disconnect(&filter->source.pin.IPin_iface);
+ filter->late_cs.DebugInfo->Spare[0] = 0; + DeleteCriticalSection(&filter->late_cs); + strmbase_sink_cleanup(&filter->sink); strmbase_source_cleanup(&filter->source); strmbase_passthrough_cleanup(&filter->passthrough); @@ -550,7 +559,9 @@ static HRESULT avi_decompressor_init_stream(struct strmbase_filter *iface) if (!filter->source.pin.peer) return S_OK;
+ EnterCriticalSection(&filter->late_cs); filter->late = -1; + LeaveCriticalSection(&filter->late_cs);
source_format = (VIDEOINFOHEADER *)filter->source.pin.mt.pbFormat; if ((res = ICDecompressBegin(filter->hvid, filter->pBihIn, &source_format->bmiHeader))) @@ -618,6 +629,9 @@ HRESULT avi_dec_create(IUnknown *outer, IUnknown **out) ISeekingPassThru_Init(&object->passthrough.ISeekingPassThru_iface, FALSE, &object->sink.pin.IPin_iface);
+ InitializeCriticalSectionEx(&object->late_cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO); + object->late_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": object.late_cs"); + TRACE("Created AVI decompressor %p.\n", object); *out = &object->filter.IUnknown_inner;
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=145991
Your paranoid android.
=== w11pro64 (32 bit report) ===
quartz: mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:945: Test failed: Got stop time 56d3, expected 56ce. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0. mpegvideo.c:942: Test failed: Got start time a, expected 0. mpegvideo.c:945: Test failed: Got stop time f, expected 0.
=== debian11 (32 bit ar:MA report) ===
Report validation errors: quartz:filtergraph crashed (c0000005)
The 2/6 problem is that GStreamer doesn't give us any PTS for Cinepak videos. With decodebin_parser, the Cinepak decoder's GstVideoDecoder base class takes the DTS, and previous PTS, and fills in missing PTS; I couldn't decipher the exact formula, so I just picked something.
If PTS plus duration feels better to you, then so be it. For some reason, the first frame does have a PTS.
avidemux sets PTS on keyframes, so that's an explanation.
Technically DTS is not supposed to mean PTS. They're supposed to increase at the same rate but they may not be equal. DirectShow doesn't directly have the concept of the DTS [note that SetTime() vs SetMediaTime() is not this]. GstVideoDecoder will supply PTS from DTS using this assumption, or supply PTS from the previous PTS plus duration.
I'm not sure that *just* using DTS is safe either, exactly. Ideally we should just omit PTS if it isn't present. The MPEG-1 splitter doesn't always output PTS. The AVI splitter does, and if some application depends on that then I'd prefer to use AVI-specific code.
The 6/6 deadlock consists of the AVI decoder calling IMemInputPin_Receive, which, for some reason, calls into another thread that calls qc_Notify. It works in decodebin_parser seemingly by accident; that function doesn't take any PE-side locks, it just calls into Unix code directly (and Unix-side locks are, of course, not held while calling IMemInputPin), so I just mirrored that.
If you can think of a better solution, do tell. Release the lock before calling IMemInputPin, maybe? But the release is higher up the call stack, so there's no obvious way to do that. Release lock, call IMemInputPin, and re-acquire lock just so strmbase/pin.c can release it?
I don't have my full notes unfortunately, but according to my limited notes I actually tested with a native filter [not sure which], and blocking in Notify() and calling Receive() does block, so it sounds like the deadlock you're describing is a real deadlock on Windows. What component is calling Notify() from Receive() and doing it from a separate thread? Is it a Wine component? Why is it using a separate thread if the call is apparently synchronous anyway?
Perhaps the filter I tested with wasn't the AVI decoder, and the AVI decoder really is immune to this deadlock. Further speculation is that our QC logic in the AVI decoder is not what native does and should just be deleted. I won't ask you to go through testing that, but I would like to know why we're calling back from a separate thread that Receive() is blocking on anyway.
On Wed Jun 5 04:22:45 2024 +0000, Elizabeth Figura wrote:
The 2/6 problem is that GStreamer doesn't give us any PTS for Cinepak
videos. With decodebin_parser, the Cinepak decoder's GstVideoDecoder base class takes the DTS, and previous PTS, and fills in missing PTS; I couldn't decipher the exact formula, so I just picked something.
If PTS plus duration feels better to you, then so be it. For some
reason, the first frame does have a PTS. avidemux sets PTS on keyframes, so that's an explanation. Technically DTS is not supposed to mean PTS. They're supposed to increase at the same rate but they may not be equal. DirectShow doesn't directly have the concept of the DTS [note that SetTime() vs SetMediaTime() is not this]. GstVideoDecoder will supply PTS from DTS using this assumption, or supply PTS from the previous PTS plus duration. I'm not sure that *just* using DTS is safe either, exactly. Ideally we should just omit PTS if it isn't present. The MPEG-1 splitter doesn't always output PTS. The AVI splitter does, and if some application depends on that then I'd prefer to use AVI-specific code.
The 6/6 deadlock consists of the AVI decoder calling
IMemInputPin_Receive, which, for some reason, calls into another thread that calls qc_Notify. It works in decodebin_parser seemingly by accident; that function doesn't take any PE-side locks, it just calls into Unix code directly (and Unix-side locks are, of course, not held while calling IMemInputPin), so I just mirrored that.
If you can think of a better solution, do tell. Release the lock
before calling IMemInputPin, maybe? But the release is higher up the call stack, so there's no obvious way to do that. Release lock, call IMemInputPin, and re-acquire lock just so strmbase/pin.c can release it? I don't have my full notes unfortunately, but according to my limited notes I actually tested with a native filter [not sure which], and blocking in Notify() and calling Receive() does block, so it sounds like the deadlock you're describing is a real deadlock on Windows. What component is calling Notify() from Receive() and doing it from a separate thread? Is it a Wine component? Why is it using a separate thread if the call is apparently synchronous anyway? Perhaps the filter I tested with wasn't the AVI decoder, and the AVI decoder really is immune to this deadlock. Further speculation is that our QC logic in the AVI decoder is not what native does and should just be deleted. I won't ask you to go through testing that, but I would like to know why we're calling back from a separate thread that Receive() is blocking on anyway.
2/6: I already changed it to ignore DTS. Yeah, I checked media time a little, and it seems to just be a frame counter (at least for Cinepak AVIs).
Make it AVI specific and put it closer to the part that needs it, sure, can do. Weird that GStreamer puts that piece in the decoder, not the demuxer, but it works for them, so that's not their problem.
6/6: The offending filter is named DDX In Place, and is implemented somewhere in the game (that string shows up in strings -el st.exe). My guess would be that the game shoves everything onto the main thread; it was released in august 2000, which is around when threading started to become relevant, so I guess the devs didn't feel confident on their threading skills and just threw the bluntest solution they could find at the problem.
Googling DDX In Place (or DDX Video Renderer, another string in the exe) returns nothing relevant. Probably some third-party lib whose vendor has dropped off the internet.
I don't know what that quality control stuff is supposed to accomplish or how it's supposed to be implemented; all I know is that this call sequence does not deadlock on native. I'll add a test, if that helps. I should've done that earlier; this is indeed a strange behavior, and strange behaviors need tests.
If you don't like the new mutex, another option would be to use TryEnterCriticalSection instead, and just ignore the call if locked. This means a few of those notifications can be lost, but that's probably no big deal, they're fundamentally timing-bound anyways.
-
Alfred Agrell -
Alfred Agrell (＠Alcaro)
-
Elizabeth Figura (＠zfigura)
-
Marvin