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 599759 - [qtdemux] Add support for more tags
[qtdemux] Add support for more tags
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-10-27 12:42 UTC by Jonathan Conder
Modified: 2009-12-21 22:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for gst-plugins-good (git) (9.54 KB, patch)
2009-10-27 12:42 UTC, Jonathan Conder
none Details | Review
patch against 0.10.16 (9.89 KB, patch)
2009-10-27 12:42 UTC, Jonathan Conder
committed Details | Review

Description Jonathan Conder 2009-10-27 12:42:00 UTC
Created attachment 146330 [details] [review]
Patch for gst-plugins-good (git)

The attached patch adds support for the Album Artist tag and various Sort Name
tags for iTunes-tagged aac/m4a files to qtdemux, which I rely on for my music
sorting.

Also contained is prelimary support for almost all tags iTunes can handle (e.g.
TV Show related stuff, Lyrics etc.) but I left this commented/ifdef'd out
because these tags have no equivalent within gstreamer at this time. I may also
do a patch to resolve this but I'm not entirely sure what to call these tags so
I thought it would be best to leave it to the developers. Additionally, there
are some pointless tags (e.g. podcast, gapless) that probably aren't worth
including, and others like compilation which might be better folding into the
album artist tag.

The patch is against the current git revision but I haven't tested it due to
the hassle of installing gstreamer-git on my system. However, I have tested a
0.10.16 patch that is basically the same.

I hope the patch is satisfactory, and I would be happy to help out with any
changes that need to be done and/or similar patches for other formats.
Comment 1 Jonathan Conder 2009-10-27 12:42:56 UTC
Created attachment 146331 [details] [review]
patch against 0.10.16

this patch is tested but out-of-date
Comment 2 Jonathan Conder 2009-10-27 13:16:10 UTC
Also, I forgot to mention I fixed up a couple of problems with the current plugin, namely that "Grouping" was pointing to GST_TAG_ARTIST for some reason, as was "Performer", and "Tool" was pointing to GST_TAG_COMMENT rather than GST_TAG_ENCODER. Some additional processing might be warranted to separate this out into GST_TAG_ENCODER and GST_TAG_ENCODER_VERSION. FYI the string looks something like this: "iTunes 9.0.1.8, QuickTime 7.6.4", so I'm not really sure how to turn it into an integer version number and whether the iTunes or QuickTime version should be used, so it might be easier just to leave it as-is.
Comment 3 Jonathan Conder 2009-10-27 23:50:11 UTC
(In reply to comment #2)
> pointing to GST_TAG_ARTIST for some reason, as was "Performer"

Forget that, I think FOURCC_perf is actually the "Artist" tag for certain 3gp files.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2009-11-12 12:30:34 UTC
patch looks good. Some nitpicks. We don't use // comments. If you change something, don't comment out the old version, just replace (we can see the change in the patch). If you want to prepare the addition of something (e.g. GST_TAG_GROUPING) then please use /* */ comment.

Btw. whats the semantics for "_grp"? If you happen to know a matching tag that is part of e.g. ID3 or other tag systems, we can add GST_TAG_GROUPING to core.
Comment 5 Jonathan Conder 2009-11-12 13:17:02 UTC
Thanks for that. Ok, I wasn't aware of the // issue. However, I didn't intend that the comments would be merged, it's really just to indicate which ones aren't defined in GStreamer yet. I think grouping was the only old version I commented out, but again only to indicate that GST_TAG_GROUPING does not exist. Anyway I'll prepare a proper (ready-to-merge) patch if you want, but I would prefer to leave that until the new tags have been defined in core.

In iTunes, "_grp" is just called "Grouping" in the tag editor. I did a few searches and it looks like it's supposed to be to group together various movements in classical music, but many people use it differently. TIT1 from ID3 looks like the relevant equivalent, but this may be already set to a different GStreamer tag in id3demux (I couldn't tell from the source).

From looking at the ID3 specs it appears that the "album" tag and "tv show" tags are the same in ID3, and the same could apply here (although both might be set at once, so the TV show tag should probably go below album in qtdemux.c). From this perspective FOURCC_sosn, FOURCC_tves and FOURCC_tvsn could correspond to GST_TAG_ALBUM_SORTNAME, GST_TAG_TRACK_NUMBER and GST_TAG_ALBUM_VOLUME_NUMBER respectively. The other one (FOURCC_tven) is probably redundant, as far as I can tell, as are FOURCC_pgap and FOURCC_pcst.

I think the compilation tag would be useful, but it might be better to respond to it by setting GST_TAG_ALBUM_ARTIST to "Various Artists" or "Compilation" or some similar, possibly locale-specific value. What do you think? After that all that remains would be to add GST_TAG_COMPOSER_SORTNAME, GST_TAG_LYRICS and possibly GST_TAG_GROUPING to core.
Comment 6 Thiago Sousa Santos 2009-12-17 09:55:36 UTC
Patch looks good (after the // removals).
I also notice some tv/seasons tags that we don't have mapped and might be worth adding to core.
Comment 7 Jonathan Conder 2009-12-17 10:11:58 UTC
Thank you :)
Yes well to clarify what I said above we could also map them like this:

tv show = album
sort tv show = album sortname
episode number = track number
season number = album volume number
episode id = ignore

But I don't mind either way. The thing is, if ID3 tags are being used for other video formats then applications are going to want to check the album-related tags anyway, because AFAIK there's no other way to do it with ID3. Also I don't think any information would be lost if we do this.

