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