Re: [FFmpeg-devel] [PATCH 14/18] avcodec/hevcdec: Don't allocate redundant HEVCContexts

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".