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

[Siarhei Siamashka]

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

Or maybe also this one (keep indentation of the whole original loop) [2]:
+    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;
+        }

Which one do you see is easier to review? In my opinion [0] is way better,
it's easier to see the logic of these two questionable lines in the context of
new code, rather than looking at them intermixed. Apparently whoever wrote
that '>5 lines' part in FFmpeg documentation agrees with me.

Much more obvious example would be a patch to replace one part of code with
a *completely* unrelated one, where some lines are common just accidentally, 
something like "    int i;" or "    }". Trying to get them preserved in the
patch with the original indentation would be just ridiculous.

If you think that the documentation is wrong, feel free to update
documentation to make it less confusing (remove >5 lines part).
But please don't accuse me of something that is not my fault.

Anyway, this all was only about the FFmpeg documentation.

Regarding the patch itself, I'm still waiting for the confirmation whether
Reimar's code should be used in 'ff_imdct_init' instead of the quoted
code fragments. That would also solve formatting/indentation issue as a side
effect :-)

-- 
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/20090513/e60df28c/attachment.pgp>