GNOME Bugzilla – Bug 343131
[wavpack] add wavpack encoder
Last modified: 2006-06-10 15:42:24 UTC
Hi, attached is a patch that adds a new wavpack encoder element and makes the needed changes to the wavpack elements that are already there. This one uses the official API and I'll convert the parser and decoder to that too in the next days Bye
Created attachment 66347 [details] [review] wavpackenc.diff patch against HEAD that adds the wavpackenc element
Created attachment 66389 [details] [review] wavpackenc-2.diff updated patch with some minor cleanup
Created attachment 66391 [details] [review] wavpackenc-3.diff the last one had a small bug, i called finalize of the parent in dispose instead dispose ;)
Cool, nice work, almost ready to go in. Some small nitpicks: gst_wavpack_enc_sink_set_caps(): - I don't think you are supposed to unref the input caps - doesn't that trigger warnings or valgrind complaints? gst_wavpack_enc_push_blocks(): - the last block should probably just be g_assert_not_reached(). gst_wavpack_enc_chain(): - we've got GST_ROUND_UP_X macros in gstutils.h ;) - we also have gst_pad_query_peer_duration() now - don't think '1LL' is something understood by all compilers, there's a GLib macro for that purpose though. - towards the end you gst_object_unref (wavpack_enc) but then happily keep accessing it. You should unref it only after you don't need it any longer really. And please tell me wavpack doesn't export functions called 'MD5Update' etc. without prefixing these? (And if they are not prefixed, are you supposed to use them?)
a) yes, that's probably the reason for the warning I sometimes get... but iirc it didn't disappear after removing the unref()... I'll test when I'm back home b) right, will be changed c) for round up that would be GST_ROUND_UP_8 (width) / 8? d) for 1LL it should be G_GINT64_CONSTANT (1)? e) right... the unref() will be fixed ;) for the MD5Update functions, etc... yes they're unfortunately exported but this will be fixed in future wavpack versions too, the library simply won't include the MD5 stuff and users of the library must have their own implementation. (it doesn't make much sense to put it in the library because the MD5 sum is on the unmodified samples but the library only gets the samples padded to 32 bit, i.e. modified ones ) I'll include the MD5 implementation with my next patch, all functions prefixed with _gst_wavpack_enc_*. Is this ok for you? I also have some other fixes locally, namely the offset and offset end of the buffers is now set (am I right that the offset would be in samples for audio? the decoder does this currently) and correction buffers also get timestamp, duration, offset and offset end. I'll upload a diff with all this changes when I'm back home... please let me know if you have any other issues :) Bye PS: the decoder is now converted to the "real" API too, was really very easy... only the parser will need some more work it seems... but should be finished for the weekend :)
> c) for round up that would be GST_ROUND_UP_8 (width) / 8? Actually, I misread the code. No need to use the macro here. > d) for 1LL it should be G_GINT64_CONSTANT (1)? yes > I'll include the MD5 implementation with my next patch, all functions > prefixed with _gst_wavpack_enc_*. Is this ok for you? Sure. If you ship them with the plugin, the prefix doesn't really matter, it'll be masked out and not be exported anyway (by the build system) (I think). > I also have some other fixes locally, namely the offset and offset end of the > buffers is now set (am I right that the offset would be in samples for audio? > the decoder does this currently) and correction buffers also get timestamp, > duration, offset and offset end. Yes, offset is in samples (per channel, ie. channel-independent, ie. same amount for mono or stereo for the same duration at the same sample rate). Not that it hurts, but setting offset/offset_end isn't really necessary or particularly useful for encoded/compressed data, only timestamp/duration is (for muxers). > I'll upload a diff with all this changes when I'm back home... please let me > know if you have any other issues :) You probably want to intercept incoming NEWSEGMENT events and just drop them (and then send your own newsegment event in BYTES format starting from 0 when you push the header). Otherwise the sink might get confused by multiple newsegment events in possibly different formats and weird things might happen.
Ok, I'll fix the offset stuff I have locally then :) Currently it's samples of all channels IIRC. For the NEWSEGMENT... how would I do that? Catch NEWSEGMENT on the sink, create my own NEWSEGMENT event in bytes and send this to the src pads. This new NEWSEGMENT event has as offset always 0? Shall I re-init the encoder on NEWSEGMENT and then start from 0 again? Or should it have the current offset in bytes, i.e. shall I just count how much I push to the srcpads and put that in the NEWSEGMENT as offset?
> For the NEWSEGMENT... how would I do that? Catch NEWSEGMENT on the sink, create > my own NEWSEGMENT event in bytes and send this to the src pads. This new > NEWSEGMENT event has as offset always 0? Shall I re-init the encoder on > NEWSEGMENT and then start from 0 again? > Or should it have the current offset in bytes, i.e. shall I just count how much > I push to the srcpads and put that in the NEWSEGMENT as offset? I'd just drop any newsegment events you receive (ie. gst_event_unref (event); return TRUE;), possibly with a GST_WARNING if you get a newsegment event when the encoding is already in process. No need to send your own newsegment event from the event handler, just send it from your chain function before you gst_pad_push() the first buffer (ie. the wavpack header). That newsegment event should always have a 0 as start position (so that filesink starts writing files from offset 0).
Thanks, I'll do that later :)
Created attachment 66481 [details] [review] wavpackenc-4.diff patch that addresses all issues noted above and some other small parts too
Currently there is still a small problem... the samples for the MD5 sum must be little endian, the samples given to the library in host byte order. So we calculate broken MD5 sums on big endian machines currently. I'll try to solve this as elegant as possible... maybe by requiring little endian raw samples via the caps and converting the samples to host byte order while padding them to the 32 bit ints as required by the library. I definitely have to test this later on PPC :)
Created attachment 66593 [details] [review] wavpackenc-6.diff ok, there we go... works fine on x86 and ppc with all sample widths and other settings
Created attachment 66613 [details] [review] wavpackenc-7.diff make the width==depth requirement explicit via the caps... sometimes linking failed because some element tried with width!=depth
Created attachment 66616 [details] [review] wavpackenc-8.diff and now fix indention... *sigh*
There are still some other smaller bugs... if someone wants to commit this, no problem. But I'll come with a new patch fixing the small bugs tomorrow or the day after ;)
Works fine for me, let's do the additional fixes separately - I'm sure there's always something else to add/fix ;) I'm not entirely sure about how the "bitrate" property works - I think it's a bit awkward. What about doing separate properties for the bits-per-sample and the bitrate stuff and using 0 for lossless? Also, I think most other encoders specify the bitrate in bits per second instead of kilobits per second, maybe that should be changed too (I think that's what we're going to standardise on in 0.11, see bug #337409). 2006-06-10 Tim-Philipp Müller <tim at centricular dot net> Patch by: Sebastian Dröge <mail at slomosnail de> * ext/wavpack/Makefile.am: * ext/wavpack/gstwavpack.c: (plugin_init): * ext/wavpack/gstwavpackcommon.h: * ext/wavpack/gstwavpackenc.c: (gst_wavpack_enc_mode_get_type), (gst_wavpack_enc_correction_mode_get_type), (gst_wavpack_enc_joint_stereo_mode_get_type), (gst_wavpack_enc_base_init), (gst_wavpack_enc_class_init), (gst_wavpack_enc_init), (gst_wavpack_enc_dispose), (gst_wavpack_enc_sink_set_caps), (gst_wavpack_enc_set_wp_config), (gst_wavpack_enc_format_samples), (gst_wavpack_enc_push_block), (gst_wavpack_enc_chain), (gst_wavpack_enc_rewrite_first_block), (gst_wavpack_enc_sink_event), (gst_wavpack_enc_change_state), (gst_wavpack_enc_set_property), (gst_wavpack_enc_get_property), (gst_wavpack_enc_plugin_init): * ext/wavpack/gstwavpackenc.h: * ext/wavpack/md5.c: * ext/wavpack/md5.h: Add wavpack encoder element (#343131). Minor changes made: - md5.[ch]: include "_stdint.h" and use glib defines for endianness; include string.h for memcpy() instead of memory.h - wavpack.c: use GST_ macros for origin etc. in plugin definition (that was already borked before though)