GNOME Bugzilla – Bug 335635
Add an Ogg/Vorbis retagging element
Last modified: 2006-10-03 11:57:54 UTC
There is currently no way to rewrite tags in an OGG file in gst 0.10. This is something that we would like to do in rhythmbox as we now have preliminary support for ID3 tag writing using "id3demux ! id3mux" (or "id3demux ! tagid3v2mux").
*** Bug 333468 has been marked as a duplicate of this bug. ***
Created attachment 62450 [details] [review] vorbistag element This patch (against -bad) adds an "vorbistag" element which modifies the vorbis comment header. Usage would be "oggdemux ! vorbistag ! oggmux".
Created attachment 62451 [details] [review] test.c Program to test the element, compilable with "gcc `pkg-config --cflags --libs gstreamer-0.10 gstreamer-base-0.10 gstreamer-plugins-base-0.10" -o test test.c Usage: ./test source.ogg dest.ogg artist foo album bar ... If the oggmux element is removed, you get what appears to be a valid vorbis stream, but with the oggmux element in place you end up with a 30-something byte file. Buggered if I know why.
While I think of it, whatever causes the oggmux issue is probably what causes the same with "filesrc ! oggdemux ! oggmux ! filesink".
Created attachment 62509 [details] [review] fixed patch This version of the patch might actually compile.
Created attachment 62995 [details] [review] working version Using gst-plugin-base from cvs (to get the newer vorbis and ogg stuff) this now works well for me, modulo the known issues with oggmux taking bits off the end.
Created attachment 62997 [details] [review] fixed test.c Updated test app that uses vorbisparse so that things work properly. Compile with " gcc `pkg-config --cflags --libs gstreamer-0.10 gstreamer-base-0.10 gstreamer-plugins-base-0.10` -g -o test test.c"
(In reply to comment #6) > Created an attachment (id=62995) [edit] > working version > > Using gst-plugin-base from cvs (to get the newer vorbis and ogg stuff) this now > works well for me, modulo the known issues with oggmux taking bits off the end. What exactly is the "known issue" with oggmux taking bits of the end? Is there bug on this? What exactly does oggmux do (when it is working properly), is the analogue of id3mux and what does it do wrong now?
(In reply to comment #8) > What exactly is the "known issue" with oggmux taking bits of the end? This is only second-hand, so I may have mis-understood, but it doesn't handle the end-of-stream condition properly. The result is the last small piece of audio go be missing. If you've ripped the ogg vorbis files with GStreamer they are probably "damaged" in this way already, so it wouldn't have any furthur effect. > Is there bug on this? No idea, it just got mentioned to me on irc. > What exactly does oggmux do (when it is working properly), is the > analogue of id3mux and what does it do wrong now? Ogg is a container (like AVI, Quicktime, etc) and oggmux takes streams, such as Vorbis or Theora, and puts them into an ogg container. When you have "ogg" audio files, they are really a Vorbis stream in an Ogg container.
(In reply to comment #9) > This is only second-hand, so I may have mis-understood, but it doesn't handle > the end-of-stream condition properly. The result is the last small piece of > audio go be missing. If you've ripped the ogg vorbis files with GStreamer they > are probably "damaged" in this way already, so it wouldn't have any furthur > effect. Bleh. Luckily I don't rip anything using gstreamer ;-) After being burnt with the lame element messing up my ID3 tags and xing headers I stayed away from any gstreamer apps for ripping. > > Is there bug on this? > No idea, it just got mentioned to me on irc. bug #337026
(In reply to comment #10) > Bleh. Luckily I don't rip anything using gstreamer ;-) After being burnt with > the lame element messing up my ID3 tags and xing headers I stayed away from any > gstreamer apps for ripping. Although ripping works fine in gstreamer 0.10 with ripping in RB, I hasten to add. (Can't test newer sound-juicer on FC-4 because it requires GNOME 2.14).
(In reply to comment #10) > > > Is there bug on this? > > No idea, it just got mentioned to me on irc. > > bug #337026 Actually, apparently that is a different (but related) bug, the truncation problem has actually been fixed in CVS by this change (didn't have a bug number): 2006-04-03 Michael Smith <msmith@fluendo.com> * ext/ogg/gstoggmux.c: (gst_ogg_mux_queue_pads): Oggmux sucks. Make it suck slightly less by writing out the final page. Still can't encode a vorbis-in-ogg file correctly, though.
(In reply to comment #6) > Created an attachment (id=62995) [edit] > working version > > Using gst-plugin-base from cvs (to get the newer vorbis and ogg stuff) this now > works well for me, modulo the known issues with oggmux taking bits off the end Current patch doesn't apply against CVS of gst-plugins-bad, probably needs some autotools updates: $ patch -p0 < gpbad-vorbistag.patch patching file configure.ac Hunk #1 FAILED at 650. Hunk #2 FAILED at 854. 2 out of 2 hunks FAILED -- saving rejects to file configure.ac.rej patching file ext/Makefile.am Hunk #1 succeeded at 215 with fuzz 2 (offset 13 lines). Hunk #2 FAILED at 270. Hunk #3 FAILED at 291. 2 out of 3 hunks FAILED -- saving rejects to file ext/Makefile.am.rej patching file ext/vorbistag/Makefile.am patching file ext/vorbistag/vorbistag.c
Created attachment 63535 [details] [review] Updated patch to CVS This works for me. Also preserves existing tags that aren't changed.
I've now got a working patch for Rhythmbox that uses this to add support for editing tags in Ogg Vorbis files (bug 339878). Issues: * What should the element do with tags sent down the pipeline? The docs don't seem to be clear on what to do, and what the merge-mode means, in a three-way merge of pipeline, internal, and application tags. Currently the element just ignore pipeline tags. * What tests should be added for the element? A few possible ones spring to mind: a) tagging an untagged vorbis stream, b) removing tags from a tagged vorbis stream, and c) retagging and tagged vorbis stream In each of the cases checking that the tags in the output stream are correct.
This works for me, any chance that this might be committed so at least it can be tested in CVS -bad? (which doesn't seem very risky since it's both in CVS and in -bad).
Very clean and nice code. Looks generally fine to me. Some random thoughts though: - is there a particular reason why you've decided to go for an element that processes and retags a raw vorbis stream rather than one that operates on a raw ogg stream? (both have advantages/disadvantages, I was just wondering what your reasons where or if this has been discussed on IRC or elsewhere before). - if the element is to be implemented as a vorbis stream retagger maybe it should derive from the already existing vorbisparse element? There are extra things to consider, like stream header packets being part of the caps etc. (apologies if all that came up before and was rejected as an idea, I haven't been following this discussion up until now) - in the current implementation in gst_vorbis_tag_chain() new_buf should probably be stamped with the same values for timestamp / duration / offset / offset_end etc. as the incoming vorbis comments packet buffer. > What should the element do with tags sent down the pipeline? The docs don't > seem to be clear on what to do, and what the merge-mode means, in a three-way > merge of pipeline, internal, and application tags. Currently the element just > ignore pipeline tags. I think that's okay, after all you get the upstream tags as part of the stream as a vorbis comments packet/header buffer. I'd just treat the existing tags as they are in the vorbis comment buffer the same way tag events are usually handled. > What tests should be added for the element? A few possible ones spring to > mind: > a) tagging an untagged vorbis stream, > b) removing tags from a tagged vorbis stream, and > c) retagging and tagged vorbis stream > > In each of the cases checking that the tags in the output stream are correct. Sounds good.
(In reply to comment #17) > - is there a particular reason why you've decided to go for an element > that processes and retags a raw vorbis stream rather than one that > operates on a raw ogg stream? (both have advantages/disadvantages, > I was just wondering what your reasons where or if this has been > discussed on IRC or elsewhere before). I don't think I'd had a real discussion about this with anyone. My reasoning for operating on vorbis streams rather than Ogg streams was that I thought it would make it easier to support re-tagging Ogg files that aren't just a single Vorbis stream. Applications use the current incarnation of the voristag element by creating a pipline with "src ! oggdemux oggmux ! sink" and listening for oggdemux's pad-added signal. In response to the signal, it would look at the caps and plug the appropriate tagging element (vorbistag for Vorbis streams) between oggdemux and oggmux. I thought that this should work for Ogg files with other (e.g. Speex) streams, if the appropriate tagging element was written, and maybe for Oggs with multiple vorbis streams. The above could probably be encapsulated in an "oggtag" element rather than every application, bonus points if it didn't hard-code the tagging elements. > - if the element is to be implemented as a vorbis stream retagger > maybe it should derive from the already existing vorbisparse > element? There are extra things to consider, like stream > header packets being part of the caps etc. > (apologies if all that came up before and was rejected as > an idea, I haven't been following this discussion up until now) Currently vorbistag requries a vorbisparse element before it, to fix all that up. Deriving from vorbisparse is probably a better idea, I'll look into it. > - in the current implementation in gst_vorbis_tag_chain() new_buf > should probably be stamped with the same values for timestamp / > duration / offset / offset_end etc. as the incoming vorbis > comments packet buffer. I'll look at fixing that too.
Created attachment 65806 [details] [review] derive for vorbisparse This patch makes vorbistag derive from vorbisparse, so application don't need to add a vorbisparse element to the pipeline. Since doing that requires vorbisparse.h, this patch is against -base not -bad. I've also made it copy timestamps/offsets/et al, and fixed a small leak.
Created attachment 65937 [details] [review] include start of tests Adds the three tests mentioned above, and a documentation section.
Created attachment 66111 [details] [review] small fixes A few minor fixes.
Looks great to me. Two minor nitpicks: - you can use gst_buffer_stamp() to transfer the stamps from one buffer to another. - you can probably get rid of the ogg_packet stuff and just check *GST_BUFFER_DATA(buf) directly. Now we just need to make sure it's ok to commit this directly to -base ..
Created attachment 66168 [details] [review] more small fixes Now correctly clean up the vorbis stuff used to craft the setup and audio buffers, and include the above two suggestions. Valgrind is reporting one memory leak for me, and I'm not sure what causes it: ==10135== 24 bytes in 2 blocks are definitely lost in loss record 2 of 24 ==10135== at 0x401B422: malloc (vg_replace_malloc.c:149) ==10135== by 0x43A6FE1: g_malloc (gmem.c:131) ==10135== by 0x43B5B84: g_slice_alloc (gslice.c:777) ==10135== by 0x439D89C: g_list_prepend (glist.c:95) ==10135== by 0x47AA0B1: gst_tag_to_vorbis_comments (gstvorbistag.c:385) ==10135== by 0x47AA26D: write_one_tag (gstvorbistag.c:398) ==10135== by 0x41BBE63: ??? (gsttaglist.c:795) ==10135== by 0x41B6611: gst_structure_foreach (gststructure.c:800) ==10135== by 0x41BBEBE: gst_tag_list_foreach (gsttaglist.c:820) ==10135== by 0x47AA344: gst_tag_list_to_vorbiscomment_buffer (gstvorbistag.c:442) ==10135== by 0x47A4B4A: gst_vorbis_tag_chain (vorbistag.c:148) ==10135== by 0x41A9803: gst_pad_chain (gstpad.c:3221)
This is currently blocking on the oggmuxer, which needs to be fixed to set the EOS flag correctly on the last ogg page (packet?) for each stream.
Ping? Any chance that this could be committed soon?
As Tim said, this is blocked by bug 337026 (oggmux's EOS handling).
Created attachment 73634 [details] [review] updated patch Syncs patch to cvs. Since the oggmux fix in now in cvs, it would be good if I could get this reviewed. Since it derived from vorbisparse, it would need to go in -base (unless vorbisparse.h got exported).
Will this work on flac files with vorbis tags?
> Will this work on flac files with vorbis tags? It won't, no. A flac retagging element should be fairly easy to write though.
IIRC 0.8 had a "flactag" element that could probably be ported.
Thanks for the patch, committed to CVS: 2006-10-03 Tim-Philipp Müller <tim at centricular dot net> Patch by: James "Doc" Livingston <doclivingston at gmail com> * ext/vorbis/Makefile.am: * ext/vorbis/vorbis.c: (plugin_init): * ext/vorbis/vorbisparse.c: (gst_vorbis_parse_class_init), (vorbis_parse_parse_packet), (vorbis_parse_chain): * ext/vorbis/vorbisparse.h: * ext/vorbis/vorbistag.c: (gst_vorbis_tag_base_init), (gst_vorbis_tag_class_init), (gst_vorbis_tag_init), (gst_vorbis_tag_parse_packet): * ext/vorbis/vorbistag.h: Add new vorbistag element which derives from vorbisparse and is essentially the same as well, only that it implements the GstTagSetter interface and can modify the stream's vorbiscomment on the fly (#335635). * tests/check/Makefile.am: * tests/check/elements/.cvsignore: * tests/check/elements/vorbistag.c: (setup_vorbistag), (cleanup_vorbistag), (buffer_probe), (start_pipeline), (get_buffer), (stop_pipeline), (_create_codebook_header_buffer), (_create_audio_buffer), (GST_START_TEST), (vorbistag_suite): Add unit test for new vorbistag element. with some minor changes: two fixes in the gobject macros, one in the boilerplate macro (we derive from GstVorbisParse, not GstElement), and I added a ::parse_packet vfunc to vorbis parse so we don't have to do the whole save-the-old-chain-function business.