Created attachment 69654 [details]
ID3v2.4 file with only one comment after retagging

1. View file in a tagger like tagtool (note "comment #1" and "comment #2" tags in file)
2. Compile rhythmbox with --enable-tag-writing
3. Add File to rhythmbox
4. Edit properties, change a tag (in this case "Year" to "1066").
5. View resulting file in a tagger, or use "hexdump -Cv" on the file, and only "comment #1"

Created attachment 69655 [details]
This is the original ID3v2.3 file with two comments *before* retagging

This file is the original file *before* retagging, attachment #69654 [details] is the file *after* retagging.

Note that using gst-plugins-good from CVS, the second comment is *not* lost, i.e. using the pipeline:

gst-launch filesrc location=multiple-comm-tags-orig.mp3 ! id3demux ! id3v2mux ! filesink location=multiple-comm-tags.mp3

(In reply to comment #0)
> preserve multiple comments, keeps only the first.

should read:
                             
preserve multiple comments, retagging by rhythmbox keeps only the first.

The only thing I can see that might affect this is that we call "gst_tag_setter_merge_tags (GST_TAG_SETTER (tagger), md->priv->tags, GST_TAG_MERGE_REPLACE_ALL);" (metadata/rb-metadata-gst.c:229), which may merge all the pipeline tags together.

Could you try changing that to GST_TAG_MERGE_REPLACE and see if that helps?

(In reply to comment #4)

> Could you try changing that to GST_TAG_MERGE_REPLACE and see if that helps?

Nope, didn't make any difference.  I tried GST_TAG_MERGE_APPEND, GST_TAG_MERGE_KEEP and it didn't appear to make any difference at all to the retagged file.

> Nope, didn't make any difference.  I tried GST_TAG_MERGE_APPEND,
> GST_TAG_MERGE_KEEP and it didn't appear to make any difference at all to the
> retagged file.

i.e. the second comment tag is lost. 

Retitling as this is a more general problem with tags that are handled by gstreamer, even if they aren't displayed in the UI.

As discussed on IRC the problem is that in RBMetaDataPrivate:

	GHashTable *metadata;

is a hash table that only handles one tag per value, (the tag name is the key, and the tag contents is the value).

Doc suggests:

> priv->metadata is what actually limits it to one, once that supported more we 
> could change the metadata to tags conversion

More specifically, we probably want to change rb_metadata_{get,set} to pass a GValueArray, instead of a GValue. Then it would be a matter of:


Making rb_metadata_gst_load_tag (rb-metadata-gst.c) convert all the tags in the list, and merging with existing values in priv->metadata (if any).

Change rb_metadata_gst_add_tag_data (rb-metadata-gst.c) to covert the whole array not just one value into the tag list.

Make rb_metadata_dbus_read_from_message (rb-metadata-dbus.c) convert to GValueArray not GValue.


For now, make set_props_from_metadata (rhythmdb.c) and just take the first value from the GValueArray.

This affects more than just ID3, also affects vorbis comments.  Updating summary accordingly.

Bug #362876 about APE tags is somewhat related to this bug.

Note also: http://live.gnome.org/Rhythmbox/MetadataPlans

Created attachment 82077 [details] [review]
some metadata backend updates

This implements a number of things from http://live.gnome.org/Rhythmbox/MetadataPlans:
- reading and writing multi-valued tags properly
- full typefinding (finds container type and audio+video media types)
- dropping ape tags in favour of anything else
- caching editable media types in rb-metadata-dbus-client to avoid spawning metadata helpers just to show properties windows
- optional use of decodebin2

Patch #82077 appears to solve the problem for tags (e.g. "TCOM" in id3v2 a.k.a. comment tags) that aren't exposed to the rhythmbox UI.

However it still doesn't keep multiple values for tags that are exposed to the UI (e.g. artist or title), modification of those tag types within rhythmbox will result in any extra (undisplayed, unmodified) tags to be lost.  To do this still requires the implementation of the GValueArray scheme outlined in comment #9.

Created attachment 82271 [details] [review]
destroy the apes

strips out ape tags when writing tags to mp3 files.

This patch implements almost exactly the scheme described in comment 9.  What it does not do is replace only the first value when multiple values exist.

(In reply to comment #16)
> This patch implements almost exactly the scheme described in comment 9.  What
> it does not do is replace only the first value when multiple values exist.

What would be required for it to only replace the first value? 

in flac_can_tag()
+	/* what possible container types are there for flac? */

You can put FLAC in at least Ogg containers, however we don't currently handle tagging them.


This looks okay to me, and improves our metadata handling a bit.


(In reply to comment #17)
> (In reply to comment #16)
> > This patch implements almost exactly the scheme described in comment 9.  What
> > it does not do is replace only the first value when multiple values exist.
> 
> What would be required for it to only replace the first value? 

You mean if a file has the artists "Foo" and "Bar", and you change "Foo" to "Baz" in the UI, you want it to end up with "Baz" and "Bar"? It would be fairly difficult to distinguish between when to do that and when to replace them all.

(In reply to comment #18)

> You mean if a file has the artists "Foo" and "Bar", and you change "Foo" to
> "Baz" in the UI, you want it to end up with "Baz" and "Bar"? It would be fairly
> difficult to distinguish between when to do that and when to replace them all.

Yes, exactly.  I can't see why in principle it couldn't be done, since rhythmbox only reads the first tag in the list, shouldn't it be possible to only rewrite the first tag in the list?  

The principle followed should be, if the metadata isn't exposed to the rhythmbox UI, it shouldn't be modified by changing in the rhythmbox UI.

Either way, this patch could be committed because it improves on the current behaviour (by keeping duplicate tags not exposed to the rhythmbox UI in the file).

No, this shouldn't be committed.  More thought needs to go into the audio format string - mp3 and aac are currently both considered 'audio/mpeg' which, while true, doesn't provide enough information for anything we use the field for.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/211.