[FFmpeg-soc] [soc]: r1538 - in aaclc: . TODO aac.h aacdec.c aactab.c aactab.h ffmpeg.patch

[Michael Niedermayer]

On Fri, Dec 07, 2007 at 05:07:01PM +0100, Andreas Öman wrote:
> Benoit Fouet wrote:
> 
> > wasn't that a modified copy of the aac/ directory ?
> > 
> 
> We had a discussion on #ffmpeg-devel regarding this matter.
> The general opinion was that it could just as well be created
> >from "scratch". The diffs back to the "original" aac.c file
> would probably be very messy anyway.

yes combining many unrelated changes leads to messy diffs thats
why each individual change should be commited seperately

let me just say that if it were ffmpeg-svn i would consider to
revoke your svn write permissions for such a commit like that here
and obviously revert the commit at the spot

now its just soc-svn and the average quality of commts here is lower
but i think this commit beats everything which was commited in 2007
in terms of low quality

somehow i feel that theres a "aac is important, lets fogrget all sanity
and get it done as quick as possible even if the result will be trash"
mentality here
i can only say we do have faad already we should do it properly or
not at all


diff -ubwiB |diffstat
ffmpeg.patch->ffmpeg.patch |    8     4     4     0 ++++----
aactab.h    ->aactab.c     |  147    70    77     0 ++++++++++++++++++++++++++++++---------------------------------
aac.c       ->aacdec.c     | 2521   830  1691     0 ++++++++++++++++++++-------------------------------------------

wc -l
 1505 aacdec.c
   99 aac.h
  676 aactab.c
   47 aactab.h
   24 ffmpeg.patch
   16 TODO

as one can see 1/2 of aacdec.c and 9/10 of aactab.c are unchanged. and
the changes contain things like:

-static void pulse_tool(AACContext * ac, const ics_struct * ics, const pulse_struct * pulse, int * icoef) {
+/**
+ * Pulse tool
+ */
+static void pulse_tool(AACContext *ac, aac_ics * const ics,
+                       aac_pulse * const pulse, int *icoef)
+{
     int i, off;
-    if (pulse->present) {
-        assert(ics->window_sequence != EIGHT_SHORT_SEQUENCE);
+    if(!pulse->present)
+        return;
+
         off = ics->swb_offset[pulse->start];
         for (i = 0; i <= pulse->num_pulse; i++) {
             off += pulse->offset[i];

such a change if it were seen would be rejected as being bad
the comment added is useless i can read the function name already without it
the change from if(){...} to if() return is even more stupid it should be
if() pulse_tool() on the caller side if anything

this also shows very nicely why commiting many unrelated things together is
bad, and why svn add is even worse, such changed are obscured

i dont know who on #ffmpeg-dev "approved" that but i do know they need their
head examined and suspect they never looked at the actual code before
approving it

also heres anoter example:
-static void transform_sce_tool(AACContext * ac, void (*sce_trans)(AACContext * ac, sce_struct * sce)) {
+
+/**
+ * Perform function \p f on all active channel elements
+ */
+static void transform_sce_tool(AACContext * ac,
+                               void (*f)(AACContext *ac, aac_sce *sce))
+{
     int i;
+
     for (i = 0; i < MAX_TAGID; i++) {
         if (ac->che_sce[i] != NULL)
-            sce_trans(ac, ac->che_sce[i]);
+            f(ac, ac->che_sce[i]);
         if (ac->che_cpe[i] != NULL) {
-            sce_trans(ac, &ac->che_cpe[i]->ch[0]);
-            sce_trans(ac, &ac->che_cpe[i]->ch[1]);
+            f(ac, &ac->che_cpe[i]->ch[0]);
+            f(ac, &ac->che_cpe[i]->ch[1]);
         }
         if (ac->che_lfe[i] != NULL)
-            sce_trans(ac, ac->che_lfe[i]);
-        if (ac->che_cc[i] != NULL)
-            sce_trans(ac, &ac->che_cc[i]->ch);
+            f(ac, ac->che_lfe[i]);
+        if(ac->che_cce[i] != NULL)
+            f(ac, &ac->che_cce[i]->ch);
     }
 }

here i first even checked the +/- where the new/old and not the other way
around
renaming sce_trans to f?

so yeah, this whole commit is bad, really bad and should be reverted and
redone properly

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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-soc/attachments/20071207/78abd97a/attachment.pgp>