Re: [FFmpeg-devel] [PATCH 09/18] avcodec/hevcdec: Add stat_coeffs to HEVCABACState

[Anton Khirnov <anton@khirnov.net>]

Quoting Andreas Rheinhardt (2022-07-01 00:29:40)
> The HEVC decoder has both HEVCContext and HEVCLocalContext
> structures. The latter is supposed to be the structure
> containing the per-slicethread state.
> 
> Yet that is not how it is handled in practice: Each HEVCLocalContext
> has a unique HEVCContext allocated for it and each of these
> coincides with the main HEVCContext except in exactly one field:
> The corresponding HEVCLocalContext.
> This makes it possible to pass the HEVCContext everywhere where
> logically a HEVCLocalContext should be used.
> 
> This led to confusion in the first version of what eventually became
> commit c8bc0f66a875bc3708d8dc11b757f2198606ffd7:
> Before said commit, the initialization of the Rice parameter derivation
> state was incorrect; the fix for single-threaded as well as
> frame-threaded decoding was to add backup stats to HEVCContext
> that are used when the cabac state is updated*, see
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/268861.html
> Yet due to what has been said above, this does not work for
> slice-threading, because the each HEVCLocalContext has its own
> HEVCContext, so the Rice parameter state would not be transferred
> between threads.
> 
> This is fixed in c8bc0f66a875bc3708d8dc11b757f2198606ffd7
> by a hack: It rederives what the previous thread was and accesses
> the corresponding HEVCContext.
> 
> Fix this by treating the Rice parameter state the same way
> the ordinary CABAC parameters are shared between threads:
> Make them part of the same struct that is shared between
> slice threads. This does not cause races, because
> the parts of the code that access these Rice parameters
> are a subset of the parts of code that access the CABAC parameters.
> 
> *: And if the persistent_rice_adaptation_enabled_flag is set.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/hevc_cabac.c | 17 ++++++++---------
>  libavcodec/hevcdec.c    | 10 +++++-----
>  libavcodec/hevcdec.h    | 10 +++++++---
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/hevc_cabac.c b/libavcodec/hevc_cabac.c
> index a194f8a02a..985c97ef2a 100644
> --- a/libavcodec/hevc_cabac.c
> +++ b/libavcodec/hevc_cabac.c
> @@ -453,19 +453,18 @@ void ff_hevc_save_states(HEVCContext *s, int ctb_addr_ts)
>          (ctb_addr_ts % s->ps.sps->ctb_width == 2 ||
>           (s->ps.sps->ctb_width == 2 &&
>            ctb_addr_ts % s->ps.sps->ctb_width == 0))) {
> -        memcpy(s->cabac_state, s->HEVClc->cabac_state, HEVC_CONTEXTS);
> +        memcpy(s->cabac->state, s->HEVClc->cabac_state, HEVC_CONTEXTS);

So if I'm reading this right, this copies the per-slice-context state
into the decoder-global state. And it's done from slice threads with no
locks. So how is this not racy?

-- 
Anton Khirnov
_______________________________________________
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".