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 334375 - [id3demux] [id3v2mux] ID3 tag rewriting is lossy
[id3demux] [id3v2mux] ID3 tag rewriting is lossy
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal major
: 0.10.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 323658
Blocks: 76524
Reported: 2006-03-13 05:02 UTC by Alex Lancaster
Modified: 2006-07-26 10:09 UTC
See Also:
GNOME target: ---
GNOME version: ---

Partial patch handles re-encoding via taglib (1.58 KB, patch)
2006-07-25 08:48 UTC, Alex Lancaster
none Details | Review
Make id3v2mux handle multiple tags of the same type and inject ID3v2 frames from blobs (21.92 KB, patch)
2006-07-25 17:28 UTC, Tim-Philipp Müller
committed Details | Review
Fix handling of comments (COMM) (22.48 KB, patch)
2006-07-26 01:42 UTC, Alex Lancaster
none Details | Review
Minor fix to not pass tag name to add_comment_tag() (22.47 KB, patch)
2006-07-26 01:48 UTC, Alex Lancaster
none Details | Review
better comments handling (1.95 KB, patch)
2006-07-26 09:20 UTC, Tim-Philipp Müller
committed Details | Review

Description Alex Lancaster 2006-03-13 05:02:39 UTC
+++ This bug was initially created as a clone of Bug #323658 +++

ID3 tag rewriting using either the id3mux or tagid3v2mux plugins will lose valid ID3v2.4.0 tags that gstreamer doesn't explicitly have a GST_TAG_ for, for example "TCOM" (for composer) or "TENC" (for encoding).

Example, using the following file (with ID3v2.3 tags):

Retagged using id3mux plugin using this pipeline:

gst-launch-0.10 filesrc location=V2.mp3 ! id3demux ! id3mux ! filesink location=V2-id3mux.mp3

results in the file in attachment #60953 [details].

Retagged using the tagid3v2mux plugin with this pipeline

gst-launch-0.10 filesrc location=V2.mp3 !  id3demux ! tagid3v2mux ! filesink location=V2-taglib.mp3

results in the file in attachment #60950 [details].
Comment 1 Alex Lancaster 2006-03-13 05:04:59 UTC
As a possible fix Christophe (teuf) and have Jan Schmidt (thaytan) have proposed adding support to id3demux to forward unknown tags as GST_TAG_UNKNOWN.

Relevant conversation on #rhythmbox on IRC, teuf (Christophe) and blarney (Alex L):

(02:35:28) teuf: the scheme described by thaytan for id3demux could be used by taglibmux however I think
(02:35:51) blarney: what was that?
(02:35:53) teuf: (taglibn seems to be able to parse an mp3 frame from a memory buffer, but not a full tag)
(02:36:15) teuf: forward unknown frames as TAG_UNKNOWN tags containing binary data
(02:36:21) teuf: and letting other elements handling them if they can
(02:36:47) blarney: so this would mean writing a tagid3v2demux element?
(02:37:10) teuf: nope, this would mean waiting for thaytan to add support for that in id3demux
(02:37:15) teuf: and then handle those unknown tags in taglib
(02:37:21) teuf: taglibmux I mean
(02:37:24) blarney: oh, ok
(02:38:11) blarney: so the same thing could be done with id3mux I guess, unless we want to drop support for it
Comment 2 Alex Lancaster 2006-03-13 08:25:48 UTC
Suggestion from Tim-Philipp Müller from bug #333501:

