[vlc-devel] [PATCH] decoder: flush the vout before flushing the decoder

[Rémi Denis-Courmont]

On Monday 30 November 2015 18:53:56 Thomas Guillem wrote:
> > > > If alternatively the decoder gets stuck flushing, then either the
> > > > decoder plug-in:
> > > > - has a bug,
> > > > - needs to cancel pending picture allocations *within* pf_flush, or
> > > 
> > > Or add decoder_abortPicture called from pf_flush to cancel thread
> > > waiting for a picture as I said in a previous mail.
> > 
> > This approach is not viable because it is racy and worker threads could
> > get
> > stuck again between step 1 and 2 retrying to allocate:
> > 1) force all already pending picture allocations to fail,
> > 2) flush.
> > (An opportunistic vout_Flush() has essentially the same supposedly fatal
> > limitation.)
> > 
> > This other approach is also dangerous because it can cause a live loop in
> > worker threads:
> > 1) force all picture allocations to fail until further notice,
> > 2) flush,
> > 3) undo step 1.
> 
> That's the idea I had in mind. (do we agree, that this approach use
> decoder_abortPicture(true/false) ?)
> A live loop ? But it will be ended when 2/flush is completed, no ?

Why would it not loop? What do you expect from a decoder worker thread upon 
picture allocation failure? Immediate retry... Dropping of all pending data, 
marking of a fatal error and refusal to decode anything ever again... Or 
anything between those two extremes?

If the worker thread just goes to sleep until *new* data is queued for 
decoding, then this works great. But there are no reasons why it would be that 
way: That approach only makes sense when flushing. Yet at that point the 
picture allocation failed, but flushing has not even started.

Far more likely, the decoder either retries allocation - leading to a real 
live loop - or discards some unspecified amount of coded data and tries to 
allocate again - leading to a transient live loop.

> > decoder_abortPicture() could be useful for a specific class of threaded
> > decoder, that would support this pattern where cancellation and flushing
> > is
> > atomic:
> > 1) start flushing in non-blocking manner,
> > 2) cancel pending picture allocation on worker threads,
> > 3) finish flushing.
> > 
> > Yet even then, a threaded decoder implementation could flush while some
> > or all
> > worker threads are blocked in allocation step. This is really
> > implementation-
> > dependent.
> > 
> > So overall I believe that decoder_abortPicture() would only be sufficient
> > *and* necessary under constrained assumptions. Notably, I believe that it
> > would not apply well to avcodec-MT, and I don´t know any other threaded
> > decoder - maybe Android MediaCodec and/or OMX-IL?
> 
> Why would it not apply to avcodec-MT ?

Does avcodec_flush_buffers() not wait for pending get_buffer() to return?
- If it does, then I don´t see how this can apply to avcodec-MT.
- If does not, then I don´t understand the purpose of the patch in the OP.

-- 
Rémi Denis-Courmont
http://www.remlab.net/