If we were waiting, and then exiting due to timeout, we still have to
recheck the condition protected by the condition variable/mutex in order
to get back to a consistent state. In this case, the queue was locked
with mp_dispatch_lock(), and mp_dispatch_queue_process() got to return
without waiting for unlock.
Also caused commit 8716c2e8. Probably an argument for replacing the
dispatch queue by a simple mutex.
Instead of using input_ctx for waiting, use the dispatch queue directly.
One big change is that the dispatch queue will just process commands
that come in (e.g. from client API) without returning. This should
reduce unnecessary playloop excutions (which is good since the playloop
got a bit fat from rechecking a lot of conditions every iteration).
Since this doesn't force a new playloop iteration on every access, this
has to be enforced manually in some cases.
Normal input (via terminal or VO window) still wakes up the playloop
every time, though that's not too important. It makes testing this
harder, though. If there are missing wakeup calls, it will be noticed
only when using the client API in some form.
At this point we could probably use a normal lock instead of the
dispatch queue stuff.
Instead of adding a lock_frame to the list when mp_dispatch_lock() is
called, just set a simple flag. This uses the fact that the lock is not
recursive, and can happen once per mp_dispatch_queue_process(). It
avoids the dynamic allocation, and makes error checking slightly
stricter.
Again, this is actually redundant and exists only for error-checking.
It'd actually need only a counter, because the actual locking is done by
"parking" the target thread in mp_dispatch_queue_process() and then
setting queue->idling=false. Only when mp_dispatch_unlock() sets it to
true again other work can proceed again. Document this too.
A deadlock bug was reported with the following test program:
mpv_handle *mpv = mpv_create();
mpv_set_option_string(mpv, "ytdl", "yes");
mpv_initialize(mpv);
mpv_terminate_destroy(mpv);
The cause of this is loading the ytdl.lua script, which triggers a
certain code path that calls mp_dispatch_queue_process() recursively. It
does so to wait until the script is loaded, and we want to keep that.
Reentrancy was not supported by mp_dispatch, which leads to the
deadlock. Rewrite the locking so that it does. We mainly get rid of the
"exclusive_lock" mutex. Instead we use the existing lock/condition
variable to wait until we can grab a logical lock.
Note that the lock_frame business can be replaced with a simple counter.
Instead of checking the lock_frame address, it'd simply increment and
store the counter when entering mp_dispatch_queue_process(), and then
compare the counter to decide whether or not to wait. But I think the
additional error checking done by the lock_frame list is valuable.
Fixes#3489.
Why do these API calls even still exist? I don't know, and maybe they
don't make any sense anymore. But whether they should be removed or not
is not a decision I want to make now. I want to get rid of
mp_dispatch_suspend/resume(), though. So implement the client APIs
slightly differently.
This covers source files which were added in mplayer2 and mpv times
only, and where all code is covered by LGPL relicensing agreements.
There are probably more files to which this applies, but I'm being
conservative here.
A file named ao_sdl.c exists in MPlayer too, but the mpv one is a
complete rewrite, and was added some time after the original ao_sdl.c
was removed. The same applies to vo_sdl.c, for which the SDL2 API is
radically different in addition (MPlayer supports SDL 1.2 only).
common.c contains only code written by me. But common.h is a strange
case: although it originally was named mp_common.h and exists in MPlayer
too, by now it contains only definitions written by uau and me. The
exceptions are the CONTROL_ defines - thus not changing the license of
common.h yet.
codec_tags.c contained once large tables generated from MPlayer's
codecs.conf, but all of these tables were removed.
From demux_playlist.c I'm removing a code fragment from someone who was
not asked; this probably could be done later (see commit 15dccc37).
misc.c is a bit complicated to reason about (it was split off mplayer.c
and thus contains random functions out of this file), but actually all
functions have been added post-MPlayer. Except get_relative_time(),
which was written by uau, but looks similar to 3 different versions of
something similar in each of the Unix/win32/OSX timer source files. I'm
not sure what that means in regards to copyright, so I've just moved it
into another still-GPL source file for now.
screenshot.c once had some minor parts of MPlayer's vf_screenshot.c, but
they're all gone.
There is not much of a reason to have these wrappers around. Use POSIX
standard functions directly, and use a separate utility function to take
care of the timespec calculations. (Course POSIX for using this weird
format for time values.)
Use the time as returned by mp_time_us() for mpthread_cond_timedwait(),
instead of calculating the struct timespec value based on a timeout.
This (probably) makes it easier to wait for a specific deadline.
The here documented guarantee might simplify code using this mechanism
a lot, because it becomes unnecessary to invent a separate mechanism to
make the mp_dispatch_queue_process loop exit after processing a dispatch
callback. (Instead, the dispatch callback can set a flag, and the caller
of mp_dispatch_queue_process can check it.)
The wakeup is needed to make mp_dispatch_queue_process() return if
suspension is not needed anymore - which is only the case when the
request count reaches 0.
The assertion added with this commit always has/had to be true.
Note that this mechanism is similarly "unreliable" as for example
pthread_cond_timedwait(). Trying to target the exact wait time will just
make it more complex.
The main use case for this is for threads which either use the dispatch
centrally and want mp_dispatch_queue_process to do a blocking wait for
new work, or threads which have to implement timers. For the former,
anything is fine, as long as they don't have to do active waiting for
new works. For the former, callers are better off recalculating their
deadline after every call.
This is much simpler, leaves fairness isues etc. to the operating
system, and will work better with threading-related debugging tools.
The "trick" to this is that the lock can be acquired and held only while
the queue is in suspend mode. This way we don't need to make sure the
lock is held outside of mp_dispatch_queue_process, which would be quite
messy to get right, because it would have to be in locked state by
default.
mp_dispatch_queue_process() releases the queue->lock mutex while
processing a dispatch callback. But this allowed mp_dispatch_lock() to
grab the "logical" lock represented by queue->locked. Grabbing the
logical lock is not a problem in itself, but it can't be allowed to
happen while the callback is still running.
Fix this by claiming the logical lock while the dispatch callback is
processed. Also make sure that the thread calling mp_dispatch_lock() is
woken up properly.
Fortunately, this didn't matter, because the locking function is unused.
This was part of osdep/threads.c out of laziness. But it doesn't contain
anything OS dependent. Note that the rest of threads.c actually isn't
all that OS dependent either (just some minor ifdeffery to work around
the lack of clock_gettime() on OSX).
wm4
Paweł Forysiuk