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

Siarhei Siamashka siarhei.siamashka
Thu May 14 10:06:21 CEST 2009


On Wednesday 13 May 2009, Michael Niedermayer wrote:
> On Wed, May 13, 2009 at 09:39:42AM +0300, Siarhei Siamashka wrote:
> > On Tuesday 12 May 2009, Diego Biurrun wrote:
> > > On Tue, May 12, 2009 at 02:43:47PM +0000, Carl Eugen Hoyos wrote:
> > > > Diego Biurrun <diego <at> biurrun.de> writes:
> > > > > > > Reindenting the "for ..." and "alpha = ..." is cosmetics.
> > > > > >
> > > > > > FFmpeg documentation does not quite agree with you (you are too
> > > > > > extreme in this case):
> > > > >
> > > > > That's MPlayer documentation.
> > > >
> > > > But FFmpeg documentation says the same:
> > > > http://ffmpeg.org/general.html#SEC27
> > >
> > > Either way, Siarhei did not follow it.
> >
> > Quite the opposite. I followed it precisely. What about that '> 5 lines'
> > part in "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"?
> >
> > Let's have a more close look. I submitted this patch [0]:
> > -    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;
> > +        }
> >
> > Possible alternative is [1]:
> > +    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] = -cos(alpha);
> > -        s->tsin[i] = -sin(alpha);
> > +            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;
> > +        }

Seems like you don't want to discuss what is considered cosmetics and
what is not... Me neither :)

> much simpler:
>
> +    s = sqrt(fabs(scale));
>      for(i=0;i<n4;i++) {
>          alpha = 2 * M_PI * (i + 1.0 / 8.0) / n;
> +        if(scale < 0)
> +            alpha += 90?;
> -        s->tcos[i] = -cos(alpha);
> -        s->tsin[i] = -sin(alpha);
> +        s->tcos[i] = -cos(alpha) * s;
> +        s->tsin[i] = -sin(alpha) * s;

And what about this variant?
 
+    alpha = (scale < 0 ? 0.5 * M_PI : 0) + 0.25 * M_PI / n;
+    scale = -sqrt(fabs(scale));
     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);
+        s->tcos[i] = cos(alpha) * scale;
+        s->tsin[i] = sin(alpha) * scale;
+        alpha += 2.0 * M_PI / n;
     }

At least the compiler generates much more compact code for the inner loop, and
division is avoided.

Seems to pass regression tests fine on x86 with the version of gcc I have
installed here, but who knows about other platforms/compilers? IIRC, tables
generation may be a bit fragile.

-- 
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/20090514/1427f2df/attachment.pgp>



More information about the ffmpeg-devel mailing list