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

[Alexander Strasser]

On 2023-11-08 17:58 -0500, Vittorio Giovara wrote:
> 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.

Please see the reply to Anton. I hope it's clearer now what I meant.


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

I wouid disagree in general.

Maybe this case is more special (targeted and specific?) then my
current understanding of the matter. IIUC the leading reason for
this patch is removing the code because of spec incompliance which
I think is not so clear and could be argued both ways.


  Alexander