After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 339918 - GstTagSetter merge-mode description unclear, and implementations inconsistent
GstTagSetter merge-mode description unclear, and implementations inconsistent
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: 0.10.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 342330
 
 
Reported: 2006-04-27 12:48 UTC by James "Doc" Livingston
Modified: 2006-05-19 10:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for taglib-based element (1.33 KB, patch)
2006-04-28 14:25 UTC, James "Doc" Livingston
committed Details | Review
patch for id3mux (501 bytes, patch)
2006-04-28 14:27 UTC, James "Doc" Livingston
committed Details | Review
patch for docs (543 bytes, patch)
2006-04-28 14:35 UTC, James "Doc" Livingston
committed Details | Review

Description James "Doc" Livingston 2006-04-27 12:48:49 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.
Comment 1 James "Doc" Livingston 2006-04-28 14:25:27 UTC
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.
Comment 2 James "Doc" Livingston 2006-04-28 14:27:08 UTC
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.
Comment 3 James "Doc" Livingston 2006-04-28 14:35:41 UTC
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.
Comment 4 Tim-Philipp Müller 2006-05-18 14:02:20 UTC
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.

Comment 5 Alex Lancaster 2006-05-19 05:13:16 UTC
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.
Comment 6 Alex Lancaster 2006-05-19 05:19:55 UTC
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.  
Comment 7 James "Doc" Livingston 2006-05-19 05:35:18 UTC
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.
Comment 8 Alex Lancaster 2006-05-19 07:06:10 UTC
(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.
Comment 9 James "Doc" Livingston 2006-05-19 08:55:56 UTC
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.
Comment 10 Alex Lancaster 2006-05-19 09:26:22 UTC
(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. 
Comment 11 Tim-Philipp Müller 2006-05-19 09:39:14 UTC
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?

Comment 12 James "Doc" Livingston 2006-05-19 10:02:38 UTC
(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).