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

[Vittorio Giovara]

On Wed, Nov 8, 2023 at 3:46 PM Alexander Strasser <eclipse7 at gmx.net> wrote:

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

To be fair, this is noncompliant spec code, it shouldn't be present at all
since it produces inconsistent results (with the spec) and there is no
device particularly needing this functionality.It's not arbitrary code
removal, it's removing something that is not needed any more since the
speed impact (pro or against) is negligible on modern computers.

I'm of the opinion that presenting an argument against such a targeted and
specific code removal with no supportive use case should be noted but not
acted upon, until relevant proof is brought over. Yes sadly that burden
should fall on whoever is presenting the argument.
-- 
Vittorio