GNOME Bugzilla – Bug 339918
GstTagSetter merge-mode description unclear, and implementations inconsistent
Last modified: 2006-05-19 10:02:38 UTC
Currently the documentation on gst_tag_setter_set_tag_merge_mode() is missing a number of words, and so makes it unclear what the merge mode should mean. This is particularly bad since different plugins make it mean different things. "The default is GST_TAG_MERGE_KEEP, which keeps the tags by this interface and discards tags from events." Consider when a tag "artist = Foo" comes doen the pipeline as an event, and another tag "artist = Bar" is set on the tag-setter interface. What do KEEP and REPLACE mean? id3mux and the taglib id3 muxer currently make KEEP mean "keep event tags and discard interface tags" and REPLACE mean "replace event tags with interface tags" vorbisenc, flacenc and speexenc currently make it mean the opposite: KEEP="keep interface tags and discard event tags" and REPLACE="replace interface tags with event tags". The default mode is KEEP, and so having that discard interface tags would mean that the default mode is to make the tag-setter interface do nothing. Having it use the tags you have it (by default) probably makes more sense. Changing either of the groups interpretations could potentially break applications that rely on the broken behaviour. All released versions of Rhythmbox that support ID3 tag editing set the mode to REPLACE (because that's what worked with id3mux), and there may be apps that depend on vorbisenc et al's interpretation being correct. Leaving it as-is would require apps to special-case elements to set the right mode, which would also be bad.
Created attachment 64466 [details] [review] patch for taglib-based element After discussion with Company on IRC, it seems the vorbisenc way is correct. This patch fixes the taglib-based id3 muxer.
Created attachment 64467 [details] [review] patch for id3mux And the patch for id3mux. There may be more elements that need fixing, I haven't checked.
Created attachment 64468 [details] [review] patch for docs This patch changes the docs for the gst_tag_setter_set_tag_merge_mode function to be a real sentence and now have word(s) missing. Whether there is a better wording, I'm not sure.
Thanks for the patches, updated where necessary (taglib patch) and committed to CVS: core: 2006-05-18 Tim-Philipp Müller <tim at centricular dot net> * gst/gsttagsetter.c: Docs additions and fixes. gst-plugins-good: 2006-05-18 Tim-Philipp Müller <tim at centricular dot net> Patch by: James "Doc" Livingston <doclivingston gmail com> * ext/taglib/gsttaglibmux.c: (gst_tag_lib_mux_render_tag): Merge event tags and tag setter tags correctly (#339918). Also, don't leak taglist in case of an error. gst-plugins-ugly: 2006-05-18 Tim-Philipp Müller <tim at centricular dot net> Patch by: James "Doc" Livingston <doclivingston gmail com> * ext/mad/gstid3tag.c: (gst_id3_tag_get_tag_to_render): Do tag merging correctly (#339918). Output taglists properly in debug statements too while we're at it.
Doesn't this mean that rhythmbox (and any other apps that use either id3mux or id3v2mux) will need to be updated? Because they still rely on the old behaviour. Shouldn't there be an API number bump because of this, because versions of the plugins within 0.10.x won't necessarily be consistent. This could cause a big problem for users updating gstreamer without upgrading the application that uses the plugin with the new behaviour, because it won't necessarily force an upgrade in the application because they are still ABI compatible.
e.g. I have rhythmbox-0.9.4.1 and gstreamer-plugins-ugly-0.10.3 (with id3mux or id3v2mux with old behaviour) . rhythmbox-0.9.5 is upgraded to use the new behaviour of 0.10.4 id3mux or id3v2mux. A new version of gst-plugins-ugly-0.10.4 comes out with the new behaviour, I upgrade to this new version, but I still have rhythmbox-0.9.4.1, and so tag writing will (silently) break. Bad situation. If the ABI version was bumped to 0.11.0 (say) for -ugly then it would refuse to install because it would mean removing rhythmbox 0.9.4.1, which is better than the silently failing situation.
As far as anyone knows Rhythmbox is the only app depending on the broken behaviour. So the breakage is if a) you are running gst-plugins-ugly with the change, and b) you are running Rhythmbox without a version check, with tag-editing enabled, then tag changes to MP3s aren't written back into files. Several music players which let you change tags don't write them back into files (including Banshee, last time I checked), and it wasn't enabled by default in RB, so it probably isn't a huge problem.
(In reply to comment #7) > As far as anyone knows Rhythmbox is the only app depending on the broken > behaviour. So the breakage is if a) you are running gst-plugins-ugly with the > change, and b) you are running Rhythmbox without a version check, with > tag-editing enabled, then tag changes to MP3s aren't written back into files. My understanding was the plugin interface was supposed to be stable within a given stable series of gstreamer (e.g. 0.8 and 0.10) and that new features could be added with the series, but that changes would never be incompatible so that any plugin version could be interchanged with any other plugin version. The existing rhythmbox in CVS (and 0.9.4 and 0.9.4.1) checks for a minimum version of id3mux, not for a specific version of id3mux. CVS rhythmbox should be updated to use the new interface for gst-plugins-ugly > 0.10.3 and use the old interface for 0.10.2 <= gst-plugins-ugly <= 0.10.3. That way rhythmbox CVS and >= 0.9.5 will always Do the Right Thing regardless of the installed gst-plugins-ugly. Then the only potential problems will be with 0.9.4 and 0.9.4.1. <= 0.9.3 isn't a problem because tag writing wasn't ever enabled with gstreamer 0.10.
Yes, the plugin interface is supposed to remain stable - however plugins also need to follow the the specs that say what the GstTagSetter interface means. Is id3mux getting the merge modes arse-backwards part of the API guarantee, or is it just a bug? If it's just a bug then fixing it isn't a problem. If changing it breaks the API guarantee, then we'd need to have "it works this way, except for id3mux which is broken" in the API docs - which would be *very* sucky.
(In reply to comment #9) > Yes, the plugin interface is supposed to remain stable - however plugins also > need to follow the the specs that say what the GstTagSetter interface means. Is > id3mux getting the merge modes arse-backwards part of the API guarantee, or is > it just a bug? > > If it's just a bug then fixing it isn't a problem. If changing it breaks the > API guarantee, then we'd need to have "it works this way, except for id3mux > which is broken" in the API docs - which would be *very* sucky. Fair enough, I filed bug #342330 to fix rhythmbox as per my suggestions in comment #8.
Alex - just so I understand this correctly - are we talking (a) breakage in practice here (ie. happening with rhythmbox as it is usually compiled/shipped by default), or (b) breakage in corner cases (ie. someone compiled rhythmbox himself with the still experimental --enable-foobar option), or (c) mostly principle? Does the code in rhythmbox explicitly set the merge mode of id3mux to something (REPLACE) that is documented - and was already documented back then - as doing the exact opposite of what rhythmbox wants to do, with the documentation of gst_tag_setter_set_merge_mode() explicitly describing KEEP mode to be what rhythmbox wants to do? Is there a released version of rhythmbox yet that uses id3v2mux? (viewcvs didn't seem to feature anything id3v2mux-related) Would it help to keep the old broken semantics for "id3mux" and only change it for id3v2mux?
(In reply to comment #11) > (a) breakage in practice here (ie. happening with rhythmbox > as it is usually compiled/shipped by default), or > > (b) breakage in corner cases (ie. someone compiled rhythmbox > himself with the still experimental --enable-foobar option), or > > (c) mostly principle? It only affects RB 0.9.4 (earlier versions didn't support tag-writing with 0.10) with --enable-tag-writing passed to configure (it is off by default); although I think a reasonable number of people compile with that option (such as seb128's 0.9.4.1 packages in his personal/non-official repo). That said, the only effect is that it won't actually use the new tags when rewriting - so it won't destroy files or anything. last time I checked, several other music players operated like this all the time, changing their db but not the files themselves. > Does the code in rhythmbox explicitly set the merge mode of id3mux to something > (REPLACE) that is documented - and was already documented back then - as doing > the exact opposite of what rhythmbox wants to do, with the documentation of > gst_tag_setter_set_merge_mode() explicitly describing KEEP mode to be what > rhythmbox wants to do? That's exactly what happens. Whoever originally wrote it must have had it not working, throught that they maybe mis-understood the docs, and tried REPLACE (which then worked). > Is there a released version of rhythmbox yet that uses id3v2mux? (viewcvs > didn't seem to feature anything id3v2mux-related) > > Would it help to keep the old broken semantics for "id3mux" and only change it > for id3v2mux? id3v2mux is the one that only got added recently, right? We don't use that yet (although I'll rememeber to do that once we have this issue sorted out).