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

[Anton Khirnov]

Quoting Michael Niedermayer (2024-07-18 16:31:51)
> On Thu, Jul 18, 2024 at 10:20:09AM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-07-18 00:32:38)
> > > the data for each decoder task should be together and not scattered around
> > > more than needed, reducing cache efficiency
> > > 
> > > 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
> > 
> 
> > A nice theory,
> 
> that doesnt sound like a friendly description

And this does not look like a friendly review. I am getting a very
strong impression that you're looking for reasons to object to the
patches, rather seriously review them.

> > but in practice
> 
> > this patchset
> 
> "this patchset" isnt "this patch", nor does any improvment from "this patchset"
> depend on the change of "this patch"
> In fact it would probably benefit from droping this patch

Just what would that benefit be?

Storing and copying around multiple copies of the same data is generally
bad for readability, as it requires more cognitive effort to understand
the code - which is why I wrote this patch in the first place. It is
also inefficient in terms of overall memory use and cache utilization,
but I expect the impact of that to be small.

If you're concerned about dereferencing a pointer in an inner loop, I
can add a temporary variable to decode_line() as below, but removing
duplicated data seems like an unambiguous improvement to me.

diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c
index 97a28b085a..d68bbda5be 100644
--- a/libavcodec/ffv1dec_template.c
+++ b/libavcodec/ffv1dec_template.c
@@ -30,6 +30,7 @@ RENAME(decode_line)(FFV1Context *f,
 {
     PlaneContext *const p = &s->plane[plane_index];
     RangeCoder *const c   = &s->c;
+    const int16_t (*quant_table)[256] = f->quant_tables[p->quant_table_index];
     int x;
     int run_count = 0;
     int run_mode  = 0;
@@ -59,7 +60,7 @@ RENAME(decode_line)(FFV1Context *f,
                 return AVERROR_INVALIDDATA;
         }

-        context = RENAME(get_context)(f->quant_tables[p->quant_table_index],
+        context = RENAME(get_context)(quant_table,
                                       sample[1] + x, sample[0] + x, sample[1] + x);
         if (context < 0) {
             context = -context;

-- 
Anton Khirnov