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

[Alexander Strasser]

On 2023-11-09 11:13 +0100, Anton Khirnov wrote:
> 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?

First this:

> > > > 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

Second there was an argument for compliance:

> > > > 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

Third there was no rejection of the change, but a request for
measurement of the effect. I would expect an approval of the patch
if the measurement leads to insignificant enough results.


> > 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.

I can't see how the reason for the presence of code can be ultimately
defined objectively and non-arbitrary.


  Alexander