[FFmpeg-devel] [PATCH] avcodec/mpegvideo: Remove spec-incompliant inverse quantisation

[Anton Khirnov]

Quoting Alexander Strasser (2023-11-08 21:55:10)
> On 2023-11-08 12:40 +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2023-10-31 09:40:44)
> > > On Mon, Oct 30, 2023 at 02:11:27PM +0100, Andreas Rheinhardt wrote:
> > > > Section 7.4.4 of the MPEG-2 specifications requires that the
> > > > last bit of the last coefficient be toggled so that the sum
> > > > of all coefficients is odd; both our decoder and encoder
> > > > did this only if the bitexact flag has been set (although
> > > > stuff like this should be behind AV_CODEC_FLAG2_FAST).
> > > > This patch changes this by removing the spec-incompliant
> > > > functions.
> > >
> > > This commit message should include benchamarks documenting the speed loss
> > > (of the unquantize, the IDCT and overall)
> > > It is expected that the speed of some IDCTs will be impacted negativly
> > > as the non zero terms will prevent the skiping of some significant code
> > >
> > > as well as information about how much PSNR improves (to the encoder input)
> > >
> > > Also the change is a +-1 in one spot before the IDCT, the IDCT is not bitexactly
> > > specified in MPEG-2 so one could think of this as a
> > > correct implementation followed by a IDCT that was sometimes +-1 off
> > > instead of spec non compliance
> > >
> > > Only after the benchmarks and PSNR is presented should we decide if this
> > > is a change we want
> >
> > I disagree that the burden of proof should be on Andreas here. It should
> > be up to whoever wants to keep this code to show that it is useful.
> 
> There was an argument presented.

I see no argument for why the code in question is useful, can you point
to the exact text?

> That argument could be challenged or otherwise explained why it more
> important to have this always behave like with bitexact.
> 
> This could lead to "OK, I think removal is better" or if not benchmarks
> could lead to one or the other decision.
> 
> Saying the burden is on whoever wants to keep the code sounds like a way
> for arbitrary code removal. While I agree getting rid of code can be a good
> thing, this would definitely take it too far.

All code is a maintenance burden, therefore all code should have a
reason for its presence in the codebase, otherwise it should be removed.

-- 
Anton Khirnov