Anyway i'll probably have some free time soon to clean up the patch so it's ready to merge if/when the new tags get into core. I'll write a patch for that too if needed, but since I'm unfamiliar with the code it might be better if I leave it to someone else.
Comment 8 Tim-Philipp Müller 2009-12-17 10:28:59 UTC
I think we should add new tags for these things. It's nicer and cleaner, and allows for the heavy lifting to be done in GStreamer rather than applications if it turns out that people really do tag movie containers with ID3.
Comment 9 Tim-Philipp Müller 2009-12-17 10:29:51 UTC
Also, it preserves information better when transcoding/transmuxing
Comment 10 Jonathan Conder 2009-12-17 10:43:08 UTC
Ok, that makes sense. According to wikipedia ID3 is practically unused outside audio containers, so I guess my points didn't matter anyway. I'll do that when I clean up the patch. So at this stage I'm looking at the following tag names, let me know if anything should be changed:

GST_TAG_COMPOSER_SORTNAME (string)
GST_TAG_LYRICS (string)
GST_TAG_GROUPING (string)

GST_TAG_TV_SHOW (string)
GST_TAG_TV_SHOW_SORTNAME (string)
GST_TAG_TV_EPISODE_NUMBER (unsigned integer)
GST_TAG_TV_SEASON_NUMBER (unsigned integer)

P.S. I will probably write a patch for qtmux as well (didn't realise it existed when I reported this).
Comment 11 Thiago Sousa Santos 2009-12-17 10:46:55 UTC
There's also episode-id, I'm not sure if we could map that to 'title' or we should have 'episode-id' as well.

(Just thinking out loudly)
Comment 12 Thiago Sousa Santos 2009-12-17 10:49:51 UTC
(In reply to comment #11)
> There's also episode-id, I'm not sure if we could map that to 'title' or we
> should have 'episode-id' as well.

I'm assuming episode-id is also the title/name of the episode and not that S01E23 or something similar namings for episodes. If not, ignore me.


> 
> (Just thinking out loudly)
Comment 13 Jonathan Conder 2009-12-17 10:54:53 UTC
right, forgot about that. I have no idea what its used for but it's a string and SxxExx seems like a good use for it to me. It isn't title because there's already a tag for that. I might see if there are any cheap tv shows on itunes just to look at the tags. need to reboot into windows though so not right now. so anyway, for now I guess GST_TAG_TV_EPISODE_ID (string) is another candidate. I also forgot about GST_TAG_COMPILATION (boolean) which I'm not sure whether to include or not.
Comment 14 Tim-Philipp Müller 2009-12-17 11:19:51 UTC
IIRC there are production codes/IDs that don't map to S**E** or *x** (see e.g. http://epguides.com/simpsons/). I'm not convinced we need to have separate tags for those, but then tags don't really cost us much, so may just as well add it if you want to. (Could always put it into an extended comment tag if we don't add a separate tag for it).

For episode titles we should just use GST_TAG_TITLE in my opinion.
Comment 15 Thiago Sousa Santos 2009-12-21 17:11:35 UTC
The commits bellow should fix it.

I left out 'gapless', 'podcast', 'compilation' and 'episode-id'. If you really need them, please reopen. Also added them to qtmux.

Module: gstreamer
Branch: master
Commit: a3078cf0cdb65c0080841c69aa21a6261e519ded
URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=a3078cf0cdb65c0080841c69aa21a6261e519ded

Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Mon Dec 21 11:58:12 2009 -0300

gsttaglist: Adds new tags

Adds the following new tags:
GST_TAG_SHOW_NAME
GST_TAG_SHOW_SORTNAME
GST_TAG_SHOW_EPISODE_NUMBER
GST_TAG_SHOW_SEASON_NUMBER
GST_TAG_LYRICS
GST_TAG_COMPOSER_SORTNAME
GST_TAG_GROUPING

Fixes #599759

Module: gst-plugins-good
Branch: master
Commit: 1112090589c68c6ac46118db6dff8928bc784f13
URL:    http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=1112090589c68c6ac46118db6dff8928bc784f13

Author: Jonathan Conder <j@skurvy.no-ip.org>
Date:   Mon Dec 21 12:01:53 2009 -0300

qtdemux: Adds new tags

Adds some new tags mapping to qtdemux.

Fixes #599759


Module: gst-plugins-bad
Branch: master
Commit: 80a192b8257c5309e7c1d348cd82624258318e72
URL:    http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=80a192b8257c5309e7c1d348cd82624258318e72

Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Mon Dec 21 12:05:37 2009 -0300

qtmux: Adds new tags

Maps more tags that are already posted by qtdemux

Fixes #599759
Comment 16 Thiago Sousa Santos 2009-12-21 17:15:40 UTC
Moved it to core as it turns out the critical part here are the new tags in it.
Comment 17 Thiago Sousa Santos 2009-12-21 17:16:35 UTC
Comment on attachment 146331 [details] [review]
patch against 0.10.16

commited with a few changes
Comment 18 Jonathan Conder 2009-12-21 22:30:23 UTC
Thank you very much :). Well I would quite like a 'compilation' tag but I'm not sure how GStreamer should handle it so I'm not going to reopen. Just one minor issue (which is my fault) the FOURCC_perf tag should be GST_TAG_ARTIST, as I mentioned above. Unfortunately I didn't realise this until I'd submitted the patch already. Anyway, thanks for your time and effort.