| Message ID | DB6PR0101MB2214DA47393D01E79A6212F38FBA9@DB6PR0101MB2214.eurprd01.prod.exchangelabs.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2,01/18] avcodec/pthread_slice: Don't reinitialise initialised mutex | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: > The HEVC decoder has both HEVCContext and HEVCLocalContext > structures. The latter is supposed to be the structure > containing the per-slicethread state. > > Yet up until now that is not how it is handled in practice: > Each HEVCLocalContext has a unique HEVCContext allocated for it > and each of these coincides except in exactly one field: The > corresponding HEVCLocalContext. This makes it possible to pass > the HEVCContext everywhere where logically a HEVCLocalContext > should be used. And up until recently, this is how it has been done. > > Yet the preceding patches changed this, making it possible > to avoid allocating redundant HEVCContexts. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ > libavcodec/hevcdec.h | 2 -- > 2 files changed, 16 insertions(+), 26 deletions(-) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index 9d1241f293..048fcc76b4 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > { > HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; > const HEVCContext *const s = lc->parent; > - HEVCContext *s1 = avctxt->priv_data; > - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; > + int ctb_size = 1 << s->ps.sps->log2_ctb_size; > int more_data = 1; > int ctb_row = job; > - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); > - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > - int thread = ctb_row % s1->threads_number; > + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); > + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > + int thread = ctb_row % s->threads_number; > int ret; > > if(ctb_row) { > @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > > ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); > > - if (atomic_load(&s1->wpp_err)) { > + if (atomic_load(&s->wpp_err)) { > ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy (this was building shared libs) CC libavcodec/hevcdec.o src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) if (atomic_load(&s->wpp_err)) { ^ ~~~~~~~~~~~ /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) ^ ~~~~~~ 1 error generated. src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed make: *** [libavcodec/hevcdec.o] Error 1 thx [...]
Michael Niedermayer: > On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >> The HEVC decoder has both HEVCContext and HEVCLocalContext >> structures. The latter is supposed to be the structure >> containing the per-slicethread state. >> >> Yet up until now that is not how it is handled in practice: >> Each HEVCLocalContext has a unique HEVCContext allocated for it >> and each of these coincides except in exactly one field: The >> corresponding HEVCLocalContext. This makes it possible to pass >> the HEVCContext everywhere where logically a HEVCLocalContext >> should be used. And up until recently, this is how it has been done. >> >> Yet the preceding patches changed this, making it possible >> to avoid allocating redundant HEVCContexts. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >> libavcodec/hevcdec.h | 2 -- >> 2 files changed, 16 insertions(+), 26 deletions(-) >> >> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> index 9d1241f293..048fcc76b4 100644 >> --- a/libavcodec/hevcdec.c >> +++ b/libavcodec/hevcdec.c >> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >> { >> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >> const HEVCContext *const s = lc->parent; >> - HEVCContext *s1 = avctxt->priv_data; >> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >> int more_data = 1; >> int ctb_row = job; >> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >> - int thread = ctb_row % s1->threads_number; >> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >> + int thread = ctb_row % s->threads_number; >> int ret; >> >> if(ctb_row) { >> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >> >> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >> >> - if (atomic_load(&s1->wpp_err)) { >> + if (atomic_load(&s->wpp_err)) { >> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); > > the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy > (this was building shared libs) > > > CC libavcodec/hevcdec.o > src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > if (atomic_load(&s->wpp_err)) { > ^ ~~~~~~~~~~~ > /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' > #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) > ^ ~~~~~~ > 1 error generated. > src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed > make: *** [libavcodec/hevcdec.o] Error 1 > > thx > Thanks for testing this. atomic_load is indeed declared without const in 7.17.7.2: C atomic_load(volatile A *object); Upon reflection this makes sense, because if atomics are implemented via mutexes, even a read may involve a preceding write. So I'll cast const away here, too, and add a comment. (It works when casting const away, doesn't it?) - Andreas
On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: > >> The HEVC decoder has both HEVCContext and HEVCLocalContext > >> structures. The latter is supposed to be the structure > >> containing the per-slicethread state. > >> > >> Yet up until now that is not how it is handled in practice: > >> Each HEVCLocalContext has a unique HEVCContext allocated for it > >> and each of these coincides except in exactly one field: The > >> corresponding HEVCLocalContext. This makes it possible to pass > >> the HEVCContext everywhere where logically a HEVCLocalContext > >> should be used. And up until recently, this is how it has been done. > >> > >> Yet the preceding patches changed this, making it possible > >> to avoid allocating redundant HEVCContexts. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ > >> libavcodec/hevcdec.h | 2 -- > >> 2 files changed, 16 insertions(+), 26 deletions(-) > >> > >> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > >> index 9d1241f293..048fcc76b4 100644 > >> --- a/libavcodec/hevcdec.c > >> +++ b/libavcodec/hevcdec.c > >> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >> { > >> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; > >> const HEVCContext *const s = lc->parent; > >> - HEVCContext *s1 = avctxt->priv_data; > >> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; > >> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; > >> int more_data = 1; > >> int ctb_row = job; > >> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); > >> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >> - int thread = ctb_row % s1->threads_number; > >> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); > >> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >> + int thread = ctb_row % s->threads_number; > >> int ret; > >> > >> if(ctb_row) { > >> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >> > >> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); > >> > >> - if (atomic_load(&s1->wpp_err)) { > >> + if (atomic_load(&s->wpp_err)) { > >> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); > > > > the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy > > (this was building shared libs) > > > > > > CC libavcodec/hevcdec.o > > src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > > if (atomic_load(&s->wpp_err)) { > > ^ ~~~~~~~~~~~ > > /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' > > #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) > > ^ ~~~~~~ > > 1 error generated. > > src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed > > make: *** [libavcodec/hevcdec.o] Error 1 > > > > thx > > > > Thanks for testing this. atomic_load is indeed declared without const in > 7.17.7.2: > > C atomic_load(volatile A *object); > > Upon reflection this makes sense, because if atomics are implemented via > mutexes, even a read may involve a preceding write. So I'll cast const > away here, too, and add a comment. (It works when casting const away, > doesn't it?) This doesnt feel "right". These pointers should not be coming from a const if they are written to The compiler accepts it with an explicit cast though. With an implicit cast it produces a warning thx [...]
Michael Niedermayer: > On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >>>> The HEVC decoder has both HEVCContext and HEVCLocalContext >>>> structures. The latter is supposed to be the structure >>>> containing the per-slicethread state. >>>> >>>> Yet up until now that is not how it is handled in practice: >>>> Each HEVCLocalContext has a unique HEVCContext allocated for it >>>> and each of these coincides except in exactly one field: The >>>> corresponding HEVCLocalContext. This makes it possible to pass >>>> the HEVCContext everywhere where logically a HEVCLocalContext >>>> should be used. And up until recently, this is how it has been done. >>>> >>>> Yet the preceding patches changed this, making it possible >>>> to avoid allocating redundant HEVCContexts. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >>>> libavcodec/hevcdec.h | 2 -- >>>> 2 files changed, 16 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >>>> index 9d1241f293..048fcc76b4 100644 >>>> --- a/libavcodec/hevcdec.c >>>> +++ b/libavcodec/hevcdec.c >>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>> { >>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >>>> const HEVCContext *const s = lc->parent; >>>> - HEVCContext *s1 = avctxt->priv_data; >>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >>>> int more_data = 1; >>>> int ctb_row = job; >>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>> - int thread = ctb_row % s1->threads_number; >>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>> + int thread = ctb_row % s->threads_number; >>>> int ret; >>>> >>>> if(ctb_row) { >>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>> >>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >>>> >>>> - if (atomic_load(&s1->wpp_err)) { >>>> + if (atomic_load(&s->wpp_err)) { >>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); >>> >>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy >>> (this was building shared libs) >>> >>> >>> CC libavcodec/hevcdec.o >>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) >>> if (atomic_load(&s->wpp_err)) { >>> ^ ~~~~~~~~~~~ >>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' >>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) >>> ^ ~~~~~~ >>> 1 error generated. >>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed >>> make: *** [libavcodec/hevcdec.o] Error 1 >>> >>> thx >>> >> >> Thanks for testing this. atomic_load is indeed declared without const in >> 7.17.7.2: >> >> C atomic_load(volatile A *object); >> >> Upon reflection this makes sense, because if atomics are implemented via >> mutexes, even a read may involve a preceding write. So I'll cast const >> away here, too, and add a comment. (It works when casting const away, >> doesn't it?) > > This doesnt feel "right". These pointers should not be coming from a const > if they are written to > The HEVCContext is not const because the underlying object is const; the HEVCContext is const when accessed from any part of the code that may be run from slice threads, because if a slice thread modifies it, you have a data race in case any of the other slice threads reads this field or modifies it itself. But this is by definition not true for atomic operations, so casting const away for them is fine. > The compiler accepts it with an explicit cast though. With an implicit cast > it produces a warning >
Andreas Rheinhardt: > Michael Niedermayer: >> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: >>> Michael Niedermayer: >>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext >>>>> structures. The latter is supposed to be the structure >>>>> containing the per-slicethread state. >>>>> >>>>> Yet up until now that is not how it is handled in practice: >>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it >>>>> and each of these coincides except in exactly one field: The >>>>> corresponding HEVCLocalContext. This makes it possible to pass >>>>> the HEVCContext everywhere where logically a HEVCLocalContext >>>>> should be used. And up until recently, this is how it has been done. >>>>> >>>>> Yet the preceding patches changed this, making it possible >>>>> to avoid allocating redundant HEVCContexts. >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>> --- >>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >>>>> libavcodec/hevcdec.h | 2 -- >>>>> 2 files changed, 16 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >>>>> index 9d1241f293..048fcc76b4 100644 >>>>> --- a/libavcodec/hevcdec.c >>>>> +++ b/libavcodec/hevcdec.c >>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>> { >>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >>>>> const HEVCContext *const s = lc->parent; >>>>> - HEVCContext *s1 = avctxt->priv_data; >>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >>>>> int more_data = 1; >>>>> int ctb_row = job; >>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>> - int thread = ctb_row % s1->threads_number; >>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>> + int thread = ctb_row % s->threads_number; >>>>> int ret; >>>>> >>>>> if(ctb_row) { >>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>> >>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >>>>> >>>>> - if (atomic_load(&s1->wpp_err)) { >>>>> + if (atomic_load(&s->wpp_err)) { >>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); >>>> >>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy >>>> (this was building shared libs) >>>> >>>> >>>> CC libavcodec/hevcdec.o >>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) >>>> if (atomic_load(&s->wpp_err)) { >>>> ^ ~~~~~~~~~~~ >>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' >>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) >>>> ^ ~~~~~~ >>>> 1 error generated. >>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed >>>> make: *** [libavcodec/hevcdec.o] Error 1 >>>> >>>> thx >>>> >>> >>> Thanks for testing this. atomic_load is indeed declared without const in >>> 7.17.7.2: >>> >>> C atomic_load(volatile A *object); >>> >>> Upon reflection this makes sense, because if atomics are implemented via >>> mutexes, even a read may involve a preceding write. So I'll cast const >>> away here, too, and add a comment. (It works when casting const away, >>> doesn't it?) >> >> This doesnt feel "right". These pointers should not be coming from a const >> if they are written to >> > > The HEVCContext is not const because the underlying object is const; the > HEVCContext is const when accessed from any part of the code that may be > run from slice threads, because if a slice thread modifies it, you have > a data race in case any of the other slice threads reads this field or > modifies it itself. But this is by definition not true for atomic > operations, so casting const away for them is fine. > >> The compiler accepts it with an explicit cast though. With an implicit cast >> it produces a warning >> Did the above explanation satisfy you? Or do you want something else? - Andreas
On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Michael Niedermayer: > >> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: > >>> Michael Niedermayer: > >>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: > >>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext > >>>>> structures. The latter is supposed to be the structure > >>>>> containing the per-slicethread state. > >>>>> > >>>>> Yet up until now that is not how it is handled in practice: > >>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it > >>>>> and each of these coincides except in exactly one field: The > >>>>> corresponding HEVCLocalContext. This makes it possible to pass > >>>>> the HEVCContext everywhere where logically a HEVCLocalContext > >>>>> should be used. And up until recently, this is how it has been done. > >>>>> > >>>>> Yet the preceding patches changed this, making it possible > >>>>> to avoid allocating redundant HEVCContexts. > >>>>> > >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >>>>> --- > >>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ > >>>>> libavcodec/hevcdec.h | 2 -- > >>>>> 2 files changed, 16 insertions(+), 26 deletions(-) > >>>>> > >>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > >>>>> index 9d1241f293..048fcc76b4 100644 > >>>>> --- a/libavcodec/hevcdec.c > >>>>> +++ b/libavcodec/hevcdec.c > >>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>> { > >>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; > >>>>> const HEVCContext *const s = lc->parent; > >>>>> - HEVCContext *s1 = avctxt->priv_data; > >>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; > >>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; > >>>>> int more_data = 1; > >>>>> int ctb_row = job; > >>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); > >>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>> - int thread = ctb_row % s1->threads_number; > >>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); > >>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>> + int thread = ctb_row % s->threads_number; > >>>>> int ret; > >>>>> > >>>>> if(ctb_row) { > >>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>> > >>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); > >>>>> > >>>>> - if (atomic_load(&s1->wpp_err)) { > >>>>> + if (atomic_load(&s->wpp_err)) { > >>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); > >>>> > >>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy > >>>> (this was building shared libs) > >>>> > >>>> > >>>> CC libavcodec/hevcdec.o > >>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > >>>> if (atomic_load(&s->wpp_err)) { > >>>> ^ ~~~~~~~~~~~ > >>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' > >>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) > >>>> ^ ~~~~~~ > >>>> 1 error generated. > >>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed > >>>> make: *** [libavcodec/hevcdec.o] Error 1 > >>>> > >>>> thx > >>>> > >>> > >>> Thanks for testing this. atomic_load is indeed declared without const in > >>> 7.17.7.2: > >>> > >>> C atomic_load(volatile A *object); > >>> > >>> Upon reflection this makes sense, because if atomics are implemented via > >>> mutexes, even a read may involve a preceding write. So I'll cast const > >>> away here, too, and add a comment. (It works when casting const away, > >>> doesn't it?) > >> > >> This doesnt feel "right". These pointers should not be coming from a const > >> if they are written to > >> > > > > The HEVCContext is not const because the underlying object is const; the > > HEVCContext is const when accessed from any part of the code that may be > > run from slice threads, because if a slice thread modifies it, you have > > a data race in case any of the other slice threads reads this field or > > modifies it itself. But this is by definition not true for atomic > > operations, so casting const away for them is fine. > > > >> The compiler accepts it with an explicit cast though. With an implicit cast > >> it produces a warning > >> > > Did the above explanation satisfy you? Or do you want something else? sure, ok [...]
Michael Niedermayer: > On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: >> Andreas Rheinhardt: >>> Michael Niedermayer: >>>> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: >>>>> Michael Niedermayer: >>>>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >>>>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext >>>>>>> structures. The latter is supposed to be the structure >>>>>>> containing the per-slicethread state. >>>>>>> >>>>>>> Yet up until now that is not how it is handled in practice: >>>>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it >>>>>>> and each of these coincides except in exactly one field: The >>>>>>> corresponding HEVCLocalContext. This makes it possible to pass >>>>>>> the HEVCContext everywhere where logically a HEVCLocalContext >>>>>>> should be used. And up until recently, this is how it has been done. >>>>>>> >>>>>>> Yet the preceding patches changed this, making it possible >>>>>>> to avoid allocating redundant HEVCContexts. >>>>>>> >>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>>>> --- >>>>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >>>>>>> libavcodec/hevcdec.h | 2 -- >>>>>>> 2 files changed, 16 insertions(+), 26 deletions(-) >>>>>>> >>>>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >>>>>>> index 9d1241f293..048fcc76b4 100644 >>>>>>> --- a/libavcodec/hevcdec.c >>>>>>> +++ b/libavcodec/hevcdec.c >>>>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>> { >>>>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >>>>>>> const HEVCContext *const s = lc->parent; >>>>>>> - HEVCContext *s1 = avctxt->priv_data; >>>>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >>>>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >>>>>>> int more_data = 1; >>>>>>> int ctb_row = job; >>>>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >>>>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>> - int thread = ctb_row % s1->threads_number; >>>>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >>>>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>> + int thread = ctb_row % s->threads_number; >>>>>>> int ret; >>>>>>> >>>>>>> if(ctb_row) { >>>>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>> >>>>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >>>>>>> >>>>>>> - if (atomic_load(&s1->wpp_err)) { >>>>>>> + if (atomic_load(&s->wpp_err)) { >>>>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); >>>>>> >>>>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy >>>>>> (this was building shared libs) >>>>>> >>>>>> >>>>>> CC libavcodec/hevcdec.o >>>>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) >>>>>> if (atomic_load(&s->wpp_err)) { >>>>>> ^ ~~~~~~~~~~~ >>>>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' >>>>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) >>>>>> ^ ~~~~~~ >>>>>> 1 error generated. >>>>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed >>>>>> make: *** [libavcodec/hevcdec.o] Error 1 >>>>>> >>>>>> thx >>>>>> >>>>> >>>>> Thanks for testing this. atomic_load is indeed declared without const in >>>>> 7.17.7.2: >>>>> >>>>> C atomic_load(volatile A *object); >>>>> >>>>> Upon reflection this makes sense, because if atomics are implemented via >>>>> mutexes, even a read may involve a preceding write. So I'll cast const >>>>> away here, too, and add a comment. (It works when casting const away, >>>>> doesn't it?) >>>> >>>> This doesnt feel "right". These pointers should not be coming from a const >>>> if they are written to >>>> >>> >>> The HEVCContext is not const because the underlying object is const; the >>> HEVCContext is const when accessed from any part of the code that may be >>> run from slice threads, because if a slice thread modifies it, you have >>> a data race in case any of the other slice threads reads this field or >>> modifies it itself. But this is by definition not true for atomic >>> operations, so casting const away for them is fine. >>> >>>> The compiler accepts it with an explicit cast though. With an implicit cast >>>> it produces a warning >>>> >> >> Did the above explanation satisfy you? Or do you want something else? > > sure, ok > > [...] > Good to hear. This patchset (namely patch 11/18: "avcodec/hevcpred: Pass HEVCLocalContext when slice-threading") includes modifications to mips code that I created blindly. Can you please test it? Here is a branch of this rebased on top of current git master: https://github.com/mkver/FFmpeg/commits/hevc_wpp (Said branch actually contains a bit of further work which also modifies mips code (in particular, https://github.com/mkver/FFmpeg/commit/cf441e559b8d4bf2c05c29483ccf49e82fc6b863 does so); you may also test this.) - Andreas
On Sat, Jul 23, 2022 at 11:42:23PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: > >> Andreas Rheinhardt: > >>> Michael Niedermayer: > >>>> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: > >>>>> Michael Niedermayer: > >>>>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: > >>>>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext > >>>>>>> structures. The latter is supposed to be the structure > >>>>>>> containing the per-slicethread state. > >>>>>>> > >>>>>>> Yet up until now that is not how it is handled in practice: > >>>>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it > >>>>>>> and each of these coincides except in exactly one field: The > >>>>>>> corresponding HEVCLocalContext. This makes it possible to pass > >>>>>>> the HEVCContext everywhere where logically a HEVCLocalContext > >>>>>>> should be used. And up until recently, this is how it has been done. > >>>>>>> > >>>>>>> Yet the preceding patches changed this, making it possible > >>>>>>> to avoid allocating redundant HEVCContexts. > >>>>>>> > >>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >>>>>>> --- > >>>>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ > >>>>>>> libavcodec/hevcdec.h | 2 -- > >>>>>>> 2 files changed, 16 insertions(+), 26 deletions(-) > >>>>>>> > >>>>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > >>>>>>> index 9d1241f293..048fcc76b4 100644 > >>>>>>> --- a/libavcodec/hevcdec.c > >>>>>>> +++ b/libavcodec/hevcdec.c > >>>>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>>>> { > >>>>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; > >>>>>>> const HEVCContext *const s = lc->parent; > >>>>>>> - HEVCContext *s1 = avctxt->priv_data; > >>>>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; > >>>>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; > >>>>>>> int more_data = 1; > >>>>>>> int ctb_row = job; > >>>>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); > >>>>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>>>> - int thread = ctb_row % s1->threads_number; > >>>>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); > >>>>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>>>> + int thread = ctb_row % s->threads_number; > >>>>>>> int ret; > >>>>>>> > >>>>>>> if(ctb_row) { > >>>>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>>>> > >>>>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); > >>>>>>> > >>>>>>> - if (atomic_load(&s1->wpp_err)) { > >>>>>>> + if (atomic_load(&s->wpp_err)) { > >>>>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); > >>>>>> > >>>>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy > >>>>>> (this was building shared libs) > >>>>>> > >>>>>> > >>>>>> CC libavcodec/hevcdec.o > >>>>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > >>>>>> if (atomic_load(&s->wpp_err)) { > >>>>>> ^ ~~~~~~~~~~~ > >>>>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' > >>>>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) > >>>>>> ^ ~~~~~~ > >>>>>> 1 error generated. > >>>>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed > >>>>>> make: *** [libavcodec/hevcdec.o] Error 1 > >>>>>> > >>>>>> thx > >>>>>> > >>>>> > >>>>> Thanks for testing this. atomic_load is indeed declared without const in > >>>>> 7.17.7.2: > >>>>> > >>>>> C atomic_load(volatile A *object); > >>>>> > >>>>> Upon reflection this makes sense, because if atomics are implemented via > >>>>> mutexes, even a read may involve a preceding write. So I'll cast const > >>>>> away here, too, and add a comment. (It works when casting const away, > >>>>> doesn't it?) > >>>> > >>>> This doesnt feel "right". These pointers should not be coming from a const > >>>> if they are written to > >>>> > >>> > >>> The HEVCContext is not const because the underlying object is const; the > >>> HEVCContext is const when accessed from any part of the code that may be > >>> run from slice threads, because if a slice thread modifies it, you have > >>> a data race in case any of the other slice threads reads this field or > >>> modifies it itself. But this is by definition not true for atomic > >>> operations, so casting const away for them is fine. > >>> > >>>> The compiler accepts it with an explicit cast though. With an implicit cast > >>>> it produces a warning > >>>> > >> > >> Did the above explanation satisfy you? Or do you want something else? > > > > sure, ok > > > > [...] > > > > Good to hear. This patchset (namely patch 11/18: "avcodec/hevcpred: Pass > HEVCLocalContext when slice-threading") includes modifications to mips > code that I created blindly. Can you please test it? Here is a branch of > this rebased on top of current git master: > https://github.com/mkver/FFmpeg/commits/hevc_wpp > (Said branch actually contains a bit of further work which also modifies > mips code (in particular, > https://github.com/mkver/FFmpeg/commit/cf441e559b8d4bf2c05c29483ccf49e82fc6b863 > does so); you may also test this.) what exact tests do we need ? simple fate ? any specific thread type count ? also note i can only test qemu mips not real hw. I stopped maintaining the MIPS hw and sofar noone volunteered to take its maintaince over thx
Michael Niedermayer: > On Sat, Jul 23, 2022 at 11:42:23PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: >>>> Andreas Rheinhardt: >>>>> Michael Niedermayer: >>>>>> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: >>>>>>> Michael Niedermayer: >>>>>>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >>>>>>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext >>>>>>>>> structures. The latter is supposed to be the structure >>>>>>>>> containing the per-slicethread state. >>>>>>>>> >>>>>>>>> Yet up until now that is not how it is handled in practice: >>>>>>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it >>>>>>>>> and each of these coincides except in exactly one field: The >>>>>>>>> corresponding HEVCLocalContext. This makes it possible to pass >>>>>>>>> the HEVCContext everywhere where logically a HEVCLocalContext >>>>>>>>> should be used. And up until recently, this is how it has been done. >>>>>>>>> >>>>>>>>> Yet the preceding patches changed this, making it possible >>>>>>>>> to avoid allocating redundant HEVCContexts. >>>>>>>>> >>>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>>>>>> --- >>>>>>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >>>>>>>>> libavcodec/hevcdec.h | 2 -- >>>>>>>>> 2 files changed, 16 insertions(+), 26 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >>>>>>>>> index 9d1241f293..048fcc76b4 100644 >>>>>>>>> --- a/libavcodec/hevcdec.c >>>>>>>>> +++ b/libavcodec/hevcdec.c >>>>>>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>>>> { >>>>>>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >>>>>>>>> const HEVCContext *const s = lc->parent; >>>>>>>>> - HEVCContext *s1 = avctxt->priv_data; >>>>>>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >>>>>>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >>>>>>>>> int more_data = 1; >>>>>>>>> int ctb_row = job; >>>>>>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >>>>>>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>>>> - int thread = ctb_row % s1->threads_number; >>>>>>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >>>>>>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>>>> + int thread = ctb_row % s->threads_number; >>>>>>>>> int ret; >>>>>>>>> >>>>>>>>> if(ctb_row) { >>>>>>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>>>> >>>>>>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >>>>>>>>> >>>>>>>>> - if (atomic_load(&s1->wpp_err)) { >>>>>>>>> + if (atomic_load(&s->wpp_err)) { >>>>>>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); >>>>>>>> >>>>>>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy >>>>>>>> (this was building shared libs) >>>>>>>> >>>>>>>> >>>>>>>> CC libavcodec/hevcdec.o >>>>>>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) >>>>>>>> if (atomic_load(&s->wpp_err)) { >>>>>>>> ^ ~~~~~~~~~~~ >>>>>>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' >>>>>>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) >>>>>>>> ^ ~~~~~~ >>>>>>>> 1 error generated. >>>>>>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed >>>>>>>> make: *** [libavcodec/hevcdec.o] Error 1 >>>>>>>> >>>>>>>> thx >>>>>>>> >>>>>>> >>>>>>> Thanks for testing this. atomic_load is indeed declared without const in >>>>>>> 7.17.7.2: >>>>>>> >>>>>>> C atomic_load(volatile A *object); >>>>>>> >>>>>>> Upon reflection this makes sense, because if atomics are implemented via >>>>>>> mutexes, even a read may involve a preceding write. So I'll cast const >>>>>>> away here, too, and add a comment. (It works when casting const away, >>>>>>> doesn't it?) >>>>>> >>>>>> This doesnt feel "right". These pointers should not be coming from a const >>>>>> if they are written to >>>>>> >>>>> >>>>> The HEVCContext is not const because the underlying object is const; the >>>>> HEVCContext is const when accessed from any part of the code that may be >>>>> run from slice threads, because if a slice thread modifies it, you have >>>>> a data race in case any of the other slice threads reads this field or >>>>> modifies it itself. But this is by definition not true for atomic >>>>> operations, so casting const away for them is fine. >>>>> >>>>>> The compiler accepts it with an explicit cast though. With an implicit cast >>>>>> it produces a warning >>>>>> >>>> >>>> Did the above explanation satisfy you? Or do you want something else? >>> >>> sure, ok >>> >>> [...] >>> >> >> Good to hear. This patchset (namely patch 11/18: "avcodec/hevcpred: Pass >> HEVCLocalContext when slice-threading") includes modifications to mips >> code that I created blindly. Can you please test it? Here is a branch of >> this rebased on top of current git master: >> https://github.com/mkver/FFmpeg/commits/hevc_wpp >> (Said branch actually contains a bit of further work which also modifies >> mips code (in particular, >> https://github.com/mkver/FFmpeg/commit/cf441e559b8d4bf2c05c29483ccf49e82fc6b863 >> does so); you may also test this.) > > what exact tests do we need ? > simple fate ? any specific thread type count ? > also note i can only test qemu mips not real hw. > I stopped maintaining the MIPS hw and sofar noone volunteered to take its > maintaince over > Compiling (in particular being on the lookout for new warnings) and a simple fate should be enough. - Andreas
On Sun, Jul 24, 2022 at 11:26:37PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Sat, Jul 23, 2022 at 11:42:23PM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >>> On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: > >>>> Andreas Rheinhardt: > >>>>> Michael Niedermayer: > >>>>>> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: > >>>>>>> Michael Niedermayer: > >>>>>>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: > >>>>>>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext > >>>>>>>>> structures. The latter is supposed to be the structure > >>>>>>>>> containing the per-slicethread state. > >>>>>>>>> > >>>>>>>>> Yet up until now that is not how it is handled in practice: > >>>>>>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it > >>>>>>>>> and each of these coincides except in exactly one field: The > >>>>>>>>> corresponding HEVCLocalContext. This makes it possible to pass > >>>>>>>>> the HEVCContext everywhere where logically a HEVCLocalContext > >>>>>>>>> should be used. And up until recently, this is how it has been done. > >>>>>>>>> > >>>>>>>>> Yet the preceding patches changed this, making it possible > >>>>>>>>> to avoid allocating redundant HEVCContexts. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >>>>>>>>> --- > >>>>>>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ > >>>>>>>>> libavcodec/hevcdec.h | 2 -- > >>>>>>>>> 2 files changed, 16 insertions(+), 26 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > >>>>>>>>> index 9d1241f293..048fcc76b4 100644 > >>>>>>>>> --- a/libavcodec/hevcdec.c > >>>>>>>>> +++ b/libavcodec/hevcdec.c > >>>>>>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>>>>>> { > >>>>>>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; > >>>>>>>>> const HEVCContext *const s = lc->parent; > >>>>>>>>> - HEVCContext *s1 = avctxt->priv_data; > >>>>>>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; > >>>>>>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; > >>>>>>>>> int more_data = 1; > >>>>>>>>> int ctb_row = job; > >>>>>>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); > >>>>>>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>>>>>> - int thread = ctb_row % s1->threads_number; > >>>>>>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); > >>>>>>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; > >>>>>>>>> + int thread = ctb_row % s->threads_number; > >>>>>>>>> int ret; > >>>>>>>>> > >>>>>>>>> if(ctb_row) { > >>>>>>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, > >>>>>>>>> > >>>>>>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); > >>>>>>>>> > >>>>>>>>> - if (atomic_load(&s1->wpp_err)) { > >>>>>>>>> + if (atomic_load(&s->wpp_err)) { > >>>>>>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); > >>>>>>>> > >>>>>>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy > >>>>>>>> (this was building shared libs) > >>>>>>>> > >>>>>>>> > >>>>>>>> CC libavcodec/hevcdec.o > >>>>>>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) > >>>>>>>> if (atomic_load(&s->wpp_err)) { > >>>>>>>> ^ ~~~~~~~~~~~ > >>>>>>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' > >>>>>>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) > >>>>>>>> ^ ~~~~~~ > >>>>>>>> 1 error generated. > >>>>>>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed > >>>>>>>> make: *** [libavcodec/hevcdec.o] Error 1 > >>>>>>>> > >>>>>>>> thx > >>>>>>>> > >>>>>>> > >>>>>>> Thanks for testing this. atomic_load is indeed declared without const in > >>>>>>> 7.17.7.2: > >>>>>>> > >>>>>>> C atomic_load(volatile A *object); > >>>>>>> > >>>>>>> Upon reflection this makes sense, because if atomics are implemented via > >>>>>>> mutexes, even a read may involve a preceding write. So I'll cast const > >>>>>>> away here, too, and add a comment. (It works when casting const away, > >>>>>>> doesn't it?) > >>>>>> > >>>>>> This doesnt feel "right". These pointers should not be coming from a const > >>>>>> if they are written to > >>>>>> > >>>>> > >>>>> The HEVCContext is not const because the underlying object is const; the > >>>>> HEVCContext is const when accessed from any part of the code that may be > >>>>> run from slice threads, because if a slice thread modifies it, you have > >>>>> a data race in case any of the other slice threads reads this field or > >>>>> modifies it itself. But this is by definition not true for atomic > >>>>> operations, so casting const away for them is fine. > >>>>> > >>>>>> The compiler accepts it with an explicit cast though. With an implicit cast > >>>>>> it produces a warning > >>>>>> > >>>> > >>>> Did the above explanation satisfy you? Or do you want something else? > >>> > >>> sure, ok > >>> > >>> [...] > >>> > >> > >> Good to hear. This patchset (namely patch 11/18: "avcodec/hevcpred: Pass > >> HEVCLocalContext when slice-threading") includes modifications to mips > >> code that I created blindly. Can you please test it? Here is a branch of > >> this rebased on top of current git master: > >> https://github.com/mkver/FFmpeg/commits/hevc_wpp > >> (Said branch actually contains a bit of further work which also modifies > >> mips code (in particular, > >> https://github.com/mkver/FFmpeg/commit/cf441e559b8d4bf2c05c29483ccf49e82fc6b863 > >> does so); you may also test this.) > > > > what exact tests do we need ? > > simple fate ? any specific thread type count ? > > also note i can only test qemu mips not real hw. > > I stopped maintaining the MIPS hw and sofar noone volunteered to take its > > maintaince over > > > > Compiling (in particular being on the lookout for new warnings) and a > simple fate should be enough. I seethis: (NOT sanity checked and i also had some other patches from today applied) Probably these are all unrelated nonsense --- fatelist-oldw 2022-07-25 21:19:24.555885524 +0200 +++ fatelistw 2022-07-25 21:19:30.715956081 +0200 @@ -1,3 +1,5 @@ +src/tests/checkasm/synth_filter.c:66:42: warning: unused variable ‘offset_b’ [-Wunused-variable] +src/tests/checkasm/sbrdsp.c:180:14: warning: unused variable ‘bw’ [-Wunused-variable] src/libavfilter/dnn/dnn_backend_common.c:94:11: warning: unused variable ‘status’ [-Wunused-variable] src/libavfilter/dnn/dnn_backend_common.c:114:11: warning: unused variable ‘status’ [-Wunused-variable] src/libavfilter/dnn/dnn_backend_common.c:80:14: warning: ‘async_thread_routine’ defined but not used [-Wunused-function] @@ -7,20 +9,20 @@ src/libavformat/dashenc.c:1960:63: warning: ‘%s’ directive output may be truncated writing up to 1023 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/dashenc.c:492:49: warning: ‘media_’ directive output may be truncated writing 6 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/dashenc.c:2257:59: warning: ‘%s’ directive output may be truncated writing up to 1023 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] -src/libavformat/matroskaenc.c:3074:58: warning: ‘%012.9f’ directive output may be truncated writing between 12 and 320 bytes into a region of size between 8 and 14 [-Wformat-truncation=] src/libavformat/mlvdec.c:361:63: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=] -src/libavformat/movenc.c:1126:8: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow] +src/libavformat/matroskaenc.c:3074:58: warning: ‘%012.9f’ directive output may be truncated writing between 12 and 320 bytes into a region of size between 8 and 14 [-Wformat-truncation=] src/libavformat/smoothstreamingenc.c:509:49: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/smoothstreamingenc.c:544:63: warning: ‘/FragmentInfo(’ directive output may be truncated writing 14 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/smoothstreamingenc.c:545:63: warning: ‘/Fragments(’ directive output may be truncated writing 11 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/smoothstreamingenc.c:537:53: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] src/libavformat/vorbiscomment.c:103:63: warning: ‘%03d’ directive output may be truncated writing between 3 and 10 bytes into a region of size 4 [-Wformat-truncation=] src/libavformat/vorbiscomment.c:104:69: warning: ‘%02d’ directive output may be truncated writing between 2 and 3 bytes into a region of size between 1 and 7 [-Wformat-truncation=] +src/libavformat/movenc.c:1126:8: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow] src/libavcodec/mips/aacsbr_mips.h:62:13: warning: ‘sbr_qmf_analysis_mips’ defined but not used [-Wunused-function] src/libavcodec/dvenc.c:786:81: warning: array subscript is above array bounds [-Warray-bounds] src/libavcodec/dvenc.c:786:81: warning: array subscript is above array bounds [-Warray-bounds] src/libavcodec/ffv1dec.c:999:13: warning: ‘copy_fields’ defined but not used [-Wunused-function] -src/libavcodec/hevcdec.c:3540:12: warning: ‘hevc_ref_frame’ defined but not used [-Wunused-function] +src/libavcodec/hevcdec.c:3529:12: warning: ‘hevc_ref_frame’ defined but not used [-Wunused-function] src/libavcodec/mobiclip.c:471:24: warning: array subscript is above array bounds [-Warray-bounds] src/libavcodec/mobiclip.c:475:16: warning: array subscript is above array bounds [-Warray-bounds] src/libavcodec/mobiclip.c:471:24: warning: array subscript is above array bounds [-Warray-bounds] @@ -41,11 +43,11 @@ src/libavcodec/pcm-bluray.c:190:49: warning: passing argument 2 of ‘bytestream2_get_buffer’ from incompatible pointer type [-Wincompatible-pointer-types] src/libavcodec/pcm-dvd.c:160:37: warning: passing argument 2 of ‘bytestream2_get_buffer’ from incompatible pointer type [-Wincompatible-pointer-types] src/libavcodec/qdm2.c:1002:47: warning: array subscript is above array bounds [-Warray-bounds] -src/libavcodec/vp8.c:2373:30: warning: variable ‘next_td’ set but not used [-Wunused-but-set-variable] -src/libavcodec/vp8.c:2586:37: warning: unused variable ‘prev_td’ [-Wunused-variable] -src/libavcodec/vp8.c:2586:20: warning: unused variable ‘next_td’ [-Wunused-variable] -src/libavcodec/vp8.c:109:12: warning: ‘vp8_ref_frame’ defined but not used [-Wunused-function] -src/libavcodec/vp9.c:1795:9: warning: unused variable ‘ret’ [-Wunused-variable] src/libavutil/timecode.c:123:60: warning: ‘%0*d’ directive output may be truncated writing between 1 and 10 bytes into a region of size between 2 and 14 [-Wformat-truncation=] +src/libavcodec/vp9.c:1795:9: warning: unused variable ‘ret’ [-Wunused-variable] src/fftools/ffprobe.c:333:11: warning: unused variable ‘new_log_buffer’ [-Wunused-variable] src/fftools/ffprobe.c:329:14: warning: unused variable ‘avc’ [-Wunused-variable] +src/libavcodec/vp8.c:2373:30: warning: variable ‘next_td’ set but not used [-Wunused-but-set-variable] +src/libavcodec/vp8.c:2587:37: warning: unused variable ‘prev_td’ [-Wunused-variable] +src/libavcodec/vp8.c:2587:20: warning: unused variable ‘next_td’ [-Wunused-variable] +src/libavcodec/vp8.c:109:12: warning: ‘vp8_ref_frame’ defined but not used [-Wunused-function] There is also the fate-filter-metadata-signalstats-yuv420p10 test which fails before and afterwards -pts=0|tag:lavfi.signalstats.UBITDEPTH=2|tag:lavfi.signalstats.YMIN=943|tag:lavfi.signalstats.YLOW=943|tag:lavfi.signalstats.YAVG=943|tag:lavfi.signalstats.YHIGH=943|tag:lavfi.signalstats.YMAX=943|tag:lavfi.signalstats.UMIN=514|tag:lavfi.signalstats.ULOW=514|tag:lavfi.signalstats.UAVG=514|tag:lavfi.signalstats.UHIGH=514|tag:lavfi.signalstats.UMAX=514|tag:lavfi.signalstats.VMIN=514|tag:lavfi.signalstats.VLOW=514|tag:lavfi.signalstats.VAVG=514|tag:lavfi.signalstats.VHIGH=514|tag:lavfi.signalstats.VMAX=514|tag:lavfi.signalstats.SATMIN=2|tag:lavfi.signalstats.SATLOW=2|tag:lavfi.signalstats.SATAVG=2|tag:lavfi.signalstats.SATHIGH=2|tag:lavfi.signalstats.SATMAX=2|tag:lavfi.signalstats.HUEMED=225|tag:lavfi.signalstats.HUEAVG=225|tag:lavfi.signalstats.YDIF=0|tag:lavfi.signalstats.UDIF=0|tag:lavfi.signalstats.VDIF=0|tag:lavfi.signalstats.YBITDEPTH=8|tag:lavfi.signalstats.VBITDEPTH=2 +pts=0|tag:lavfi.signalstats.UBITDEPTH=1|tag:lavfi.signalstats.YMIN=940|tag:lavfi.signalstats.YLOW=940|tag:lavfi.signalstats.YAVG=940|tag:lavfi.signalstats.YHIGH=940|tag:lavfi.signalstats.YMAX=940|tag:lavfi.signalstats.UMIN=512|tag:lavfi.signalstats.ULOW=512|tag:lavfi.signalstats.UAVG=512|tag:lavfi.signalstats.UHIGH=512|tag:lavfi.signalstats.UMAX=512|tag:lavfi.signalstats.VMIN=512|tag:lavfi.signalstats.VLOW=512|tag:lavfi.signalstats.VAVG=512|tag:lavfi.signalstats.VHIGH=512|tag:lavfi.signalstats.VMAX=512|tag:lavfi.signalstats.SATMIN=0|tag:lavfi.signalstats.SATLOW=0|tag:lavfi.signalstats.SATAVG=0|tag:lavfi.signalstats.SATHIGH=0|tag:lavfi.signalstats.SATMAX=0|tag:lavfi.signalstats.HUEMED=180|tag:lavfi.signalstats.HUEAVG=180|tag:lavfi.signalstats.YDIF=0|tag:lavfi.signalstats.UDIF=0|tag:lavfi.signalstats.VDIF=0|tag:lavfi.signalstats.YBITDEPTH=6|tag:lavfi.signalstats.VBITDEPTH=1 Test filter-metadata-signalstats-yuv420p10 failed. Look at tests/data/fate/filter-metadata-signalstats-yuv420p10.err for details. [...]
Michael Niedermayer: > On Sun, Jul 24, 2022 at 11:26:37PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Sat, Jul 23, 2022 at 11:42:23PM +0200, Andreas Rheinhardt wrote: >>>> Michael Niedermayer: >>>>> On Sat, Jul 23, 2022 at 07:44:40AM +0200, Andreas Rheinhardt wrote: >>>>>> Andreas Rheinhardt: >>>>>>> Michael Niedermayer: >>>>>>>> On Sat, Jul 02, 2022 at 08:32:06AM +0200, Andreas Rheinhardt wrote: >>>>>>>>> Michael Niedermayer: >>>>>>>>>> On Fri, Jul 01, 2022 at 12:29:45AM +0200, Andreas Rheinhardt wrote: >>>>>>>>>>> The HEVC decoder has both HEVCContext and HEVCLocalContext >>>>>>>>>>> structures. The latter is supposed to be the structure >>>>>>>>>>> containing the per-slicethread state. >>>>>>>>>>> >>>>>>>>>>> Yet up until now that is not how it is handled in practice: >>>>>>>>>>> Each HEVCLocalContext has a unique HEVCContext allocated for it >>>>>>>>>>> and each of these coincides except in exactly one field: The >>>>>>>>>>> corresponding HEVCLocalContext. This makes it possible to pass >>>>>>>>>>> the HEVCContext everywhere where logically a HEVCLocalContext >>>>>>>>>>> should be used. And up until recently, this is how it has been done. >>>>>>>>>>> >>>>>>>>>>> Yet the preceding patches changed this, making it possible >>>>>>>>>>> to avoid allocating redundant HEVCContexts. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>>>>>>>> --- >>>>>>>>>>> libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ >>>>>>>>>>> libavcodec/hevcdec.h | 2 -- >>>>>>>>>>> 2 files changed, 16 insertions(+), 26 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >>>>>>>>>>> index 9d1241f293..048fcc76b4 100644 >>>>>>>>>>> --- a/libavcodec/hevcdec.c >>>>>>>>>>> +++ b/libavcodec/hevcdec.c >>>>>>>>>>> @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>>>>>> { >>>>>>>>>>> HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; >>>>>>>>>>> const HEVCContext *const s = lc->parent; >>>>>>>>>>> - HEVCContext *s1 = avctxt->priv_data; >>>>>>>>>>> - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; >>>>>>>>>>> + int ctb_size = 1 << s->ps.sps->log2_ctb_size; >>>>>>>>>>> int more_data = 1; >>>>>>>>>>> int ctb_row = job; >>>>>>>>>>> - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); >>>>>>>>>>> - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>>>>>> - int thread = ctb_row % s1->threads_number; >>>>>>>>>>> + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); >>>>>>>>>>> + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; >>>>>>>>>>> + int thread = ctb_row % s->threads_number; >>>>>>>>>>> int ret; >>>>>>>>>>> >>>>>>>>>>> if(ctb_row) { >>>>>>>>>>> @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, >>>>>>>>>>> >>>>>>>>>>> ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); >>>>>>>>>>> >>>>>>>>>>> - if (atomic_load(&s1->wpp_err)) { >>>>>>>>>>> + if (atomic_load(&s->wpp_err)) { >>>>>>>>>>> ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); >>>>>>>>>> >>>>>>>>>> the consts in "const HEVCContext *const " make clang version 6.0.0-1ubuntu2 unhappy >>>>>>>>>> (this was building shared libs) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> CC libavcodec/hevcdec.o >>>>>>>>>> src/libavcodec/hevcdec.c:2574:13: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_int *' (aka 'const _Atomic(int) *') invalid) >>>>>>>>>> if (atomic_load(&s->wpp_err)) { >>>>>>>>>> ^ ~~~~~~~~~~~ >>>>>>>>>> /usr/lib/llvm-6.0/lib/clang/6.0.0/include/stdatomic.h:134:29: note: expanded from macro 'atomic_load' >>>>>>>>>> #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST) >>>>>>>>>> ^ ~~~~~~ >>>>>>>>>> 1 error generated. >>>>>>>>>> src/ffbuild/common.mak:81: recipe for target 'libavcodec/hevcdec.o' failed >>>>>>>>>> make: *** [libavcodec/hevcdec.o] Error 1 >>>>>>>>>> >>>>>>>>>> thx >>>>>>>>>> >>>>>>>>> >>>>>>>>> Thanks for testing this. atomic_load is indeed declared without const in >>>>>>>>> 7.17.7.2: >>>>>>>>> >>>>>>>>> C atomic_load(volatile A *object); >>>>>>>>> >>>>>>>>> Upon reflection this makes sense, because if atomics are implemented via >>>>>>>>> mutexes, even a read may involve a preceding write. So I'll cast const >>>>>>>>> away here, too, and add a comment. (It works when casting const away, >>>>>>>>> doesn't it?) >>>>>>>> >>>>>>>> This doesnt feel "right". These pointers should not be coming from a const >>>>>>>> if they are written to >>>>>>>> >>>>>>> >>>>>>> The HEVCContext is not const because the underlying object is const; the >>>>>>> HEVCContext is const when accessed from any part of the code that may be >>>>>>> run from slice threads, because if a slice thread modifies it, you have >>>>>>> a data race in case any of the other slice threads reads this field or >>>>>>> modifies it itself. But this is by definition not true for atomic >>>>>>> operations, so casting const away for them is fine. >>>>>>> >>>>>>>> The compiler accepts it with an explicit cast though. With an implicit cast >>>>>>>> it produces a warning >>>>>>>> >>>>>> >>>>>> Did the above explanation satisfy you? Or do you want something else? >>>>> >>>>> sure, ok >>>>> >>>>> [...] >>>>> >>>> >>>> Good to hear. This patchset (namely patch 11/18: "avcodec/hevcpred: Pass >>>> HEVCLocalContext when slice-threading") includes modifications to mips >>>> code that I created blindly. Can you please test it? Here is a branch of >>>> this rebased on top of current git master: >>>> https://github.com/mkver/FFmpeg/commits/hevc_wpp >>>> (Said branch actually contains a bit of further work which also modifies >>>> mips code (in particular, >>>> https://github.com/mkver/FFmpeg/commit/cf441e559b8d4bf2c05c29483ccf49e82fc6b863 >>>> does so); you may also test this.) >>> >>> what exact tests do we need ? >>> simple fate ? any specific thread type count ? >>> also note i can only test qemu mips not real hw. >>> I stopped maintaining the MIPS hw and sofar noone volunteered to take its >>> maintaince over >>> >> >> Compiling (in particular being on the lookout for new warnings) and a >> simple fate should be enough. > > > > I seethis: (NOT sanity checked and i also had some other patches from today applied) > Probably these are all unrelated nonsense > > --- fatelist-oldw 2022-07-25 21:19:24.555885524 +0200 > +++ fatelistw 2022-07-25 21:19:30.715956081 +0200 > @@ -1,3 +1,5 @@ > +src/tests/checkasm/synth_filter.c:66:42: warning: unused variable ‘offset_b’ [-Wunused-variable] > +src/tests/checkasm/sbrdsp.c:180:14: warning: unused variable ‘bw’ [-Wunused-variable] > src/libavfilter/dnn/dnn_backend_common.c:94:11: warning: unused variable ‘status’ [-Wunused-variable] > src/libavfilter/dnn/dnn_backend_common.c:114:11: warning: unused variable ‘status’ [-Wunused-variable] > src/libavfilter/dnn/dnn_backend_common.c:80:14: warning: ‘async_thread_routine’ defined but not used [-Wunused-function] > @@ -7,20 +9,20 @@ > src/libavformat/dashenc.c:1960:63: warning: ‘%s’ directive output may be truncated writing up to 1023 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/dashenc.c:492:49: warning: ‘media_’ directive output may be truncated writing 6 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/dashenc.c:2257:59: warning: ‘%s’ directive output may be truncated writing up to 1023 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > -src/libavformat/matroskaenc.c:3074:58: warning: ‘%012.9f’ directive output may be truncated writing between 12 and 320 bytes into a region of size between 8 and 14 [-Wformat-truncation=] > src/libavformat/mlvdec.c:361:63: warning: ‘__builtin___snprintf_chk’ output may be truncated before the last format character [-Wformat-truncation=] > -src/libavformat/movenc.c:1126:8: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow] > +src/libavformat/matroskaenc.c:3074:58: warning: ‘%012.9f’ directive output may be truncated writing between 12 and 320 bytes into a region of size between 8 and 14 [-Wformat-truncation=] > src/libavformat/smoothstreamingenc.c:509:49: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/smoothstreamingenc.c:544:63: warning: ‘/FragmentInfo(’ directive output may be truncated writing 14 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/smoothstreamingenc.c:545:63: warning: ‘/Fragments(’ directive output may be truncated writing 11 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/smoothstreamingenc.c:537:53: warning: ‘/temp’ directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=] > src/libavformat/vorbiscomment.c:103:63: warning: ‘%03d’ directive output may be truncated writing between 3 and 10 bytes into a region of size 4 [-Wformat-truncation=] > src/libavformat/vorbiscomment.c:104:69: warning: ‘%02d’ directive output may be truncated writing between 2 and 3 bytes into a region of size between 1 and 7 [-Wformat-truncation=] > +src/libavformat/movenc.c:1126:8: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow] > src/libavcodec/mips/aacsbr_mips.h:62:13: warning: ‘sbr_qmf_analysis_mips’ defined but not used [-Wunused-function] > src/libavcodec/dvenc.c:786:81: warning: array subscript is above array bounds [-Warray-bounds] > src/libavcodec/dvenc.c:786:81: warning: array subscript is above array bounds [-Warray-bounds] > src/libavcodec/ffv1dec.c:999:13: warning: ‘copy_fields’ defined but not used [-Wunused-function] > -src/libavcodec/hevcdec.c:3540:12: warning: ‘hevc_ref_frame’ defined but not used [-Wunused-function] > +src/libavcodec/hevcdec.c:3529:12: warning: ‘hevc_ref_frame’ defined but not used [-Wunused-function] > src/libavcodec/mobiclip.c:471:24: warning: array subscript is above array bounds [-Warray-bounds] > src/libavcodec/mobiclip.c:475:16: warning: array subscript is above array bounds [-Warray-bounds] > src/libavcodec/mobiclip.c:471:24: warning: array subscript is above array bounds [-Warray-bounds] > @@ -41,11 +43,11 @@ > src/libavcodec/pcm-bluray.c:190:49: warning: passing argument 2 of ‘bytestream2_get_buffer’ from incompatible pointer type [-Wincompatible-pointer-types] > src/libavcodec/pcm-dvd.c:160:37: warning: passing argument 2 of ‘bytestream2_get_buffer’ from incompatible pointer type [-Wincompatible-pointer-types] > src/libavcodec/qdm2.c:1002:47: warning: array subscript is above array bounds [-Warray-bounds] > -src/libavcodec/vp8.c:2373:30: warning: variable ‘next_td’ set but not used [-Wunused-but-set-variable] > -src/libavcodec/vp8.c:2586:37: warning: unused variable ‘prev_td’ [-Wunused-variable] > -src/libavcodec/vp8.c:2586:20: warning: unused variable ‘next_td’ [-Wunused-variable] > -src/libavcodec/vp8.c:109:12: warning: ‘vp8_ref_frame’ defined but not used [-Wunused-function] > -src/libavcodec/vp9.c:1795:9: warning: unused variable ‘ret’ [-Wunused-variable] > src/libavutil/timecode.c:123:60: warning: ‘%0*d’ directive output may be truncated writing between 1 and 10 bytes into a region of size between 2 and 14 [-Wformat-truncation=] > +src/libavcodec/vp9.c:1795:9: warning: unused variable ‘ret’ [-Wunused-variable] > src/fftools/ffprobe.c:333:11: warning: unused variable ‘new_log_buffer’ [-Wunused-variable] > src/fftools/ffprobe.c:329:14: warning: unused variable ‘avc’ [-Wunused-variable] > +src/libavcodec/vp8.c:2373:30: warning: variable ‘next_td’ set but not used [-Wunused-but-set-variable] > +src/libavcodec/vp8.c:2587:37: warning: unused variable ‘prev_td’ [-Wunused-variable] > +src/libavcodec/vp8.c:2587:20: warning: unused variable ‘next_td’ [-Wunused-variable] > +src/libavcodec/vp8.c:109:12: warning: ‘vp8_ref_frame’ defined but not used [-Wunused-function] > > > There is also the fate-filter-metadata-signalstats-yuv420p10 test > which fails before and afterwards > -pts=0|tag:lavfi.signalstats.UBITDEPTH=2|tag:lavfi.signalstats.YMIN=943|tag:lavfi.signalstats.YLOW=943|tag:lavfi.signalstats.YAVG=943|tag:lavfi.signalstats.YHIGH=943|tag:lavfi.signalstats.YMAX=943|tag:lavfi.signalstats.UMIN=514|tag:lavfi.signalstats.ULOW=514|tag:lavfi.signalstats.UAVG=514|tag:lavfi.signalstats.UHIGH=514|tag:lavfi.signalstats.UMAX=514|tag:lavfi.signalstats.VMIN=514|tag:lavfi.signalstats.VLOW=514|tag:lavfi.signalstats.VAVG=514|tag:lavfi.signalstats.VHIGH=514|tag:lavfi.signalstats.VMAX=514|tag:lavfi.signalstats.SATMIN=2|tag:lavfi.signalstats.SATLOW=2|tag:lavfi.signalstats.SATAVG=2|tag:lavfi.signalstats.SATHIGH=2|tag:lavfi.signalstats.SATMAX=2|tag:lavfi.signalstats.HUEMED=225|tag:lavfi.signalstats.HUEAVG=225|tag:lavfi.signalstats.YDIF=0|tag:lavfi.signalstats.UDIF=0|tag:lavfi.signalstats.VDIF=0|tag:lavfi.signalstats.YBITDEPTH=8|tag:lavfi.signalstats.VBITDEPTH=2 > +pts=0|tag:lavfi.signalstats.UBITDEPTH=1|tag:lavfi.signalstats.YMIN=940|tag:lavfi.signalstats.YLOW=940|tag:lavfi.signalstats.YAVG=940|tag:lavfi.signalstats.YHIGH=940|tag:lavfi.signalstats.YMAX=940|tag:lavfi.signalstats.UMIN=512|tag:lavfi.signalstats.ULOW=512|tag:lavfi.signalstats.UAVG=512|tag:lavfi.signalstats.UHIGH=512|tag:lavfi.signalstats.UMAX=512|tag:lavfi.signalstats.VMIN=512|tag:lavfi.signalstats.VLOW=512|tag:lavfi.signalstats.VAVG=512|tag:lavfi.signalstats.VHIGH=512|tag:lavfi.signalstats.VMAX=512|tag:lavfi.signalstats.SATMIN=0|tag:lavfi.signalstats.SATLOW=0|tag:lavfi.signalstats.SATAVG=0|tag:lavfi.signalstats.SATHIGH=0|tag:lavfi.signalstats.SATMAX=0|tag:lavfi.signalstats.HUEMED=180|tag:lavfi.signalstats.HUEAVG=180|tag:lavfi.signalstats.YDIF=0|tag:lavfi.signalstats.UDIF=0|tag:lavfi.signalstats.VDIF=0|tag:lavfi.signalstats.YBITDEPTH=6|tag:lavfi.signalstats.VBITDEPTH=1 > Test filter-metadata-signalstats-yuv420p10 failed. Look at tests/data/fate/filter-metadata-signalstats-yuv420p10.err for details. > > Thanks for your work. Nothing of the above is related to my patches (e.g. the hevcdec warning is due to you not having threads enabled). So I will apply these patches now. - Andreas
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index 9d1241f293..048fcc76b4 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -2548,13 +2548,12 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, { HEVCLocalContext *lc = ((HEVCLocalContext**)hevc_lclist)[self_id]; const HEVCContext *const s = lc->parent; - HEVCContext *s1 = avctxt->priv_data; - int ctb_size = 1<< s1->ps.sps->log2_ctb_size; + int ctb_size = 1 << s->ps.sps->log2_ctb_size; int more_data = 1; int ctb_row = job; - int ctb_addr_rs = s1->sh.slice_ctb_addr_rs + ctb_row * ((s1->ps.sps->width + ctb_size - 1) >> s1->ps.sps->log2_ctb_size); - int ctb_addr_ts = s1->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; - int thread = ctb_row % s1->threads_number; + int ctb_addr_rs = s->sh.slice_ctb_addr_rs + ctb_row * ((s->ps.sps->width + ctb_size - 1) >> s->ps.sps->log2_ctb_size); + int ctb_addr_ts = s->ps.pps->ctb_addr_rs_to_ts[ctb_addr_rs]; + int thread = ctb_row % s->threads_number; int ret; if(ctb_row) { @@ -2572,7 +2571,7 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP); - if (atomic_load(&s1->wpp_err)) { + if (atomic_load(&s->wpp_err)) { ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP); return 0; } @@ -2595,7 +2594,8 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, ff_hevc_hls_filters(lc, x_ctb, y_ctb, ctb_size); if (!more_data && (x_ctb+ctb_size) < s->ps.sps->width && ctb_row != s->sh.num_entry_point_offsets) { - atomic_store(&s1->wpp_err, 1); + /* Casting const away here is safe, because it is an atomic operation. */ + atomic_store((atomic_int*)&s->wpp_err, 1); ff_thread_report_progress2(s->avctx, ctb_row ,thread, SHIFT_CTB_WPP); return 0; } @@ -2617,7 +2617,8 @@ static int hls_decode_entry_wpp(AVCodecContext *avctxt, void *hevc_lclist, return 0; error: s->tab_slice_address[ctb_addr_rs] = -1; - atomic_store(&s1->wpp_err, 1); + /* Casting const away here is safe, because it is an atomic operation. */ + atomic_store((atomic_int*)&s->wpp_err, 1); ff_thread_report_progress2(s->avctx, ctb_row ,thread, SHIFT_CTB_WPP); return ret; } @@ -2647,18 +2648,15 @@ static int hls_slice_data_wpp(HEVCContext *s, const H2645NAL *nal) ff_alloc_entries(s->avctx, s->sh.num_entry_point_offsets + 1); for (i = 1; i < s->threads_number; i++) { - if (s->sList[i] && s->HEVClcList[i]) + if (s->HEVClcList[i]) continue; - av_freep(&s->sList[i]); - av_freep(&s->HEVClcList[i]); - s->sList[i] = av_malloc(sizeof(HEVCContext)); s->HEVClcList[i] = av_mallocz(sizeof(HEVCLocalContext)); - if (!s->sList[i] || !s->HEVClcList[i]) { + if (!s->HEVClcList[i]) { res = AVERROR(ENOMEM); goto error; } s->HEVClcList[i]->logctx = s->avctx; - s->HEVClcList[i]->parent = s->sList[i]; + s->HEVClcList[i]->parent = s; } offset = (lc->gb.index >> 3); @@ -2697,10 +2695,8 @@ static int hls_slice_data_wpp(HEVCContext *s, const H2645NAL *nal) s->data = data; for (i = 1; i < s->threads_number; i++) { - memcpy(s->sList[i], s, sizeof(HEVCContext)); - s->sList[i]->HEVClc = s->HEVClcList[i]; - s->sList[i]->HEVClc->first_qp_group = 1; - s->sList[i]->HEVClc->qp_y = s->sList[0]->HEVClc->qp_y; + s->HEVClcList[i]->first_qp_group = 1; + s->HEVClcList[i]->qp_y = s->HEVClc->qp_y; } atomic_store(&s->wpp_err, 0); @@ -3613,15 +3609,13 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) av_freep(&s->sh.offset); av_freep(&s->sh.size); - if (s->HEVClcList && s->sList) { + if (s->HEVClcList) { for (i = 1; i < s->threads_number; i++) { av_freep(&s->HEVClcList[i]); - av_freep(&s->sList[i]); } } av_freep(&s->HEVClc); av_freep(&s->HEVClcList); - av_freep(&s->sList); ff_h2645_packet_uninit(&s->pkt); @@ -3639,13 +3633,11 @@ static av_cold int hevc_init_context(AVCodecContext *avctx) s->HEVClc = av_mallocz(sizeof(HEVCLocalContext)); s->HEVClcList = av_mallocz(sizeof(HEVCLocalContext*) * s->threads_number); - s->sList = av_mallocz(sizeof(HEVCContext*) * s->threads_number); - if (!s->HEVClc || !s->HEVClcList || !s->sList) + if (!s->HEVClc || !s->HEVClcList) return AVERROR(ENOMEM); s->HEVClc->parent = s; s->HEVClc->logctx = avctx; s->HEVClcList[0] = s->HEVClc; - s->sList[0] = s; s->cabac = av_malloc(sizeof(*s->cabac)); if (!s->cabac) diff --git a/libavcodec/hevcdec.h b/libavcodec/hevcdec.h index b33e8ae0f6..3367ee312a 100644 --- a/libavcodec/hevcdec.h +++ b/libavcodec/hevcdec.h @@ -476,8 +476,6 @@ typedef struct HEVCContext { const AVClass *c; // needed by private avoptions AVCodecContext *avctx; - struct HEVCContext **sList; - HEVCLocalContext **HEVClcList; HEVCLocalContext *HEVClc;
The HEVC decoder has both HEVCContext and HEVCLocalContext structures. The latter is supposed to be the structure containing the per-slicethread state. Yet up until now that is not how it is handled in practice: Each HEVCLocalContext has a unique HEVCContext allocated for it and each of these coincides except in exactly one field: The corresponding HEVCLocalContext. This makes it possible to pass the HEVCContext everywhere where logically a HEVCLocalContext should be used. And up until recently, this is how it has been done. Yet the preceding patches changed this, making it possible to avoid allocating redundant HEVCContexts. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/hevcdec.c | 40 ++++++++++++++++------------------------ libavcodec/hevcdec.h | 2 -- 2 files changed, 16 insertions(+), 26 deletions(-)