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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
