Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
>>> ---
>>> 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(>wpp_err)) {
>>> +if (atomic_load(>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(>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".
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> > ---
> > 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(>wpp_err)) {
> > +if (atomic_load(>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(>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
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> ---
> 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(>wpp_err)) {
> +if (atomic_load(>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(>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
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> >>> ---
> >>> 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(>wpp_err)) {
> >>> +if (atomic_load(>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(>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
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
>>> ---
>>> 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(>wpp_err)) {
>>> +if (atomic_load(>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(>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
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> > ---
> > 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(>wpp_err)) {
> > +if (atomic_load(>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(>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
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> ---
> 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(>wpp_err)) {
> +if (atomic_load(>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(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
---
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(>wpp_err)) {
+if (atomic_load(>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(>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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> >> ---
> >> 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(>wpp_err)) {
> >> +if (atomic_load(>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(>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 GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
>> ---
>> 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(>wpp_err)) {
>> +if (atomic_load(>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)
>
>
> CClibavcodec/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(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
> ---
> 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(>wpp_err)) {
> +if (atomic_load(>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(>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 GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts
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
---
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(>wpp_err)) {
+if (atomic_load(>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(>wpp_err, 1);
+/* Casting const away here is safe, because it is an atomic
operation. */
+atomic_store((atomic_int*)>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(>wpp_err, 1);
+/* Casting const away here is safe, because it is an atomic operation. */
+atomic_store((atomic_int*)>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(>sList[i]);
-av_freep(>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(>wpp_err, 0);
@@ -3613,15 +3609,13 @@ static av_cold int hevc_decode_free(AVCodecContext
*avctx)
av_freep(>sh.offset);
av_freep(>sh.size);
-if (s->HEVClcList && s->sList) {
+if (s->HEVClcList) {
for (i = 1; i < s->threads_number; i++) {
av_freep(>HEVClcList[i]);
-av_freep(>sList[i]);
}
}
av_freep(>HEVClc);
