[FFmpeg-devel] [PATCH] Fix for inverted sign in ffvorbis audio?output (resubmit)

[Siarhei Siamashka]

On Sunday 10 May 2009, Reimar D?ffinger wrote:
> On Sun, May 10, 2009 at 03:36:13PM +0300, Siarhei Siamashka wrote:
> > Could you elaborate? How can code duplication be avoided here without
> > obviously sacrificing performance (moving 'if' in the inner loop)?
>
> First, that function is av_cold, so I _very_ much hope it is not even
> remotely performance-critical.

So do I. And if it really turns out to be a performance problem for any
specific case, it can be always replaced with a hardcoded table.

> But what I meant is that in
>
> > -    for(i=0;i<n4;i++) {
> > -        alpha = 2 * M_PI * (i + 1.0 / 8.0) / n;
> > -        s->tcos[i] = -cos(alpha);
> > -        s->tsin[i] = -sin(alpha);
> > +    if (scale < 0.0) {
> > +        scale = sqrt(-scale);
> > +        for(i=0;i<n4;i++) {
> > +            alpha = 2 * M_PI * (i + 1.0 / 8.0) / n;
> > +            s->tcos[i] = sin(alpha) * scale;
> > +            s->tsin[i] = -cos(alpha) * scale;
> > +        }
> > +    } else {
> > +        scale = sqrt(scale);
> > +        for(i=0;i<n4;i++) {
> > +            alpha = 2 * M_PI * (i + 1.0 / 8.0) / n;
> > +            s->tcos[i] = -cos(alpha) * scale;
> > +            s->tsin[i] = -sin(alpha) * scale;
> > +        }
>
> Reindenting the "for ..." and "alpha = ..." is cosmetics.

FFmpeg documentation does not quite agree with you (you are too extreme in
this case):

"We refuse source indentation and other cosmetic changes if they are mixed 
with functional changes, such commits will be rejected and removed. Every 
developer has his own indentation style, you should not change it. Of course 
if you (re)write something, you can use your own style, even though we would 
prefer if the indentation throughout FFmpeg was consistent (Many projects 
force a given indentation style - we do not.). If you really need to make 
indentation changes (try to avoid this), separate them strictly from real 
changes. 
NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of code, 
then either do NOT change the indentation of the inner part within (do not 
move it to the right)! or do so in a separate commit "

> Also it could be e.g.
>
> > +    theta = scale < 0 ? 0.5 * M_PI : 0;
> > +    scale = sqrt(FFABS(scale));
> >      for(i=0;i<n4;i++) {
> >          alpha = 2 * M_PI * (i + 1.0 / 8.0) / n + theta;
> > -        s->tcos[i] = -cos(alpha);
> > -        s->tsin[i] = -sin(alpha);
> > +        s->tcos[i] = -cos(alpha) * scale;
> > +        s->tsin[i] = -sin(alpha) * scale;
> >      }

Yes this looks much more compact. The only drawback is that it introduces an
extra addition per iteration in the inner loop. So it's not a complete win
perfect replacement either. Should I resubmit a patch with this way of table
initialization?

> > > wouldn't
> > > it possibly make more sense to instead add a ff_mdct_init_scaled with
> > > the scale and leave the rest unchanged?
> >
> > That was in my initial submission, ironically it had exactly the same
> > function name as you suggested :-) Here is also a link:
> > http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-September/053931.ht
> >ml
>
> Ok, whatever then.
>
> > > I don't really know, just thinking.
> > > I also wonder if it might be better if the calling code did the sqrt(),
> > > for the current uses doing it in ff_mdct_init seems like it would only
> > > add pointless complexity and possibly inaccuracies (or is e.g.
> > > sqrt(1.0) == 1.0 always guaranteed by the C standard?).
> >
> > Yes, I also felt that the use of additional '_scaled' variant was a
> > reasonable choice (either with some code duplication to avoid 'sqrt', or
> > just calling one function from another for now).
>
> You misunderstood, my question was why not just change the scale argument
> to be the sqrt of the scale?

OK, sorry. Thanks for clarification.

> So far the scale is always a constant, so this 
> means that for now we can avoid rounding issues due to the sqrt by
> precalculating the value (or at least have the compiler do it), and in
> general having the sqrt in the mdct_init call would not increase
> complexity much.

I have no strong opinion about this (and I doubt that it matters here
anyway).

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090512/fd21e905/attachment.pgp>