GNOME Bugzilla – Bug 598763
New plugin: aiffmux
Last modified: 2009-10-31 20:29:38 UTC
Created attachment 145658 [details] [review] Basic AIFF muxer Add a basic AIFF muxer. At the moment just the FORM, COMM and SSND chunks are written as they are the only ones that are mandatory. I will have to do some work to make sure that all chunk types are copied if remuxing as this is mandated by the spec I found here: http://www.music.mcgill.ca/~ich/classes/synth/AudioIFF1.2.1/AudioIFF1.2.1.html I used the gst-template tool to create a generic template plugin and then edited it while reading the plugin writer's guide as well as looking at the wavenc plugin. One of the fields of the SSND chunk ('sampleRate') is written to the bitstream as an IEEE754 80-bit extended float. I borrowed some LGPL code from FFmpeg that reformats a double to this format and clearly noted in the code which portion this is. I've tested with gst-launch, mplayer, ffplay and vlc. The two former ones worked fine. ffplay seemed to stutter and sometimes crash on wavs and aiffs created by gstreamer so I think maybe there's some issue in ffplay. vlc stuttered on the aiff, then played a wav of the same data just fine, then played the same aiff file just fine. I'm wondering if they use a different audio output that cause stuttering. I will test with xine and sox as well. This is my first plugin so I'm eager to hear some feedback as to better ways to do things in GStreamer, particularly with regard to the event/signal/property stuff. I looked at wavenc for those parts though maybe it's not the best example.
Review of attachment 145658 [details] [review]: ::: gst/aiffmux/aiffmux.c @@ +97,3 @@ + GST_PAD_SRC, + GST_PAD_ALWAYS, + GST_STATIC_CAPS ("audio/x-raw-aiff") You should use the same caps as existing plugins. aiffparse, ffdemux_aiff and ffmux_aiff all use "audio/x-aiff" @@ +176,3 @@ + * Copyright (c) 2005 Michael Niedermayer <michaelni@gmx.at> + */ + while it makes sense to re-use existing code... you could maybe try to make a GST_{READ|WRITE}_IEEE80FLOAT_* or extend GstByteReader/Writer to support that, like that other plugins can benefit from it too.
I've tested a 6-channel 48kHz signed 16-bit big endian aiff created using the muxer in Quicktime (7.6.4 518.35) and VLC (1.0.2) in Mac OS X and they both play it fine. I don't know if it's specific to this file but Audacity (1.3.9) crashed when trying to open the same file. In Ubuntu Karmic, the same file works fine in xine (0.99.6cvs) and SoX (14.3.0).
Created attachment 145667 [details] [review] Fix mimetype of output
Created attachment 145668 [details] [review] Port to GstByteWriter
Review of attachment 145658 [details] [review]: 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)
(In reply to comment #5) > Review of attachment 145658 [details] [review]: > > Looks very nice, hard to believe this is your first time you're writing > GStreamer code :) Thanks for both the compliment and the review. :) > ::: 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? Do you usually have demuxers and muxers together in one element? > ::: 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 I'll make a patch for mpegpsmux as well in a minute. > ::: 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 tried both of these and neither produced the desired behaviour of signed 16-bit little endian being converted to signed 16-bit big endian. The former produced (as I recall) depth 25, width 32. The latter produced depth 16, width 32. I reverted back to the original order as that seems to work. > - 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 AIFF is big endian. Maybe the extension, AIFF-C, supports little endian, I don't know. I'm not sure about WAV off the top of my head. I thought it could support both endian types but maybe that's only in WAVE_FORMAT_EXTENSIBLE WAVs. I've noted that wavenc doesn't support WAVE_FORMAT_EXTENSIBLE format. > @@ +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? This bug came from wavenc. I thought it looked wrong when I copied it but as it's in -good I figured I must have been missing something. :) I'll fix it there too and submit a patch. Thanks again for the review. New patch attached...
Created attachment 145699 [details] [review] Add basic AIFF muxer This should have addressed all issues mentioned above apart from writing GStreamer's own IEEE80 read/write functions.
*** Bug 105882 has been marked as a duplicate of this bug. ***
Committed, thanks! commit d65d2888449ca0810842151b96093159c20166de Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Sat Oct 31 17:50:54 2009 +0000 aiff: enable new aiff muxer Fixes #598763 even more. commit 20f4a1afd9f6e90364d3bf259fc5421ae42d5ab7 Author: Robert Swain <robert.swain@gmail.com> Date: Sat Oct 17 22:58:03 2009 +0100 aiff: add basic AIFF muxer Fixes #598763.