Commits on Source (17)
-
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.
a6c3e3c6 -
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.
3dd0f548 -
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>
ac7f9ce2 -
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.
8f7b3838 -
The test is reliably failing with thread sanitizer enabled, without the previous patches: ================== WARNING: ThreadSanitizer: data race (pid=242170) Write of size 8 at 0x7b6c00010088 by thread T10: #0 DecoderPlayCc ../../src/input/decoder.c:1008 (libvlccore.so.9+0xebbf5) #1 ModuleThread_QueueCc ../../src/input/decoder.c:1064 (libvlccore.so.9+0xec031) #2 decoder_QueueCc ../../include/vlc_codec.h:444 (test_src_input_decoder+0x5768) #3 decoder_decode_check_cc ../../test/src/input/decoder/input_decoder_scenarios.c:79 (test_src_input_decoder+0x5a4e) #4 DecoderDecode ../../test/src/input/decoder/input_decoder.c:90 (test_src_input_decoder+0x3dd4) #5 DecoderThread_DecodeBlock ../../src/input/decoder.c:1376 (libvlccore.so.9+0xed9f1) #6 DecoderThread_ProcessInput ../../src/input/decoder.c:1498 (libvlccore.so.9+0xee14a) #7 DecoderThread ../../src/input/decoder.c:1786 (libvlccore.so.9+0xef877) Previous read of size 1 at 0x7b6c00010088 by thread T8: #0 vlc_input_decoder_HasCCChanFlag ../../src/input/decoder.c:2445 (libvlccore.so.9+0xf2a2b) #1 vlc_input_decoder_SetCcState ../../src/input/decoder.c:2464 (libvlccore.so.9+0xf2b5f) #2 EsOutSelectEs ../../src/input/es_out.c:2446 (libvlccore.so.9+0x1095c4) #3 EsOutSelect ../../src/input/es_out.c:2686 (libvlccore.so.9+0x10a7fc) #4 EsOutVaControlLocked ../../src/input/es_out.c:3270 (libvlccore.so.9+0x10dea5) #5 EsOutControlLocked ../../src/input/es_out.c:3147 (libvlccore.so.9+0x10d0ae) #6 EsOutVaPrivControlLocked ../../src/input/es_out.c:3759 (libvlccore.so.9+0x112282) #7 EsOutPrivControl ../../src/input/es_out.c:4028 (libvlccore.so.9+0x11505c) #8 es_out_vaPrivControl ../../src/input/es_out.h:105 (libvlccore.so.9+0x125a49) #9 es_out_PrivControl ../../src/input/es_out.h:112 (libvlccore.so.9+0x125b38) #10 es_out_SetEs ../../src/input/es_out.h:124 (libvlccore.so.9+0x125c4b) #11 Control ../../src/input/input.c:2123 (libvlccore.so.9+0x130e15) #12 MainLoop ../../src/input/input.c:724 (libvlccore.so.9+0x129676) #13 Run ../../src/input/input.c:428 (libvlccore.so.9+0x127faa)
2e41d0c6 -
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.
322ea200 -
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.
2a4d648e -
The default case is supposed to be unreachable.
e5b48bc7 -
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>
7dcc370d -
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>
34a548cc -
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.
18e83e64 -
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.
91aabbf0 -
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.
29303ae4 -
f68a7291
-
There is no usage for picture_pool_Cancel anymore. The pool cancellation was a mechanism for asynchronous signalling within decoders that the decoder was being flushed and that waiting on picture could be cancelled. Since it was a concern for decoders, it was moved to the decoder handling code by ensuring the decoder has enough picture to finish decoding and reach the pf_flush function.
889548be -
The while loop will return at the first loop, or not even reach the inner of the loop if no picture is available, so transform the loop into an early return and de-indent the picture pool cloning case.
48bdd6cd -
p_owner->p_vout is supposed to be used under lock, but we already got an holding reference of the vout from cfg.vout, so use it instead.
b510622c
Showing
- include/vlc_frame.h 22 additions, 0 deletionsinclude/vlc_frame.h
- include/vlc_picture_pool.h 0 additions, 8 deletionsinclude/vlc_picture_pool.h
- src/input/decoder.c 265 additions, 242 deletionssrc/input/decoder.c
- src/misc/fifo.c 5 additions, 0 deletionssrc/misc/fifo.c
- src/misc/picture_pool.c 6 additions, 33 deletionssrc/misc/picture_pool.c
- test/Makefile.am 7 additions, 0 deletionstest/Makefile.am
- test/src/input/decoder/input_decoder.c 353 additions, 0 deletionstest/src/input/decoder/input_decoder.c
- test/src/input/decoder/input_decoder.h 48 additions, 0 deletionstest/src/input/decoder/input_decoder.h
- test/src/input/decoder/input_decoder_scenarios.c 313 additions, 0 deletionstest/src/input/decoder/input_decoder_scenarios.c
test/src/input/decoder/input_decoder.c
0 → 100644
test/src/input/decoder/input_decoder.h
0 → 100644