[FFmpeg-devel] ffvorbis, inverted output

[Michael Niedermayer]

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


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

i have no *mdct 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.

yes, i just explicitly asked because of the wma mdct windows float breakage

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080929/476fc305/attachment.pgp>