[gstreamer-bugs] [Bug 598763] New plugin: aiffmux

Sat Oct 17 09:14:18 PDT 2009

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

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145658|none                        |reviewed
             status|                            |

--- Comment #5 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2009-10-17 16:14:13 UTC ---
Review of attachment 145658:

Looks very nice, hard to believe this is your first time you're writing
GStreamer code :)

::: configure.ac
@@ +253,3 @@
 AG_GST_CHECK_PLUGIN(adpcmdec)
 AG_GST_CHECK_PLUGIN(aiffparse)
+AG_GST_CHECK_PLUGIN(aiffmux)

Maybe the aiffparse and aiffmux plugin should be combined into a single aiff
plugin?

::: gst/aiffmux/Makefile.am
@@ +5,3 @@
+
+libgstaiffmux_la_CFLAGS = $(GST_CFLAGS) $(GST_BASE_CLFAGS)
+libgstaiffmux_la_LIBADD = $(GST_LIBS) $(GST_BASE_LIBS)

- CLFAGS, eh? (Looks like it needs fixing in mpegpsmux too)
- I believe the order should be ... $(GST_BASE_XYZ) $(GST_XYZ) instead

::: gst/aiffmux/aiffmux.c
@@ +91,3 @@
+        "signed = (boolean) true, "
+        "endianness = (int) BIG_ENDIAN, "
+        "channels = (int) [ 1, MAX ], " "rate = (int) [ 1, MAX ]; ")

Couple of points:
 - order in caps often matters: usually the first structure is taken to be the
'prefered' one, so the higher quality formats should probably come first
 - not sure if it makes sense to have separate structs for all of these just to
micro-manage the depth parameters - I think width=(int){8, 16, 24, 32},
depth=(int)[1,32] would do as well? (but then, if you keep them separate you
can express a preference for the higher-width ones, so maybe just keep them
like that, dunno).
 - I think the endianness=BIG_ENDIAN thing is not very user-friendly (wavenc
only accepts LITTLE_ENDIAN, and that's also not very nice IMHO) - maybe it
should be made to support both with on-the-fly byteswapping if needed? (It's
not obvious from gst-inspect that the endianness is fixed, and people will not
realise the need to always put an audioconvert element in front if they want
their pipeline to bt portable, which then might cause failurs on
other-endianness architectures). Don't know if efficiency matters so much here

@@ +158,3 @@
+#define AIFF_SSND_HEADER_LEN 8 + 8
+#define AIFF_HEADER_LEN \
+  AIFF_FORM_HEADER_LEN + AIFF_COMM_HEADER_LEN + AIFF_SSND_HEADER_LEN

It might not matter here, but in general one should use brackets here, so that
something like AIFF_FOO_LEN * 2 in the code is expanded to the right thing

@@ +164,3 @@
+    guint8 * header)
+{
+  // ckID == 'FORM'

Please use C-style comments (there are C compilers who will fail on C++-style
comments in C code IIRC).

@@ +165,3 @@
+{
+  // ckID == 'FORM'
+  GST_WRITE_UINT32_LE (header, GST_MAKE_FOURCC ('F', 'O', 'R', 'M'));

This makes me wonder if we should add a GST_WRITE_FOURCC convenience macro (and
maybe the same for GstByteWriter)

@@ +251,3 @@
+  gst_pad_push_event (aiffmux->srcpad,
+      gst_event_new_new_segment (FALSE, 1.0, GST_FORMAT_BYTES,
+          0, GST_CLOCK_TIME_NONE, 0));

GST_CLOCK_TIME_NONE should only be used when dealing with timestamps or
GST_FORMAT_TIME, best to just use -1 here (or GST_BUFFER_OFFSET_NONE if you
prefer a macro).

Also, since you probably rely on the 'seek to beginning' here when rewriting
the header at EOS, you should check the return value of _push_event() and at
least print a debug warning message or so if the push failed (or maybe even
post a warning or error message with GST_ELEMENT_{ERROR|WARNING} if the file
will not be readable without the header fixed up).

@@ +283,3 @@
+  GstFlowReturn flow = GST_FLOW_OK;
+
+  g_return_val_if_fail (aiffmux->channels > 0, GST_FLOW_WRONG_STATE);

g_return_val_if_fail() and friends should only be used to guard against
programming errors, not input errors. Arguably, there's a bit of a gray area
here, but input caps checking or checking that caps have been set at all should
really be regarded as 'input checking' IMHO. Keep in mind that the code may be
compiled with -DG_DISABLE_CHECKS or whatever it is called, which would just
expand this line to a NOOP. This is often done on embedded systems, for
example.

So I think this line should just read: if (aiffmux->channels == 0) return
GST_FLOW_NOT_NEGOTIATED. (NOT_NEGOTIATED seems more appropriate here, as it's
used for bad input caps or lack of input caps). It's a good thing you check for
that in the first place though, I think many elements don't :)

@@ +291,3 @@
+
+    if (flow != GST_FLOW_OK)
+      return flow;

I think you might be leaking the input buffer here in the error case.

@@ +305,3 @@
+
+  gst_buffer_set_caps (buf, GST_PAD_CAPS (aiffmux->srcpad));
+  GST_BUFFER_OFFSET (buf) = AIFF_HEADER_LEN + aiffmux->length;

Is this right here given that we have already increased aiffmux->length by the
buffer size a few lines above?

@@ +354,3 @@
+  gint chans, rate, depth;
+
+  aiffmux = GST_AIFFMUX (gst_pad_get_parent (pad));

FWIW, In the sink pad _set_caps() it's usually fine to just use GST_PAD_PARENT
as well, same as in the chain function (both will be called with the pad's
streaming lock held).

@@ +364,3 @@
+
+  structure = gst_caps_get_structure (caps, 0);
+  name = gst_structure_get_name (structure);

name isn't used/needed, is it?

@@ +426,3 @@
+#ifndef PACKAGE
+#define PACKAGE "aiffmux"
+#endif

I think this ifndef block can be removed?

::: gst/aiffmux/aiffmux.h
@@ +52,3 @@
+  (gst_aiffmux_get_type())
+#define GST_AIFFMUX(obj) \
+  (G_TYPE_CHECK_INSTANCE_CAST((obj),GST_TYPE_AIFFMUX,GstAIFFMux))

The canonical scheme is: either GstAiffMux or GstAIFFMux (I prefer the first,
but tastes differ) and then gst_aiff_mux_* and GST_TYPE_AIFF_MUX, GST_AIFF_MUX
etc. (ie. an additional underscore between aiff and mux in all cases)

-- 
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.