On Tue, Jul 16, 2024 at 07:11:42PM +0200, Anton Khirnov wrote:
> Frame threading in the FFV1 decoder works in a very unusual way - the
> state that needs to be propagated from the previous frame is not decoded
> pixels(¹), but each slice's entropy coder state after decoding the slice.
> 
> For that purpose, the decoder's update_thread_context() callback stores
> a pointer to the previous frame thread's private data. Then, when
> decoding each slice, the frame thread uses the standard progress
> mechanism to wait for the corresponding slice in the previous frame to
> be completed, then copies the entropy coder state from the
> previously-stored pointer.
> 
> This approach is highly dubious, as update_thread_context() should be
> the only point where frame-thread contexts come into direct contact.
> There are no guarantees that the stored pointer will be valid at all, or
> will contain any particular data after update_thread_context() finishes.
> 
> More specifically, this code can break due to the fact that keyframes
> reset entropy coder state and thus do not need to wait for the previous
> frame. As an example, consider a decoder process with 2 frame threads -
> thread 0 with its context 0, and thread 1 with context 1 - decoding a
> previous frame P, current frame F, followed by a keyframe K. Then
> consider concurrent execution consistent with the following sequence of
> events:
> * thread 0 starts decoding P
> * thread 0 reads P's slice header, then calls
>   ff_thread_finish_setup() allowing next frame thread to start
> * main thread calls update_thread_context() to transfer state from
>   context 0 to context 1; context 1 stores a pointer to context 0's private
>   data
> * thread 1 starts decoding F
> * thread 1 reads F's slice header, then calls
>   ff_thread_finish_setup() allowing the next frame thread to start
>   decoding
> * thread 0 finishes decoding P
> * thread 0 starts decoding K; since K is a keyframe, it does not
>   wait for F and reallocates the arrays holding entropy coder state
> * thread 0 finishes decoding K
> * thread 1 reads entropy coder state from its stored pointer to context
>   0, however it finds state from K rather than from P
> 
> This execution is currently prevented by special-casing FFV1 in the
> generic frame threading code, however that is supremely ugly. It also
> involves unnecessary copies of the state arrays, when in fact they can
> only be used by one thread at a time.
> 
> This commit addresses these deficiencies by changing the array of
> PlaneContext (each of which contains the allocated state arrays)
> embedded in FFV1SliceContext into a RefStruct object. This object can
> then be propagated across frame threads in standard manner. Since the
> code structure guarantees only one thread accesses it at a time, no
> copies are necessary. It is also re-created for keyframes, solving the
> above issue cleanly.
> 
> Special-casing of FFV1 in the generic frame threading code will be
> removed in a later commit.
> 
> (¹) except in the case of a damaged slice, when previous frame's pixels
>     are used directly
> ---
>  libavcodec/ffv1.c    | 30 ++++++++++++++++++++++++------
>  libavcodec/ffv1.h    |  4 +++-
>  libavcodec/ffv1dec.c | 33 +++++++++------------------------
>  3 files changed, 36 insertions(+), 31 deletions(-)

LGTM

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.