[FFmpeg-devel] [PATCH 4/5] avcodec/on2avc: Avoid indirection when calling float dsp function

[Anton Khirnov]

Quoting Andreas Rheinhardt (2020-10-21 07:37:03)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2020-10-19 13:07:05)
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> >> ---
> >>  libavcodec/on2avc.c | 35 ++++++++++++++++++-----------------
> >>  1 file changed, 18 insertions(+), 17 deletions(-)
> >>
> > 
> > I don't quite see the point of this. I cannot imagine it is measurably
> > faster, the memory savings should also be negligible.
> > And strictly speaking, it is API abuse as the functions are tied to the
> > context. It is conceivable that they could be compiled at runtime and
> > freeing the context would invalidate them.
> > 
> Seems like we disagree here: I don't think that something needs to be
> measurably faster for it to be done; I don't think that small memory
> savings are negligible.

Yeah, I guess we disagree. In my view, the "baseline" value (to the
project) of any change is negative, because it necessarily adds
- noise in git history
- noise on the ML
- a risk of introducing a new bug
IMO to counterbalance these, every change should overcome some minimum
threshold of usefulness in order to rise above the noise.

Saving something like 80 bytes per decoder instance is never ever going
to have a practical impact on anything. Same for potentially saving a
few cycles for the extra pointer dereference. Especially since this is a
rather obscure decoder few people use. So in my view the change is just
not useful enough to be worth it. But then again that's just my opinion,
so feel free to contest it if you care enough.

> And I absolutely don't think it to be API abuse:
> The API does not say that these functions are tied to the context.

It doesn't, but the usual convention is that objects are owned by their
parent unless specifically documented otherwise.

> And I'd really be interested to know how freeing the context could
> somehow invalidate the functions -- after all, the AVFloatDSPContext
> doesn't have a custom free function.

Right, I missed that fact.

-- 
Anton Khirnov