[Bug 647748] [aacenc] add AAC audio encoder based on vo-aacenc lib

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=647748
  GStreamer | gst-plugins-bad | 0.10.21

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #186008|none                        |needs-work
             status|                            |

--- Comment #10 from Sebastian Dröge <slomo at circular-chaos.org> 2011-04-15 13:20:46 UTC ---
Review of attachment 186008:
 --> (https://bugzilla.gnome.org/review?bug=647748&attachment=186008)

* I would call it voaacenc instead of just aacenc
* Not sure if this is a problem but the Apache License is IIRC not GPL/LGPL
compatible until GPL/LGPL 3

::: ext/aacenc/gstaacenc.c
@@ +30,3 @@
+ * ]|
+ * Please note that the above stream misses the header, that is needed to play
+ * the stream.

Which header? In theory it should be possible to play that file after passing
it through aacparse

@@ +64,3 @@
+        "endianness = (int) BYTE_ORDER, "
+        "rate = (int) {8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100,
48000, 64000, 88200, 96000}, "
+        "channels = (int) [1, 6]")

You need a getcaps function that returns the exact channel layouts that are
required for >2 channels (same goes for faac too I guess). See vorbisenc for an
example

@@ +72,3 @@
+    GST_STATIC_CAPS ("audio/mpeg, " "mpegversion = (int) 4, "
+        "rate = (int) {8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100,
48000, 64000, 88200, 96000}, "
+        "channels = (int) [1, 6], " "stream-format = (string) { adts, raw } ")

Does this library support output as mpeg2 stream too?

@@ +197,3 @@
+  aacenc->rate = AAC_ENC_DEFAULT_RATE;
+  aacenc->channels = AAC_ENC_DEFAULT_CHANNELS;
+  aacenc->output_format = AAC_ENC_DEFAULT_OUTPUTFORMAT;

Why? You'll always get the rate and channels and format from the caps, just
keep them at 0 here and check in the chain function if you got rate/channel
(and return NOT_NEGOTIATED otherwise, but you're doing that part already)

@@ +238,3 @@
+        aacenc->output_format = 0;
+      } else {
+        GST_DEBUG_OBJECT (aacenc, "unknown stream-format: %s", str);

Set an output format here if none is given

@@ +277,3 @@
+    gst_caps_unref (src_caps);
+    ret = aacenc_core_set_parameter (aacenc);
+  }

I guess if src_caps == NULL you want to return FALSE here?

@@ +303,3 @@
+
+  /* take latest timestamp, FIXME timestamp is the one of the
+   * first buffer in the adapter. */

Use gst_adapter_prev_timestamp() here and interpolate between these timestamps

@@ +322,3 @@
+
+    /* max size */
+    out = gst_buffer_new_and_alloc (aacenc->inbuf_size);

Use gst_pad_alloc_buffer()

@@ +329,3 @@
+    if (aacenc->ts != -1) {
+      aacenc->ts += aacenc->duration;
+    }

You will accumulate rounding errors here

@@ +337,3 @@
+    gst_buffer_set_caps (out, GST_PAD_CAPS (aacenc->srcpad));
+
+    data = gst_adapter_take (aacenc->adapter, aacenc->inbuf_size);

You could use gst_adapter_peek() here, this will prevent one additional
malloc() per frame and will often prevent memcpy()

@@ +397,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_READY_TO_NULL:
+      aacenc_core_uninit (aacenc);

clear the adapter here

::: ext/aacenc/gstaacenc.h
@@ +23,3 @@
+#include <gst/gst.h>
+#include "vo-aacenc/voAAC.h"
+#include "vo-aacenc/cmnMemory.h"

Use <> here, "" is usually used for including headers relative to the current
directory

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.