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

[Anton Khirnov]

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).

> 
> 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.

-- 
Anton Khirnov