[FFmpeg-devel] ffvorbis, inverted output

[Siarhei Siamashka]

On Monday 29 September 2008, Michael Niedermayer wrote:
> On Mon, Sep 29, 2008 at 09:42:35AM +0300, Siarhei Siamashka wrote:
> > On Sunday 28 September 2008, Michael Niedermayer wrote:
> > > On Sat, Sep 27, 2008 at 09:59:21PM +0300, Siarhei Siamashka wrote:
> > > > On Sunday 14 September 2008, Michael Niedermayer wrote:
> > > > > On Sun, Sep 14, 2008 at 04:50:59AM +0300, Siarhei Siamashka wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I tried to do PSNR comparison of libvorbis/ffvorbis/tremor and
> > > > > > noticed that output from ffvorbis is actually inverted (ex.
> > > > > > output 0xFFFF in libvorbis corresponds to 0x0001 in ffvorbis and
> > > > > > so on) when compared to the output from the other decoders.
> > > > > >
> > > > > > Should this be fixed?
> > > > >
> > > > > yes, if all (/most) other vorbis decoders match and we differ from
> > > > > that
> > > >
> > > > Can these two patches be used as a fix? The first one adds support
> > > > for scaled imdct output. The second one inverts output of the decoder
> > > > (to match libvorbis and tremor) using negative scale factor.
> > > >
> > > > As additional bonus, 'copy_normalize' function from vorbis decoder is
> > > > simplified. Though I get some inconsistent benchmark results
> > > > (performance difference is negligible with one or another variant
> > > > getting ahead randomly) and would like someone to confirm that there
> > > > is no performance regression.
> > > >
> > > > Getting scaled imdct output involves sqrt operation and scale factor
> > > > uses odd power of two, so there is some difference in PSNR compared
> > > > to SVN trunk (taking inversion into account):
> > > >
> > > > stddev:    0.02 PSNR:127.97 bytes: 22057216/ 22057216
> > >
> > > [...]
> > >
> > > > @@ -98,6 +108,11 @@
> > > >      return -1;
> > > >  }
> > > >
> > > > +int ff_mdct_init(MDCTContext *s, int nbits, int inverse)
> > > > +{
> > > > +    return ff_mdct_init_scaled(s, nbits, inverse, 1.0);
> > > > +}
> > > > +
> > > >  /* complex multiplication: p = a * b */
> > > >  #define CMUL(pre, pim, are, aim, bre, bim) \
> > > >  {\
> > >
> > > i think its better to just add scale to ff_mdct_init()
> >
> > OK, should I resubmit a patch to also include changes for all
> > the other occurrences of 'ff_mdct_init' to use scale factor 1.0?
>
> yes, please

Updated patch series attached.

[...]

> > > * it does not break the regression tests
> >
> > This patch does not break the regression tests. It is a basic rule for
> > submitting any patches AFAIK.
>
> yes, i just explicitly asked because of the wma mdct windows float breakage

The change to (i)mdct does not affect regression test results for the other
codecs. But ffvorbis itself is not covered by regression tests, so I only
tested it using md5sum and tiny_psnr by comparing its output to the output
generated by older revisions, original noncompressed file, libvorbis, tremor.

Probably something that can be used in regression tests with the tolerance to
small output changes might be useful.

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-for-getting-i-MDCT-output-multiplied-by-a-c.patch
Type: text/x-diff
Size: 10085 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081003/1cdf982c/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Support-for-testing-i-MDCT-output-scale-factor-in-f.patch
Type: text/x-diff
Size: 3420 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081003/1cdf982c/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Fix-for-a-problem-with-inverted-sign-of-output-data.patch
Type: text/x-diff
Size: 1415 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081003/1cdf982c/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Use-iMDCT-output-scaling-to-simplify-ffvorbis-and-ma.patch
Type: text/x-diff
Size: 2624 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081003/1cdf982c/attachment-0003.patch>