The streaming threads and GST_Seeking_SetPositions() contend for the same flushing locks. GST_Seeking_SetPositions() needs to acquire all the flushing locks before doing the actual stream seeking, having the streaming threads fighting for the same locks will make GST_Seeking_SetPositions() wait for a unusually long time.
This reduces PowerPoint 2016 video seeking time from ~5s to almost an instant and fixes a regression for Fallout 3.
From: Zhiyi Zhang zzhang@codeweavers.com
The streaming threads and GST_Seeking_SetPositions() contend for the same flushing locks. GST_Seeking_SetPositions() needs to acquire all the flushing locks before doing the actual stream seeking, having the streaming threads fighting for the same locks will make GST_Seeking_SetPositions() wait for a unusually long time.
This reduces PowerPoint 2016 video seeking time from ~5s to almost an instant and fixes a regression for Fallout 3.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53403 --- dlls/winegstreamer/quartz_parser.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 9369b529642..3f6b4e11385 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -84,6 +84,11 @@ struct parser_source bool need_segment;
bool eos; + + /* When is_seeking is true, the code is in GST_Seeking_SetPositions() and is trying to change + * positions. In this case, pause the streaming thread until seeking is completed. Otherwise, + * GST_Seeking_SetPositions() may take a long time acquiring all the flushing locks. */ + bool is_seeking; };
static inline struct parser *impl_from_strmbase_filter(struct strmbase_filter *iface) @@ -929,6 +934,9 @@ static DWORD CALLBACK stream_thread(void *arg) { struct wg_parser_buffer buffer;
+ while (pin->is_seeking) + Sleep(1); + EnterCriticalSection(&pin->flushing_cs);
if (pin->eos) @@ -1462,7 +1470,10 @@ static HRESULT WINAPI GST_Seeking_SetPositions(IMediaSeeking *iface, struct parser_source *flush_pin = filter->sources[i];
if (flush_pin->pin.pin.peer) + { + flush_pin->is_seeking = true; EnterCriticalSection(&flush_pin->flushing_cs); + } }
SourceSeekingImpl_SetPositions(iface, current, current_flags, stop, stop_flags); @@ -1494,6 +1505,7 @@ static HRESULT WINAPI GST_Seeking_SetPositions(IMediaSeeking *iface,
if (flush_pin->pin.pin.peer) { + flush_pin->is_seeking = false; LeaveCriticalSection(&flush_pin->flushing_cs); WakeConditionVariable(&flush_pin->eos_cv); }
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=125698
Your paranoid android.
=== debian11 (32 bit report) ===
ntoskrnl.exe: driver_pnp.c:737: Test failed: expected IRP_MN_REMOVE_DEVICE
- /* When is_seeking is true, the code is in GST_Seeking_SetPositions() and is trying to change
* positions. In this case, pause the streaming thread until seeking is completed. Otherwise,* GST_Seeking_SetPositions() may take a long time acquiring all the flushing locks. */
Yeah, although I don't think this describes the situation quite accurately enough.
The intent of grabbing flushing_lock *is* to pause the stream thread. It's basically just a convenient, if unconventional, use of a mutex. The problem is that this only works if mutexes are fair, in the specific sense that if thread A releases a mutex and thread B is waiting, thread B will always grab that mutex.
As it happens, win32 critical sections *are* fair in that sense, or at least testing strongly suggests that it's the case. Our current implementation violates that assumption, and there's a good argument that we should fix Wine in case other applications are broken. (Then again, if we haven't heard a report of breakage... but also, this issue would be rather difficult to debug.) On the other hand, it's not clear to me what the correct way of fixing the problem is. Putting a wait queue in the critical section works, but so does just yielding after RtlUnWaitCriticalSection, or even RtlWakeByAddressSingle or NtAlertThreadByThreadId, and I don't know how to tell which of these is the correct place to yield if any. (We don't want to make wakeups slower unless we're sure they should be...)
Despite that I think we should also fix winegstreamer not to depend on that, because it's subtle and I don't trust every mutex implementation to guarantee that (and it'd be nice if we can port winegstreamer code elsewhere, especially the high-level quartz stuff.)
I'm not sure how I feel about the Sleep(1) loop. In practice the sleep can be longer than 1 ms, in fact I think Windows has it as 16 ms. A one-time cost when seeking maybe doesn't matter though, even if you consider that seeking should be seamless.
Yeah, I don't like this approach much just from looking at it. I don't know that timing is a concern (although I'm also not sure that it's *not*), but I don't like how non-obvious the threading is here.
If I had to prescribe an approach, I'd just use a separate CV for flushing. This will require a separate lock, but we really should have one anyway to protect filter->streaming.
This merge request was closed by Zhiyi Zhang.
Fixed by fcdc76388f6b9d5abfe17bdee1d2577b62866a96
-
Marvin -
Zebediah Figura (＠zfigura) -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)