GNOME Bugzilla – Bug 599759
[qtdemux] Add support for more tags
Last modified: 2009-12-21 22:30:23 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.
Created attachment 146331 [details] [review] patch against 0.10.16 this patch is tested but out-of-date
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.
(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.
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.
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.
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.
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.
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.
Also, it preserves information better when transcoding/transmuxing
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).
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)
(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)
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.
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.
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
Moved it to core as it turns out the critical part here are the new tags in it.
Comment on attachment 146331 [details] [review] patch against 0.10.16 commited with a few changes
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.