| Message ID | AS8P250MB0744F8E14996CB5ABFF13D838FFAA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | New API for reference counting and ThreadFrames | expand |
On Tue, Sep 19, 2023 at 09:57:27PM +0200, Andreas Rheinhardt wrote: > The API is very similar by the ProgressFrame API, with the exception > that it no longer has an included AVFrame. Instead one can wait > on anything via a ThreadProgress. One just has to ensure that > the lifetime of the object containing the ThreadProgress is long > enough (the corresponding problem for ProgressFrames is solved > by allocating the progress and giving each thread a reference > to it). This will typically be solved by putting a ThreadProgress > in a refcounted structure that is shared between threads. > It will be put to the test in the following commits. > > An alternative to the check for whether the owner exists > (meaning "do we use frame-threading?") would be to initialize > the progress to INT_MAX in case frame threading is not in use. > This would probably be preferable on arches where atomic reads > are cheap (like x86), but are there ones where it is not? > > One could also (guarded by e.g. an ASSERT_LEVEL check) actually > track the progress for non-framethreading, too, in order to > track errors even in single-threaded mode. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/pthread_frame.c | 50 +++++++++++++++++++++++++++++++++++++ > libavcodec/threadprogress.h | 37 +++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > create mode 100644 libavcodec/threadprogress.h Seems to break build here with shared libs / clang CC libavcodec/pthread_frame.o src/libavcodec/pthread_frame.c:1108:9: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) atomic_load_explicit(&pro->progress, memory_order_acquire) >= n) ^ ~~~~~~~~~~~~~~ /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:135:30: note: expanded from macro 'atomic_load_explicit' #define atomic_load_explicit __c11_atomic_load ^ src/libavcodec/pthread_frame.c:1116:12: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) while (atomic_load_explicit(&pro->progress, memory_order_relaxed) < n) ^ ~~~~~~~~~~~~~~ /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:135:30: note: expanded from macro 'atomic_load_explicit' #define atomic_load_explicit __c11_atomic_load ^ 2 errors generated. src/ffbuild/common.mak:81: recipe for target 'libavcodec/pthread_frame.o' failed make: *** [libavcodec/pthread_frame.o] Error 1 make: Target 'all' not remade because of errors. [...]
Michael Niedermayer: > On Tue, Sep 19, 2023 at 09:57:27PM +0200, Andreas Rheinhardt wrote: >> The API is very similar by the ProgressFrame API, with the exception >> that it no longer has an included AVFrame. Instead one can wait >> on anything via a ThreadProgress. One just has to ensure that >> the lifetime of the object containing the ThreadProgress is long >> enough (the corresponding problem for ProgressFrames is solved >> by allocating the progress and giving each thread a reference >> to it). This will typically be solved by putting a ThreadProgress >> in a refcounted structure that is shared between threads. >> It will be put to the test in the following commits. >> >> An alternative to the check for whether the owner exists >> (meaning "do we use frame-threading?") would be to initialize >> the progress to INT_MAX in case frame threading is not in use. >> This would probably be preferable on arches where atomic reads >> are cheap (like x86), but are there ones where it is not? >> >> One could also (guarded by e.g. an ASSERT_LEVEL check) actually >> track the progress for non-framethreading, too, in order to >> track errors even in single-threaded mode. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/pthread_frame.c | 50 +++++++++++++++++++++++++++++++++++++ >> libavcodec/threadprogress.h | 37 +++++++++++++++++++++++++++ >> 2 files changed, 87 insertions(+) >> create mode 100644 libavcodec/threadprogress.h > > Seems to break build here with shared libs / clang > > CC libavcodec/pthread_frame.o > src/libavcodec/pthread_frame.c:1108:9: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > atomic_load_explicit(&pro->progress, memory_order_acquire) >= n) > ^ ~~~~~~~~~~~~~~ > /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:135:30: note: expanded from macro 'atomic_load_explicit' > #define atomic_load_explicit __c11_atomic_load > ^ > src/libavcodec/pthread_frame.c:1116:12: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > while (atomic_load_explicit(&pro->progress, memory_order_relaxed) < n) > ^ ~~~~~~~~~~~~~~ > /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:135:30: note: expanded from macro 'atomic_load_explicit' > #define atomic_load_explicit __c11_atomic_load > ^ > 2 errors generated. > src/ffbuild/common.mak:81: recipe for target 'libavcodec/pthread_frame.o' failed > make: *** [libavcodec/pthread_frame.o] Error 1 > make: Target 'all' not remade because of errors. > The original version of C11 did not allow to use atomic_load() on const-qualified atomic objects (presumably because on systems that don't have native support for atomic operations this might involve locking a mutex or so and this would involve writes); this has since been changed (see defect report N1807), yet your (presumably) ancient toolchain does not reflect that. I'll cast const away here and add a comment. - Andreas
Quoting Andreas Rheinhardt (2023-09-19 21:57:27) > The API is very similar by the ProgressFrame API, with the exception > that it no longer has an included AVFrame. Instead one can wait > on anything via a ThreadProgress. One just has to ensure that > the lifetime of the object containing the ThreadProgress is long > enough (the corresponding problem for ProgressFrames is solved > by allocating the progress and giving each thread a reference > to it). This will typically be solved by putting a ThreadProgress > in a refcounted structure that is shared between threads. > It will be put to the test in the following commits. > > An alternative to the check for whether the owner exists > (meaning "do we use frame-threading?") would be to initialize > the progress to INT_MAX in case frame threading is not in use. > This would probably be preferable on arches where atomic reads > are cheap (like x86), but are there ones where it is not? > > One could also (guarded by e.g. an ASSERT_LEVEL check) actually > track the progress for non-framethreading, too, in order to > track errors even in single-threaded mode. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/pthread_frame.c | 50 +++++++++++++++++++++++++++++++++++++ > libavcodec/threadprogress.h | 37 +++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+) > create mode 100644 libavcodec/threadprogress.h Again, I'd prefer to do without owner.
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 9e827f0606..219ab16ccd 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -40,6 +40,7 @@ #include "refstruct.h" #include "thread.h" #include "threadframe.h" +#include "threadprogress.h" #include "version_major.h" #include "libavutil/avassert.h" @@ -1069,3 +1070,52 @@ void ff_thread_progress_await(const ProgressFrame *f, int n) pthread_cond_wait(&p->progress_cond, &p->progress_mutex); pthread_mutex_unlock(&p->progress_mutex); } + +void ff_thread_progress_init(ThreadProgress *pro, AVCodecContext *owner) +{ + PerThreadContext *p = pro->owner = owner->active_thread_type & FF_THREAD_FRAME ? + owner->internal->thread_ctx : NULL; + if (!p) + return; + atomic_init(&pro->progress, -1); + if (atomic_load_explicit(&p->debug_threads, memory_order_relaxed)) + av_log(owner, AV_LOG_DEBUG, "Initializing ThreadProgress %p\n", (void*)pro); +} + +void ff_thread_progress_report2(ThreadProgress *pro, int n) +{ + PerThreadContext *p = pro->owner; + + if (!p || + atomic_load_explicit(&pro->progress, memory_order_relaxed) >= n) + return; + + if (atomic_load_explicit(&p->debug_threads, memory_order_relaxed)) + av_log(p->avctx, AV_LOG_DEBUG, + "%p finished %d\n", (void*)pro, n); + + pthread_mutex_lock(&p->progress_mutex); + + atomic_store_explicit(&pro->progress, n, memory_order_release); + + pthread_cond_broadcast(&p->progress_cond); + pthread_mutex_unlock(&p->progress_mutex); +} + +void ff_thread_progress_await2(const ThreadProgress *pro, int n) +{ + PerThreadContext *p = pro->owner; + + if (!p || + atomic_load_explicit(&pro->progress, memory_order_acquire) >= n) + return; + + if (atomic_load_explicit(&p->debug_threads, memory_order_relaxed)) + av_log(p->avctx, AV_LOG_DEBUG, + "thread awaiting %d from %p\n", n, (void*)pro); + + pthread_mutex_lock(&p->progress_mutex); + while (atomic_load_explicit(&pro->progress, memory_order_relaxed) < n) + pthread_cond_wait(&p->progress_cond, &p->progress_mutex); + pthread_mutex_unlock(&p->progress_mutex); +} diff --git a/libavcodec/threadprogress.h b/libavcodec/threadprogress.h new file mode 100644 index 0000000000..4f65e7da4d --- /dev/null +++ b/libavcodec/threadprogress.h @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2022 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVCODEC_THREADPROGRESS_H +#define AVCODEC_THREADPROGRESS_H + +#include <stdatomic.h> + +struct AVCodecContext; + +typedef struct ThreadProgress { + atomic_int progress; + struct PerThreadContext *owner; +} ThreadProgress; + +void ff_thread_progress_init(ThreadProgress *pro, struct AVCodecContext *owner); +void ff_thread_progress_report2(ThreadProgress *pro, int progress); +void ff_thread_progress_await2(const ThreadProgress *pro, int progress); + +#endif /* AVCODEC_THREADPROGRESS_H */
The API is very similar by the ProgressFrame API, with the exception that it no longer has an included AVFrame. Instead one can wait on anything via a ThreadProgress. One just has to ensure that the lifetime of the object containing the ThreadProgress is long enough (the corresponding problem for ProgressFrames is solved by allocating the progress and giving each thread a reference to it). This will typically be solved by putting a ThreadProgress in a refcounted structure that is shared between threads. It will be put to the test in the following commits. An alternative to the check for whether the owner exists (meaning "do we use frame-threading?") would be to initialize the progress to INT_MAX in case frame threading is not in use. This would probably be preferable on arches where atomic reads are cheap (like x86), but are there ones where it is not? One could also (guarded by e.g. an ASSERT_LEVEL check) actually track the progress for non-framethreading, too, in order to track errors even in single-threaded mode. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/pthread_frame.c | 50 +++++++++++++++++++++++++++++++++++++ libavcodec/threadprogress.h | 37 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 libavcodec/threadprogress.h