[FFmpeg-devel] [PATCH 13/39] lavc/ffv1: drop redundant PlaneContext.quant_table

[Michael Niedermayer]

On Tue, Jul 16, 2024 at 07:11:28PM +0200, Anton Khirnov wrote:
> It is a copy of FFV1Context.quant_tables[quant_table_index].
> ---
>  libavcodec/ffv1.h             |  1 -
>  libavcodec/ffv1_template.c    | 22 +++++++++++-----------
>  libavcodec/ffv1dec.c          | 28 ++++++++++++----------------
>  libavcodec/ffv1dec_template.c | 14 +++++++++-----
>  libavcodec/ffv1enc.c          | 24 ++++++++++++------------
>  libavcodec/ffv1enc_template.c | 13 ++++++++-----
>  6 files changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
> index 4d57172d5b..a87c2d2a36 100644
> --- a/libavcodec/ffv1.h
> +++ b/libavcodec/ffv1.h
> @@ -59,7 +59,6 @@ typedef struct VlcState {
>  } VlcState;
>  
>  typedef struct PlaneContext {
> -    int16_t quant_table[MAX_CONTEXT_INPUTS][256];
>      int quant_table_index;
>      int context_count;
>      uint8_t (*state)[CONTEXT_SIZE];
> diff --git a/libavcodec/ffv1_template.c b/libavcodec/ffv1_template.c
> index c5f61b0182..d15ad11021 100644
> --- a/libavcodec/ffv1_template.c
> +++ b/libavcodec/ffv1_template.c
> @@ -29,25 +29,25 @@ static inline int RENAME(predict)(TYPE *src, TYPE *last)
>      return mid_pred(L, L + T - LT, T);
>  }
>  
> -static inline int RENAME(get_context)(PlaneContext *p, TYPE *src,
> -                                      TYPE *last, TYPE *last2)
> +static inline int RENAME(get_context)(const int16_t quant_table[MAX_CONTEXT_INPUTS][256],
> +                                      TYPE *src, TYPE *last, TYPE *last2)
>  {
>      const int LT = last[-1];
>      const int T  = last[0];
>      const int RT = last[1];
>      const int L  = src[-1];
>  
> -    if (p->quant_table[3][127] || p->quant_table[4][127]) {
> +    if (quant_table[3][127] || quant_table[4][127]) {

the data for each decoder task should be together and not scattered around
more than needed, reducing cache efficiency


[...]
> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
> index 66d9f63c1a..618020d10f 100644
> --- a/libavcodec/ffv1dec.c
> +++ b/libavcodec/ffv1dec.c
> @@ -117,7 +117,8 @@ static int is_input_end(FFV1Context *s, GetBitContext *gb)
>  #define RENAME(name) name ## 32
>  #include "ffv1dec_template.c"
>  
> -static int decode_plane(FFV1Context *s, FFV1SliceContext *sc,
> +static int decode_plane(FFV1Context *f,
> +                        FFV1Context *s, FFV1SliceContext *sc,

[...]
>  static av_always_inline int
> -RENAME(decode_line)(FFV1Context *s, FFV1SliceContext *sc, GetBitContext *gb,
> +RENAME(decode_line)(FFV1Context *f,
> +                    FFV1Context *s, FFV1SliceContext *sc,
> +                    GetBitContext *gb,
>                      int w, TYPE *sample[2], int plane_index, int bits)
>  {
>      PlaneContext *const p = &s->plane[plane_index];
> @@ -57,7 +59,8 @@ RENAME(decode_line)(FFV1Context *s, FFV1SliceContext *sc, GetBitContext *gb,
>                  return AVERROR_INVALIDDATA;
>          }
>  
> -        context = RENAME(get_context)(p, sample[1] + x, sample[0] + x, sample[1] + x);
> +        context = RENAME(get_context)(f->quant_tables[p->quant_table_index],
> +                                      sample[1] + x, sample[0] + x, sample[1] + x);

putting all this extra code in the inner per pixel loop is not ok
especially not for the sake of avoiding a memcpy of a few hundread bytes multiple levels of loops outside

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240718/ec160719/attachment.sig>