[FFmpeg-devel] ffvorbis, inverted output

[Siarhei Siamashka]

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?

> besides this iam fine with the patch given that
> * it works with all optimized (i)mdcts we have now

None of (i)mdcts from ffmpeg should have any problems. But in the case I
missed something, this can be verified with a slightly modified build of
fft-test (something like the patch attached).

> * it does not conflict with planned optimizations

It does not conflict with the optimizations planned by me, but
can't say for the others. Can anybody share their plans?

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

> * others (loren?) have no objections

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffttest-scaled.diff
Type: text/x-diff
Size: 2890 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080929/8d32fb71/attachment.diff>