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

[Michael Niedermayer]

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;
> +        }

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;


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20090513/b8f84bf9/attachment.pgp>