GNOME Bugzilla – Bug 330317
New xingmux element for writing Xing headers
Last modified: 2006-05-03 21:43:49 UTC
Here is a patch which writes a proper Xing header to mp3s creating with the lame element. I guess this would fit better in a separate xingmux element, I can do that if needed. Atm I'm mainly attaching this patch since it works for me (though the number of frames written to the header is 1 less than the number of frames in the original mp3 when I do a mad ! lame transcoding) and I'd like to get some feedback on it (about things to improve).
Created attachment 58887 [details] [review] The patch
I forgot at least one thing, a g_free in gst_lame_finalize.
Hi Christophe, Mind updating this one with the cleanup fixes, and with a changelog entry?
Created attachment 59522 [details] [review] Updated patch, added ChangeLog entry and a missing g_free. Compile tested only
Thanks for the patch! Raising priority so someone can have a look at it sooner.
Fwiw I (slowly) started to write a xingmux element yesterday evening, so it might be worth waiting a week or two to see if I finish it or not ;)
Created attachment 60632 [details] Xing header writer as a separate element (C file) I finished to put the Xing header code in a separate header, it seems to be working quite well and not leaking resource. I'm unsure I'm doing the right thing in the state change functions though (dunno if I should reset the header state when going from PAUSE to READY or from READY to NULL), and I don't know if there are events I should handle apart from EOS. I'm not sure the method I'm using to seek at the beginning of the stream to output my header is the best one. And it's also missing seek table support.
Created attachment 60633 [details] Xing header writer (H file) I'm unsure where these 2 files should be put, for now I modified gst-plugins-ugly/ext/lame/Makefile.am to build them, but maybe they should go somewhere else?
Created attachment 60634 [details] [review] Mark the xingheader property as obsolete in the lame element It's probably a bad idea to remove an existing property from the lame element, but we could at least mark it as obsolete (if the xingmux element gets in obviously ;)
Created attachment 60699 [details] Updated version of the C file This new version handles NEW_SEGMENT events.
Ok, no need to review that for now, I'll have to take into account some of the comments done in bug #333501 first since they apply to the code in that element as well.
Created attachment 60852 [details] [review] Cleaned up code
Since the input and ouput caps are identical, do I need to call buffer_set_caps in my chain function? (it's not done currently)
Generally not, but you probably need to call it on the xing header buffer if that's the first buffer you send out.
Created attachment 60992 [details] [review] Added missing gst_buffer_set_caps I'm wondering if the file names (gst-xing-header.c) are in line with gstreamer conventions (if there are any conventions about file names)
So here goes the usual nitpicking: * the filenames should be gstxingmux.[ch], not gst-xing-mux.[ch] * the plugin name in GST_PLUGIN_DEFINE should be "xingheader" or "xing", and the plugin filename in Makefile.am should be libgstxingheader.so or libgstxing.so (the element is then still called "xingmux"). That way it's possible to list the plugin details separately in gst-inspect, and also it matches the directory naming (gst/xingheader/). Just a personal preference of mine though, there are plenty of cases where this isn't done. * Here const gchar mp3_header[4] = {0xff, 0xfb, 0x90, 0x44}; you should use guint8 or guchar. * for elements that aren't base classes for other elements you don't really need private structs like GstXingMuxPriv, as the header file is not installed, so there aren't any ABI/API issues here. Doesn't hurt to have it of course, just thought I'd mention it. Feel free to commit to -bad when you think it's ready (do change the filenames though).
Created attachment 61080 [details] [review] Committed patch Here is the patch I committed. Should the xingheader property be marked as obsolete in lame, or should we keep it as is for now?
Does xingmux already do everything that lame does when writing the tag? If yes, I'd say mark the lame property as deprecated and recommend xingmux instead. If not, better wait until xingmux is feature-equivalent for now.
xingmux writes far less info than the command-line lame binary, but xing header writing from the gstreamer lame element never worked properly, ie it didn't even properly write the correct length because of limitations in liblame API. So xingmux is already a far better replacement (ie a working one) to the xingheader=TRUE gstlame property.
Cool, let's deprecate it then :)
Is the patch from comment #9 ok to commit? (it's probably better to replace OBSOLETE with BROKEN in it). Or are there different ways to deprecate it?
Oh, didn't see that one. Sure, the patch looks fine - please commit it.
It's done.