The test check that the stream output is correctly flushed when the
input decoder is flushed, ensuring that previous commit didn't break
the triggering of the flush.
This test provides a non-regression test for the issue #26915, in which
a decoder with internal pool gets stuck when flush is not done correctly
and which has been fixed right before.
The i_preroll_end state is reset at the end of the flush call on the
es_out side, but it's only set from the decoder thread which would be
flushing the decoders anyway, so it can be set from there (ie. the last
point of flush) where it's already locked.
Flush was mostly entirely executed from the decoder thread, which meant
that it was blocked by previous call from decoder_t::pf_decode. Since
the decoder_t::pf_decode could be blocked waiting for a picture from the
vout to be able to resume decoding, and that flushing the video output
was done in this thread, it could deadlock.
A previous mechanism, picture_pool_Cancel, was introduced to allow the
external code to cancel the picture pool from the core and ensure
pf_decode would stop as soon as possible, but it was leading to more
spurrious unhandleable errors and could not work with decoder owning
their own pool.
Instead, ensure we set the flushing state directly from the flush call,
and flush the outputs. Setting the state will prevent decoder from
queueing new pictures by discarding them directly, which means that we
don't need to flush after flush has happened, and the flushing state
will be reset before new frames are queued into the decoder.
Fixes#26915
Co-authored-by: Thomas Guillem <thomas@gllm.f>
The input decoder component is made of three different states:
- Lock A:
The input_decoder itself, loading the decoder and protected against
concurrent usage of the decoder through p_owner->lock.
- Lock B:
The decoder implementation, that might create internal lock or
synchronization object to process decoding asynchronously.
- Lock C:
The decoder implementation output, or the "owner" part viewed by the
decoder implementation, which needs to be protected against access
from the decoder and access from the input_decoder and is protected
through the fifo lock, also protecting the fifo in which input data
is pushed.
Because the decoder implementation is protected by the p_owner->lock (or
here, lock A), we can never lock A from the decoder implementation,
which is what the previous code was doing.
Likewise, since the decoder implementation will use the output to queue
picture and signal state changes, it must take Lock C and thus the
output part can never take either A or B.
Instead, we enforce the order:
Lock A -> Lock B -> Lock C:
Which means that:
- The decoder implementation can lock the input decoder output (C) to
push new changes.
- The input_decoder can lock its output (C) directly to affect what
state the decoder implementation will see when queueing changes.
- The input_decoder will only lock A to protect the decoder_t object
from being used concurrently.
This fixes deadlock in specific conditions where an asynchronous decoder
implementation would queue a picture with Lock B taken, trying to lock A
while the input_decoder client would have locked A already and would try
to flush the decoder implementation, taking lock B at the same time.
In practice, lock A is not really "useful" given that most of the
decoder_t methods are called from the decoder thread (owned by the input
decoder implementation), lock B is hidden in this part of the code, and
most of the "input decoder" is protected by the fifo lock, which then
must not be taken when calling decoder_t function to avoid reentrance of
the lock.
Co-authored-by: Thomas Guillem <thomas@gllm.f>
The substream handling was under a lot of precondition, leading to a lot
of indentation. Moving this handling into a separate function allows
early return within the function, which reduce to a single indentation
level and greatly simplify the reading of the function while simplifying
the caller site where it was only a specific case to handle.
vlc_fifo_t are coming with their own lock which is exposed on the public
interface, so provide sanitization state check functions like those
available for vlc_mutex_t.
vlc_input_decoder_HasCCChanFlag also needs to be protected, and the
owner->lock mutex will be removed in later patches, to be completely
replaced by the already existing fifo lock.
The new cc.lock allows better thread safety without risking a deadlock
between the super-decoder and the sub-decoder, by protecting the very
state being synced by both of them.
Sub-decoders are decoders whose state is defined by previous decoders.
Specifically in the current case, picture frames with specific SEI data
can convey subtitles as closed captions and report them from the decoder
implementation itself.
Since the core never locks the implementation, and the implementation
can be asynchronously reporting subtitles, a common lock need to be
setup so that the core can then check and use the sub-decoders being
created without racing against the upper decoder implementation.
Co-authored-by: François Cartegnie <fcvlcdev@free.fr>
Separate the early return to make it clear that what follows require a
valid closed-caption decoder.
Then, use vlc_input_decoder_Flush directly, because pf_flush must be
called from the CC decoder thread, and the fifo must be flushed
beforehand, instead of calling it directly.
The test provides an infrastructure based on previous transcode tests
and video output tests, that will serve as basis for other
input_decoder.c tests.
Just like we do with autotools builds, we configure the target every time we
build a contrib.
In the case of cmake, touching `toolchain.cmake` does rebuild the cmake
contribs but if there's a cache it's not reconfigured and doesn't rebuild
anything.
lrint uses libm which is set in CMAKE_REQUIRED_LIBRARIES. But
check_function_exists() won't use it with cmake older than 3.12.
This removes the need for Wno-dev.
It's used to set the library prefix between glew (dynamic) and glew_s (static).
But only the static target (glew_s) is selected in our environment. It will
pick the proper prefix on its own. It's not even a public option to set.
This is required when building for linux, and might cause a system
provided version to be used, causing conflicts during linking.
Typically, the error message at link time will look like this:
/usr/bin/ld: .libs/libqt_plugin.so: version node not found for symbol qt_version_tag@Qt_5.10
Co-authored-by: Alexandre Janniaux <ajanni@videolabs.io>
They are currently enabled only when the matching contrib has been found
on the system. The plugin name doesn't match with the plugin from the
contrib, but still enforce at link time the plugin to be present.
This always use the XdgShell integration regardless of what is supported
for now.
They are currently enabled only when the matching contrib has been found
on the system. The plugin name doesn't match with the plugin from the
contrib, but still enforce at link time the plugin to be present.
Alexandre Janniaux
Johannes Kauffmann
Steve Lhomme
Hugo Beauzée-Luyssen