> 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.
Comment 3 Christophe Fergeau 2006-03-13 08:34:55 UTC
Fwiw, I feel approach (a) is cleaner (and probably trivial to implement when there is a working tagid3v2demux), but approach (b) would work too. I started to work on a tagid3v2demux, the code is mostly done, but I have to figure out why taglib can't parse the id3v2 tag it's fed with.
Comment 4 Tim-Philipp Müller 2006-05-04 08:52:26 UTC
Over to -good now that id3v2mux is there too.
Comment 5 Alex Lancaster 2006-05-04 09:18:08 UTC
(In reply to comment #4)
> Over to -good now that id3v2mux is there too.

Has tagid3v2mux been renamed to id3v2mux?  

Given that this has been switched to -good I assume that this lossless tag rewriting, if it implemented is going to be via a combination of id3demux and id3v2mux and will not include id3mux?  Is there any progress on this?
Comment 6 Tim-Philipp Müller 2006-05-04 09:31:28 UTC
Yes, it's been renamed to id3v2mux (sorry for the confusion).

Lossless tag rewriting is going to be a combination of id3demux and id3v2mux. I don't think it will include the current (libid3tag-based) id3mux element in its present form, no. However, my hope is that we will get rid of the id3mux element in its current incarnation completely and replace it with something API compatible under the same name based on the taglib stuff in -good (that could be just id3v2mux registering itself under an additional element name or a bin containing id3v2mux and a new id3v1mux or something, who knows).

No progress on this yet, but now that the taglib plugin is in -good things should be much easier to do. Hopefully we might get this into the next -good release (after the pending one).
Comment 7 Alex Lancaster 2006-07-19 11:36:18 UTC
See bug #347091 for a solution for OGG vorbis tags.
Comment 8 Tim-Philipp Müller 2006-07-23 10:59:51 UTC
Step 1:

 2006-07-23  Tim-Philipp Müller  <tim at centricular dot net>

        * gst/id3demux/id3tags.c:
          Put ID3v2 frames we can't parse as binary blobs into private
          tags, so that they are not lost when retagging, at least once
          id3v2mux has been taught to re-inject those frames again.
          See bug #334375.
Comment 9 Alex Lancaster 2006-07-25 08:48:36 UTC
Created attachment 69560 [details] [review]
Partial patch handles re-encoding via taglib

This is a partial patch handling the re-encoding the binary blob via taglib.  Tim is working on the other part which is to handle multiple binary tags (including binary blobs).  This current patch will only re-encode the first binary blob it finds.
Comment 10 Tim-Philipp Müller 2006-07-25 17:28:40 UTC
Created attachment 69596 [details] [review]
Make id3v2mux handle multiple tags of the same type and inject ID3v2 frames from blobs

Patch against -good CVS. Rewrites id3v2mux tag render logic to handle multiple occurrences of the major tags that can occur multiple times; also makes id3v2mux re-inject unhandled ID3v2 frames from binary blobs as extracted by id3demux from CVS (frames will be converted by taglib to ID3v2.4.0 format though, there's not much we can do about that).
Comment 11 Alex Lancaster 2006-07-26 01:42:42 UTC
Created attachment 69623 [details] [review]
Fix handling of comments (COMM)

Comments use the COMM tag, not TCOM (which is composer).  I added a new method add_comment_tag() because they need to use the handling using the special setComment() method.

A setText() on a generic TextIdentificationFrame doesn't work properly.
Comment 12 Alex Lancaster 2006-07-26 01:48:30 UTC
Created attachment 69624 [details] [review]
Minor fix to not pass tag name to add_comment_tag()
Comment 13 Tim-Philipp Müller 2006-07-26 09:18:54 UTC
Great, thanks for testing. Committed my original version:

 2006-07-26  Tim-Philipp Müller  <tim at centricular dot net>

        * ext/taglib/
          Handle multiple tags of the same type properly. Re-inject
          unparsed ID3v2 frames that we get as binary blobs from
          id3demux into the tag again so we don't lose information
          when retagging (#334375).

You're right about the comment tag handling, that needs to be fixed. I would prefer to keep separate comments as separate strings/frames though instead of just concatenating all the comment strings into one string, if that's possible.

Will attach possible patch for that in a second.
Comment 14 Tim-Philipp Müller 2006-07-26 09:20:20 UTC
Created attachment 69648 [details] [review]
better comments handling
Comment 15 Alex Lancaster 2006-07-26 09:55:50 UTC
Works for me, modulo not preserving some iTunes specific metadata.
Comment 16 Tim-Philipp Müller 2006-07-26 10:09:21 UTC
Committed this one as well:

 2006-07-26  Tim-Philipp Müller  <tim at centricular dot net>

       * ext/taglib/
         Fix writing of comment frames (should be COMM not TCOM),
         is still sub-optimal though, since we don't retain or
         extract the comment descriptions properly (#334375,
         also see #334375).

I think this bug can be closed now.

I've opened bug #348762 for the additional comments handling improvements needed.