[FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy

[David Bryant]

On 3/28/20 6:23 AM, Anton Khirnov wrote:
> Quoting David Bryant (2020-03-27 23:51:19)
>> On 3/27/20 5:57 AM, Anton Khirnov wrote:
>>> The current design, where
>>> - proper init is called for the first per-thread context
>>> - first thread's private data is copied into private data for all the
>>>   other threads
>>> - a "fixup" function is called for all the other threads to e.g.
>>>   allocate dynamically allocated data
>>> is very fragile and hard to follow, so it is abandoned. Instead, the
>>> same init function is used to init each per-thread context. Where
>>> necessary, AVCodecInternal.is_copy can be used to differentiate between
>>> the first thread and the other ones (e.g. for decoding the extradata
>>> just once).
>>> ---
>> I'm not sure I fully understand this change. You mention that AVCodecInternal.is_copy can be used where different treatment
>> might be necessary for subsequent threads, and I see that done in a couple places, but in most cases you have simply deleted the
>> init_thread_copy() function even when it's not at all obvious (to me anyway) that that won't break the codec.
> In most cases, just deleting init_thread_copy() is the right thing to
> do. E.g. all decoders that do not implement update_thread_context() have
> to be intra-only, so every frame thread is effectively a completely
> standalone decoder. And in most of the more complex decoders (like h264)
> the important parameters are dynamically changeable during decoding, so
> not that much is done in decoder init beyond allocating some stuff that
> does not depend on the bistream properties.
>
> My intent is for each frame-thread worker to be treated inasmuch as
> possible as a standalone decoder, and where it has to share data with
> other threads to make this sharing explicit (rather than implicit as is
> the case now).

Yes, this makes sense. The confusing part is when the decode_init() function looks completely different than the
init_thread_copy() function. This is often because the decode_init() function is generating things (tables, etc.) from scratch
and the init_thread_copy() is just copying the necessary things from the original. In cases where the original generation might
be time consuming this might make sense, but usually it's probably just making the code more complex and difficult to follow
(which I believe was your original point).

One possible interim solution for complex cases that break would be to leave the init_thread_copy() function there, but instead
of having it in the AVCodec struct and called from outside (which is no longer possible), it simply gets called first thing from
decode_init() if AVCodecInternal.is_copy is set. That way the architecture is cleaned up now, and the codec won't break and can
be cleaned up when time permits. Just a thought.

>
>> Specifically this will break WavPack because now it will be allocating a new dsdctx for every thread instead of sharing one
>> between all threads. I am happy to fix and test this case once the patch goes in, but is the intent of this patch that the
>> respective authors will find and fix all the breakages? Or did I just happen to catch the one case you missed?
> I certainly intended to convert the decoders correctly myself,
> apparently I didn't pay enough attention to the recent wavpack changes.
> Hopefully the other conversions are correct, but I will go through the
> changes again to check.
>
> Looking at wavpack, the code looks suspicious to me. You allocate one
> dsdctx per channel at init, but AFAIU the number of channels can change
> at each frame.
>
Multichannel WavPack consists of sequences of stereo or mono WavPack blocks that include flags indicating whether they are the
beginning or end of a "frame" or "packet". This allows the number of channels available to be essentially unlimited, however the
total number of channels in a stream may not change. When decoding, if a sequence of blocks generates more or less than the
correct number of channels, this is flagged as an error and overrun is prevented (see libavcodec/wavpack.c, line 1499 and line
1621).

If you are curious, the WavPack format is described in detail here:

https://github.com/dbry/WavPack/blob/master/doc/WavPack5FileFormat.pdf

Thanks!

-David