Currently, ff_frame_thread_free() uses the last worker thread
to updates the first worker thread via update_context_from_thread()
immediately before freeing all these worker threads. This is
a remnant of the time in which the first worker was special.
(E.g. the first worker shared its AVCodecInternal with the public
AVCodecContext.)
But these times are over (none of the uses of is_copy matter
for ff_frame_thread_free()); nowadays the only thing that
update_context_from_thread() does is referencing a few
buffers/frames and replacing them with other references instead.
These new references will then be freed immediately thereafter
when the first worker thread is freed. Ensuring that the code is
free of double-frees is achieved by using reference-counted structures
(or in case of AVChannelLayouts: by giving each worker its own copy).
Some archaeology:
a) Updating the first worker thread from the last one used
has been done since frame-threading was added in
37b00b47cb.
b) The precursor to ff_mpv_common_end() checked for is_copy
before freeing pictures (i.e. it only freed them for the first
worker thread).
c) Commits c2dfb1e37c and
e33811bd26 modified the
update_thread_context function of the H.264 decoder
so that it could fail before calling ff_mpeg_update_thread_context().
d) This led to a double free/an assert violation with a H.264
sample for which ff_mpeg_update_thread_context() is not reached
for the final update_context_from_thread(). Commit
a6e4796fbf added code to fix this
sample.
e) This issue was fixed (even with the last mentioned commit reverted)
when the H.264 decoder was deMpegEncContextized in commit
b7fe35c9e5 (merging commit
2c54155407).
f) mpegvideo.c stopped using is_copy when it was switched to refcounted
frames in 759001c534.
g) 1f4cf92cfb removed the init_thread_copy
callbacks; now no FFCodec.close callback checks for is_copy at all
any more.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
This is possible, because every given FFCodec has to implement
exactly one of these. Doing so decreases sizeof(FFCodec) and
therefore decreases the size of the binary.
Notice that in case of position-independent code the decrease
is in .data.rel.ro, so that this translates to decreased
memory consumption.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Up until now, codec.h contains both public and private parts
of AVCodec. This exposes the internals of AVCodec to users
and leads them into the temptation of actually using them
and forces us to forward-declare structures and types that
users can't use at all.
This commit changes this by adding a new structure FFCodec to
codec_internal.h that extends AVCodec, i.e. contains the public
AVCodec as first member; the private fields of AVCodec are moved
to this structure, leaving codec.h clean.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Also move FF_CODEC_TAGS_END as well as struct AVCodecDefault.
This reduces the amount of files that have to include internal.h
(which comes with quite a lot of indirect inclusions), as e.g.
most encoders don't need it. It is furthemore in preparation
for moving the private part of AVCodec out of the public codec.h.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
This avoids including version.h in all source files, avoiding
unnecessary rebuilds when the version number is bumped. Only
version_major.h is included by the main header, which defines
availability of e.g. FF_API_* macros, and which is bumped much
less often.
This isn't done for libavutil/version.h, because that header needs
to be included essentially everywhere due to LIBAVUTIL_VERSION_INT
being used wherever an AVClass is constructed.
Signed-off-by: Martin Storsjö <martin@martin.st>
Since the request_channel_layout is used only by a handful of codecs,
move the option to codec private contexts.
Signed-off-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: James Almer <jamrial@gmail.com>
If a frame-threaded decoder with inter-frame dependencies
returns an error when decoding a frame and the returned frame
isn't clean, an error message is emitted claiming that this
is a bug. This seems to be based upon the thinking that
in this case a ThreadFrame has not been properly unreferenced.
Yet this is wrong, as decoders with inter-frame dependencies
don't use the frame for output for synchronization and therefore
don't use ThreadFrames at all for this. So unreferencing
this frame generically is fine and not a bug.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Use ff_thread_release_buffer() instead of av_frame_unref(),
as the former handles the case of non-thread-safe callbacks
properly. (This is possible now that ff_thread_release_buffer()
no longer requires a ThreadFrame.)
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
The majority of frame-threaded decoders (mainly the intra-only)
need exactly one part of ThreadFrame: The AVFrame. They don't
need the owners nor the progress, yet they had to use it because
ff_thread_(get|release)_buffer() requires it.
This commit changes this and makes these functions work with ordinary
AVFrames; the decoders that need the extra fields for progress
use ff_thread_(get|release)_ext_buffer() which work exactly
as ff_thread_(get|release)_buffer() used to do.
This also avoids some unnecessary allocations of progress AVBuffers,
namely for H.264 and HEVC film grain frames: These frames are not
used for synchronization and therefore don't need a ThreadFrame.
Also move the ThreadFrame structure as well as ff_thread_ref_frame()
to threadframe.h, the header for frame-threaded decoders with
inter-frame dependencies.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
These will be used by the codecs that need allocated progress
and is in preparation for no longer using ThreadFrame by the codecs
that don't.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
This is in preparation for further commits that will stop
using ThreadFrame for frame-threaded codecs that don't use
ff_thread_(await|report)_progress(); the API for those codecs
having inter-frame depdendencies will live in threadframe.h.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Several of our decoders support both frame- as well as slice-threading;
in case of the latter avctx->internal->thread_ctx points to
a SliceThreadContext, not to a frame-thread PerThreadContext.
So only treat avctx->internal->thread_ctx as the latter after
having checked that frame-threading is active.
Reviewed-by: Anton Khirnov <anton@khirnov.net>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
'*src' and '*avctx' point to the same memory. It is enough to keep one of them.
Signed-off-by: Yu Yang <yuyang14@kuaishou.com>
Signed-off-by: Anton Khirnov <anton@khirnov.net>
We have more mutexes/condition variables whose initialization is
unchecked.
Also use a proper namespace for these functions.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Some frame threaded decoders set it, but this information never reached the
caller in frame threading scenarios.
Signed-off-by: James Almer <jamrial@gmail.com>
Right now all AVCodecContexts except those using frame-threaded decoding
call the codec's init function and expect its close function to be
called. In order to make sure that the close function is not called for
frame-threaded decoding ff_frame_thread_free() resets
AVCodecContext.codec (and because of this it has to free the private
AVOptions of the main AVCodecContext itself). This is not obvious and
potentially fragile. Instead add a field to AVCodecInternal that
indicates whether close should be called for this AVCodecContext.
It is always zero when using frame-threaded decoding, so that resetting
the codec is no longer necessary and has been removed.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Deprecated in 40cf1bbacc.
(The currently disabled filter vf_mcdeint and vf_uspp were users of
this field; they have not been changed, so that whoever wants to fix
them can see the state of these filters when they were disabled.)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Signed-off-by: James Almer <jamrial@gmail.com>
Up until now, initializing the mutexes/condition variables wasn't
checked by ff_frame_thread_init(). This commit changes this.
Given that it is not documented to be save to destroy a zeroed but
otherwise uninitialized mutex/condition variable, one has to choose
between two approaches: Either one duplicates the code to free them
in ff_frame_thread_init() in case of errors or one records which have
been successfully initialized. This commit takes the latter approach:
For each of the two structures with mutexes/condition variables
an array containing the offsets of the members to initialize is added.
Said array is used both for initializing and freeing and the only thing
that needs to be recorded is how many of these have been successfully
initialized.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
In case an error happened when setting up the child threads,
ff_frame_thread_init() would up until now call ff_frame_thread_free()
to clean up all threads set up so far, including the current, not
properly initialized one.
But a half-allocated context needs special handling which
ff_frame_thread_frame_free() doesn't provide.
Notably, if allocating the AVCodecInternal, the codec's private data
or setting the options fails, the codec's close function will be
called (if there is one); it will also be called if the codec's init
function fails, regardless of whether the FF_CODEC_CAP_INIT_CLEANUP
is set. This is not supported by all codecs; in ticket #9099 it led
to a crash.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
They add considerable complexity to frame-threading implementation,
which includes an unavoidably leaking error path, while the advantages
of this option to the users are highly dubious.
It should be always possible and desirable for the callers to make their
get_buffer2() implementation thread-safe, so deprecate this option.
Currently the next thread's context is updated from the previous one's
if the codec descriptor is not marked as intra-only. That is not
entirely correct, since that property does not necessarily imply
anything about how a specific decoder implementation behaves.
Instead, use the presence of the update_thread_context() callback to
decide whether an update should be performed. Fixes races in CFHD,
should cause no behaviour change in any other decoders.
It is a constant known at codec init, so set it in
ff_frame_thread_init(). Also, only set it for video, since the meaning
of this field is not well-defined for audio with frame threading.
Fixes availability of delay in callbacks invoked from the per-thread
contexts after 1f4cf92cfb.
Currently the frame pool used by the default get_buffer2()
implementation is a single struct, allocated when opening the decoder.
A pointer to it is simply copied to each frame thread and we assume that
no thread attempts to modify it at an unexpected time. This is rather
fragile and potentially dangerous.
With this commit, the frame pool is made refcounted, with the reference
being propagated across threads along with other context variables. The
frame pool is now also immutable - when the stream parameters change we
drop the old reference and create a new one.
Specifically, between the user-facing one and the first frame thread
one.
This is fragile and dangerous, allocate separate private data for each
per-thread context.
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).
Resolution/format changes lead to re-initialization of hardware
accelerations(vaapi/dxva2/..) with new hwaccel_priv_data in
the worker-thread. But hwaccel_priv_data in user context won't
be updated until the resolution changing frame is output.
A termination with "-vframes" just after the reinit will lead to:
1. memory leak in worker-thread.
2. double free in user-thread.
Update user context in ff_frame_thread_free with the last thread
submit_packet() was called on.
To reproduce:
ffmpeg -hwaccel vaapi(dxva2) -v verbose -i
fate-suite/h264/reinit-large_420_8-to-small_420_8.h264 -pix_fmt nv12
-f rawvideo -vsync passthrough -vframes 47 -y out.yuv
Signed-off-by: Linjie Fu <linjie.fu@intel.com>
Signed-off-by: Anton Khirnov <anton@khirnov.net>
This removes the XP compatibility code, and switches entirely to SRW
locks, which are available starting at Windows Vista.
This removes CRITICAL_SECTION use, which allows us to add
PTHREAD_MUTEX_INITIALIZER, which will be useful later.
Windows XP is hereby not a supported build target anymore.
Signed-off-by: Diego Biurrun <diego@biurrun.de>
And remove the function altogether while at it. It's a duplicate of
another.
Reviewed-by: wm4 <nfxjfg@googlemail.com>
Signed-off-by: James Almer <jamrial@gmail.com>
This removes the XP compatibility code, and switches entirely to SWR
locks, which are available starting at Windows Vista.
This removes CRITICAL_SECTION use, which allows us to add
PTHREAD_MUTEX_INITIALIZER, which will be useful later.
Windows XP is hereby not a supported build target anymore. It was
decided in a project vote that this is OK.
The patch does not fix the tsan warning it was intended to fix.
Reverting the patch moves the av_log() back to the outside of the lock.
Signed-off-by: Wan-Teh Chang <wtc@google.com>
Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
Add the debug_threads boolean field to PerThreadContext. For
PerThreadContext *p, p->debug_threads records whether the
FF_DEBUG_THREADS bit is set in p->avctx->debug, and p->debug_threads and
p->avctx->debug are kept in sync. The debug_threads field is defined as
an atomic_int to allow atomic read by another thread in
ff_thread_await_progress().
This fixes the tsan warning that
2e664b9c1e attempted to fix:
WARNING: ThreadSanitizer: data race (pid=452658)
Write of size 4 at 0x7b640003f4fc by main thread (mutexes: write M248499):
#0 update_context_from_user [..]/libavcodec/pthread_frame.c:335:19 (5ab42bb1a6f4b068d7863dabe9b2bacc+0xe73859)
[..]
Previous read of size 4 at 0x7b640003f4fc by thread T130 (mutexes: write M248502, write M248500):
#0 ff_thread_await_progress [..]/libavcodec/pthread_frame.c:591:26 (5ab42bb1a6f4b068d7863dabe9b2bacc+0xe749a1)
Signed-off-by: Wan-Teh Chang <wtc@google.com>
Signed-off-by: Ronald S. Bultje <rsbultje@gmail.com>
So, all frames and errors are correctly reported in order.
Also limit the numbers of error during draining to prevent infinite loop.
This fix fate failure with THREADS>=4:
make fate-h264-attachment-631 THREADS=4
This also reverts a755b725ec.
Suggested-by: wm4, Ronald S. Bultje, Marton Balint
Reviewed-by: w4 <nfxjfg@googlemail.com>
Reviewed-by: Ronald S. Bultje <rsbultje@gmail.com>
Reviewed-by: Michael Niedermayer <michael@niedermayer.cc>
Signed-off-by: Muhammad Faiz <mfcc64@gmail.com>
Andreas Rheinhardt
Anton Khirnov
Martin Storsjö
Vittorio Giovara
Yu Yang
James Almer
Hendrik Leppkes
Andreas Rheinhardt
Mark Thompson
Linjie Fu
Jun Zhao
wm4
Wan-Teh Chang
Muhammad Faiz