GNOME Bugzilla – Bug 333501
[patch] taglib element
Last modified: 2006-08-24 06:49:50 UTC
I wrote a simple GStreamer element to write id3 tags using taglib, see attached tarball.
Created attachment 60698 [details] New taglibmux plug-in This tarball puts some files in the gst-plugins-good directory. I put my element there for the testing phase, I'm not sure what it takes for a plugin to be included in -good, so just tell me if it must live elsewhere :) There's a taglib.diff file making the necessary changes to configure.ac/Makefile.am and an ext/taglib directory. It creates id3v2.4 tags only, so many taggers won't be able to read the tags. It could be made to write other tags than id3v2 tags (APE or FLAC for example), but I chose to keep it simple for now.
it will have to go in -bad before. Can you update your tarball/patch so that it goes in -bad ? Thanks
Sure, I'll try to do that tonight.
Created attachment 60744 [details] Put taglibmux in -bad Actually I just did it. And I realized that I forgot to make that bug depend on bug 333417 otherwise it won't compile.
This is quite cool :) Some random comments about the code: * the class description string in the element details should probably be Formatter/Metadata (as per the new gstreamer/docs/design/draft-klass.txt) * the parent_class=... line in _class_init() isn't required when you use GST_BOILERPLATE, GST_BOILERPLATE will do that for you * GST_BOILERPLATE_WITH_INTERFACE is probably not the right thing to use here, it's meant for interfaces that are only implemented under special circumstances (like GstMixer) and that are wrapped by GstImplementsInterface. GstTagSetter is a normal interface though that doesn't need wrapping with GstImplementsInterface, so GST_BOILERPLATE_FULL might be more appropriate. * it's always good to use GST_DEBUG_FUNCPTR() IMHO, like this: gst_pad_set_chain_function (taglib->sinkpad, GST_DEBUG_FUNCPTR (gst_taglib_chain)); That will make any debug output much more readable later on. Same goes for vfuncs. Just a minor detail of course. * in gst_taglib_render_tag() I personally wouldn't worry about how expensive memcpy() is - it's only done once and the buffer size is pretty small too. buf = gst_buffer_new_and_alloc (rendered_tag.size()); memcpy (GST_BUFFER_DATA (buf), rendered_tag.data(), rendered_tag.size()); or something similar should do the job just fine IMHO. * in gst_taglib_chain(): - I wouldn't _join() the incoming buffer and the tag buffer, I'd rather push out the tag separately as a single buffer, and then push the incoming buffer out after that. No particular reason, just seems cleaner somehow (and easier to debug later on). - you should set new caps on the buffers you send out with gst_buffer_set_caps (buf, GST_PAD_CAPS (taglibmux->srcpad)); or so. * in gst_taglib_sink_event(): - in case of GST_EVENT_TAG, wouldn't we want to send a new TAG event with the merged tags downstream, rather than just forwarding the tags we got? - in case of GST_EVENT_NEWSEGMENT: I think it should be result = gst_pad_push_event (...); instead of gst_pad_push_event (...); result = TRUE; (not that I can think of a case where it really matters in practice, just seems like the right thing to do) * in gst_taglib_change_state(): you need to chain up to the parent class before handling a downwards (PAUSED=>READY) state change. Only upwards state changes are handled before chaining up to the parent class. I'm not entirely sure about the element name. Personally, I'd prefer something like 'tagid3mux' or maybe even 'id3v2mux' or 'tagid3v2mux' - that leaves us more options to add other muxer elements based on TagLib later on (like tagapemux or so). I think at least the fact that it is muxing ID3 tags should somehow be reflected by the name of the element.
Installed against gst-plugins-bad CVS, but won't compile. I am using FC-4 running gcc-4.0.2-8.fc4: if /bin/sh ../../libtool --tag=CXX --mode=compile g++ -DHAVE_CONFIG_H -I. -I. -I../.. -I../../gst-libs -I../../gst-libs -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -Wall -Werror -I/usr/include/taglib -g -O2 -MT libgsttaglib_la-gsttaglib.lo -MD -MP -MF ".deps/libgsttaglib_la-gsttaglib.Tpo" -c -o libgsttaglib_la-gsttaglib.lo `test -f 'gsttaglib.cc' || echo './'`gsttaglib.cc; \ then mv -f ".deps/libgsttaglib_la-gsttaglib.Tpo" ".deps/libgsttaglib_la-gsttaglib.Plo"; else rm -f ".deps/libgsttaglib_la-gsttaglib.Tpo"; exit 1; fi libtool: ignoring unknown tag CXX g++ -DHAVE_CONFIG_H -I. -I. -I../.. -I../../gst-libs -I../../gst-libs -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -Wall -Werror -I/usr/include/taglib -g -O2 -MT libgsttaglib_la-gsttaglib.lo -MD -MP -MF .deps/libgsttaglib_la-gsttaglib.Tpo -c gsttaglib.cc -fPIC -DPIC -o .libs/libgsttaglib_la-gsttaglib.o gsttaglib.cc: In function 'void gst_taglib_implements_interface_init(GstImplementsInterfaceClass*)': gsttaglib.cc:36: error: invalid conversion from 'void*' to 'gboolean (*)(GstImplementsInterface*, GType)' make: *** [libgsttaglib_la-gsttaglib.lo] Error 1
Alex: you need GStreamer CVS from this afternoon or later (there should be a new patch coming up soon which doesn't require this any longer though I believe)
(In reply to comment #6) > Installed against gst-plugins-bad CVS, but won't compile. I am using FC-4 > running gcc-4.0.2-8.fc4: Hmm, I guessing that's because of bug #333417. Does this mean I have to upgrade to gstreamer CVS to compile this? Bleh.
(In reply to comment #7) > Alex: you need GStreamer CVS from this afternoon or later (there should be a > new patch coming up soon which doesn't require this any longer though I > believe) I've been trying to get away with using the RPMs from gstreamer.net for the core libraries and only compile the plugins from CVS, but it seems that the plugins are drifting out of date against the gstreamer RPMs. Also, switching this to the -bad component.
I'm about to attach a patch which might be compilable without getting GStreamer CVS. If you don't want to upgrade to GStreamer CVS, you can patch the installed header by hand, it shouldn't hurt ;)
Created attachment 60786 [details] [review] New patch taking into account the review I kept the plugin name as taglibmux, but renamed the element to tagid3v2mux, apart from that I think I took into account all your comments
Created attachment 60788 [details] [review] Yay, another one Use gst_event_unref (event), not gst_object_unref (event) ;)
(In reply to comment #10) > I'm about to attach a patch which might be compilable without getting GStreamer > CVS. If you don't want to upgrade to GStreamer CVS, you can patch the installed > header by hand, it shouldn't hurt ;) Compiles against the RPM version of gst 0.10. Thanks.
Can you post the gst-launch-0.10 pipeline that I can use to test it in standalone mode? I tried replacing id3mux with tagid3v2mux in this patch for rhythmbox: http://bugzilla.gnome.org/attachment.cgi?id=58495 which is attached to bug #309609, but for some reason (probably because the element isn't recognised, although I can set GST_PLUGIN_PATH and see it via gst-inspect-0.10) the dialog box doesn't switch to "editable" tags mode.
Looks excellent. Some more minor nitpicks that I missed the first time, then I think you're ready to go and can commit this to gst-plugins-bad: - the class name is GstTagLibMux, so functions should be called gst_tag_lib_mux_*() whenever possible - the debug category variable should probably have a prefix/suffix, just 'debug' isn't good even if it's static - some of the GST_DEBUG/etc. statements have newline characters at the end of the string, those aren't needed and should probably be removed. - lines should be commented out with /* ... */, as // doesn't work with some C compilers (like Sun's Forte). - in the sink event function, NEWSEGMENT part, at the beginning there's a gst_pad_push_event(); result = TRUE; that you forgot to convert - in the sink event function, 'default' case, it should be: result = gst_pad_event_default (pad, event); As test pipeline % gst-launch-0.10 filesrc location=foo.ogg ! oggdemux ! vorbisdec ! audioconvert ! lame ! tagid3v2mux ! filesink location=foo.mp3 works quite well, % gst-launch-0.10 playbin uri=file://path/to/foo.mp3 -t displays all the tags correctly :) (Now to try with some proper UTF8 strings ...)
Created attachment 60800 [details] [review] Integrated new comments I made all the changes you suggested. This is a c++ program, so the // comment was probably valid. I changed it anyway.
Created attachment 60804 [details] [review] I should test my patches before uploading them... I had screwed up the gst_pad_event_default parameters
Created attachment 60805 [details] [review] Patch spam ;) This revised version properly works with UTF-8 tags.
(In reply to comment #16) > Created an attachment (id=60800) [edit] > Integrated new comments > > I made all the changes you suggested. This is a c++ program, so the // comment > was probably valid. I changed it anyway. Before applying this version of the patch, I got it to convert an existing mp3 successfully using this pipeline (using id3demux): GST_PLUGIN_PATH=/usr/local/lib/gstreamer-0.10 gst-launch-0.10 filesrc location=V2.mp3 ! id3demux ! tagid3v2mux ! filesink location=foo.mp3 However upon applying this patch it hangs (although it appears to write out the file): GST_PLUGIN_PATH=/usr/local/lib/gstreamer-0.10 gst-launch-0.10 filesrc location=V2.mp3 ! id3demux ! tagid3v2mux ! filesink location=foo.mp3 Setting pipeline to PAUSED ... Pipeline is PREROLLING ... Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock (gst-launch-0.10:18008): GStreamer-WARNING **: pad id3demux0:src sending eos event in wrong direction (it hangs here)
With new patch, works again now. Now trying to get it to work with rhythmbox.
I can now do tag writing with RB, yay! Interestingly enough with the latest gst-plugins-ugly package 0.10.2 (not CVS), it also appears that the old id3mux plugin now works too, although it too only writes id3v2.4 tags too. There seems to be some differences between the tags re-written by id3mux vs tagid3v2mux, in that ones written by tagid3v2mux can be read by xmms, but ones written by id3mux cannot. Strange.
A few unsupported tags. Can see by running as GST_DEBUG=taglibmux:5. The important one is album-disc-number because rhythmbox allows the user to set it (see also bug #333683). WARN (0x89eae38 - 0:00:09.999265000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: replaygain-track-peak WARN (0x89eae38 - 0:00:10.013728000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: replaygain-album-gain WARN (0x89eae38 - 0:00:10.013887000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: replaygain-album-peak WARN (0x89eae38 - 0:00:10.014002000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: album-disc-number WARN (0x89eae38 - 0:00:10.014115000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: bitrate WARN (0x89eae38 - 0:00:10.014227000) taglibmux(31055) gsttaglib.cc(211):void add_one_tag(const GstTagList*, const gchar*, void*): Unsupported tag: replaygain-track-gain
Also bitrate is important when doing tag rewriting.
Any tags that aren't supported in gstreamer tag writing backend are deleted, e.g. TCON (for composer), TCOP (for copyright), TENC (for name of encoder) when rewriting using this element. (This is also a problem with other plugins such as id3mux, bug #323658) What should happen is that any tags that gstreamer *doesn't* directly overwrite should be left intact otherwise there will be data lossage.
It's not easy to get all those tags intact, at least it can be done for any unknown tags, since what happens is that id3demux (or whatever element creating the tags) translate TCON, TCOP, TENC, ... to a generic symbolic constant (GST_TAG_SOMETHING), and then tagid3v2mux does the opposite translation. However, it's pretty easy to make tagid3v2mux handle some tags it doesn't handle at the moment, patch welcome ;) (alternatively, if you provide me with a mapping between gstreamer tag names and the corresponding id3v2 header (TCON, TCOP, ...), I can easily add them).
Why can't the plugins simply pass through any unknown tags back into the updated mp3 files unchanged? I understand if you are converting from another file type (e.g. mp3 -> OGG), but why does it need to do a "translation" when going from mp3 -> mp3?
Because this is the way GStreamer work ;) The element reading the tags has no way to know what kind of elements will handle the tags it's parsing (if any), ie it doesn't know if the tags will be received by a mp3 tag writer, or by an ogg encoder, by both, ... So it has to transmit them in a media-agnostic format to be sure that other elements will be able to understand them. I hope that makes sense... ;)
(In reply to comment #25) > However, it's pretty easy to make tagid3v2mux handle some tags it doesn't > handle at the moment, patch welcome ;) I did one for id3mux for disc number at bug #333683, I had a quick look at the taglibmux but didn't see immediately where to include an analogous code block.
As I see it, there are two possible solutions to this problem: (a) have a separate id3remux element that specialises in modifying ID3 tags and reads + writes them. Could be based on taglib as well. (b) id3demux from gst-plugins-good could send a custom event downstream with ID3 frames that it doesn't recognise or that can't be conveyed via the GStreamer tags system. id3v2mux could detect those events and 'inject' those frames again into the tag that it's going to write. Or something like that. Surely solvable somehow that way.
Created attachment 60950 [details] File retagged using tagid3v2mux This is a file retagged by tagid3v2mux using the following pipeline: GST_PLUGIN_PATH=/usr/local/lib/gstreamer-0.10 gst-launch-0.10 filesrc location=../orig/V2.mp3 ! id3demux ! tagid3v2mux ! filesink location=V2-taglib.mp3 The original file was a ID3v2.3 file from here: http://gstreamer.net/media/incoming/V2.mp3 It appears that the ID3 tags in this version can be seen by xmms and xine (but not tagtool). I will also attach a retagged version using id3mux to show the differencesI alluded to in comment #21.
Created attachment 60953 [details] Same file retagged using id3mux This is the same file: http://gstreamer.net/media/incoming/V2.mp3 retagged using id3mux, with this pipeline: GST_PLUGIN_PATH=/usr/local/lib/gstreamer-0.10 gst-launch-0.10 filesrc location=../orig/V2.mp3 ! id3demux ! id3mux ! filesink location=V2-id3mux.mp3 to match the version in attachment #60950 [details]. xmms and xine do not see the tags in this file.
I committed that plugin to CVS, with support for TRACK_COUNT, VOLUME_ALBUM_COUNT and VOLUME_ALBUM_NUMBER. Alex, if you feel the differences between id3mux and tagid3v2mux are important, you'd better file a separate bug report ;)
(In reply to comment #29) > As I see it, there are two possible solutions to this problem: > > (a) have a separate id3remux element that specialises in modifying > ID3 tags and reads + writes them. Could be based on taglib as > well. > > (b) id3demux from gst-plugins-good could send a custom event downstream > with ID3 frames that it doesn't recognise or that can't be conveyed > via the GStreamer tags system. id3v2mux could detect those events > and 'inject' those frames again into the tag that it's going to > write. Or something like that. Surely solvable somehow that way. Opened this tag rewriting issue as bug #334375.
*** Bug 351873 has been marked as a duplicate of this bug. ***