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
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
