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

[Alexander Strasser]

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.

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.


Best regards,
  Alexander