GNOME Bugzilla – Bug 705999
taglist: handle publisher, interpreted-by and key tags
Last modified: 2013-08-20 12:46:30 UTC
First patch add support for publisher, interpreted-by and key tags. Second patch (related to gst-plugins-base) add support for these tags inside the id3 demuxer.
Created attachment 251620 [details] [review] taglist: handle publisher, interpreted-by and key tags
Created attachment 251621 [details] [review] tag: handle publisher, interpreted-by and key tags
Review of attachment 251620 [details] [review]: There seems to be some overlap between composer, performer and interpreted-by somehow. Not sure ::: gst/gsttaglist.h @@ +1062,3 @@ + * Musical key in which the sound starts (string with maximum length of 3) + */ +#define GST_TAG_KEY "key" This one should be in libgsttag in gst-plugins-base as it's media specific. Also just calling it KEY is a bit too generic. Also list examples of how it should look like or reference a standard. I assume this is something like "C3", "C#3", ie http://en.wikipedia.org/wiki/Scientific_pitch_notation?
Review of attachment 251621 [details] [review]: Looks good. Maybe also add to qtdemux, matroskademux, apedemux and others? :)
Created attachment 251720 [details] [review] taglist: handle publisher and interpreted-by tags
Created attachment 251721 [details] [review] tag: add musical-key tag
Created attachment 251722 [details] [review] tag: id3: handle publisher, interpreted-by and musical-key tags
Created attachment 251723 [details] [review] id3mux: handle publisher, interpreted-by and musical-key tags
Thanks for the review. New patches attached: GST_TAG_KEY renamed to GST_TAG_MUSICAL_KEY to gst-plugin-base/libgsttag whith better documentation. Composer, interpreted-by, and performer should not overlap since they are different (but close) as far as i understand: - interpreted-by refers to a remixer for example, - composer the person who wrote the song, - performer may refer to the artist who performed the orginal song (might be the same as the artist). I also added a patch to handle those new tags in id3mux. I will also add support for those new tags in the other demuxers/muxers.
Review of attachment 251720 [details] [review]: ::: gst/gsttaglist.h @@ +1055,3 @@ + * Information about the people behind a remix and similar + interpretations of another existing piece (string) + */ Add Since: 1.2 markers to both new tags
Review of attachment 251721 [details] [review]: ::: gst-libs/gst/tag/tag.h @@ +85,3 @@ + * GST_TAG_MUSICAL_KEY: + * + * Musical key in which the sound starts. It is represented as a string Should it be the key in which the song (sound?) starts or the main key of the song? @@ +88,3 @@ + * with a maximum length of three characters. The ground keys are + * represented with "A","B","C","D","E", "F" and "G" and halfkeys + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). What's the %00 here? :) @@ +89,3 @@ + * represented with "A","B","C","D","E", "F" and "G" and halfkeys + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). + * Off key is represented with an "o" only. What about non-major/minor keys? Keys that are not on any of the half keys but between? Thinking about non-western music here. @@ +90,3 @@ + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). + * Off key is represented with an "o" only. + */ Add Since: 1.2 marker her
Review of attachment 251722 [details] [review]: Looks good
Review of attachment 251723 [details] [review]: ::: gst/id3tag/id3tag.c @@ +1081,3 @@ + GST_TAG_PUBLISHER, add_text_tag, "TPUB"}, { + GST_TAG_INTERPRETED_BY, add_text_tag, "TPE4"}, { + GST_TAG_MUSICAL_KEY, add_text_tag, "TKEY"}, { Just curious but do we handle all tags here now that are also handled by libgsttag/id3?
(In reply to comment #11) > @@ +89,3 @@ > + * represented with "A","B","C","D","E", "F" and "G" and halfkeys > + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). > + * Off key is represented with an "o" only. > > What about non-major/minor keys? Keys that are not on any of the half keys but > between? Thinking about non-western music here. Or let's say all music that is not modern, popular European/western music :) In Europe there non-major/minor scales were rather popular in the past too.
(In reply to comment #14) > (In reply to comment #11) > > > @@ +89,3 @@ > > + * represented with "A","B","C","D","E", "F" and "G" and halfkeys > > + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). > > + * Off key is represented with an "o" only. > > > > What about non-major/minor keys? Keys that are not on any of the half keys but > > between? Thinking about non-western music here. > > Or let's say all music that is not modern, popular European/western music :) In > Europe there non-major/minor scales were rather popular in the past too. I used the description from the id3v2 documentation (might not be the best one) and I don't know if this notation is supposed to handle non major-minor keys. I only encountered this tag and used it on modern european/western music.
Created attachment 251806 [details] [review] taglist: handle publisher and interpreted-by tags
(In reply to comment #11) > Review of attachment 251721 [details] [review]: > > ::: gst-libs/gst/tag/tag.h > @@ +85,3 @@ > + * GST_TAG_MUSICAL_KEY: > + * > + * Musical key in which the sound starts. It is represented as a string > > Should it be the key in which the song (sound?) starts or the main key of the > song? > > @@ +88,3 @@ > + * with a maximum length of three characters. The ground keys are > + * represented with "A","B","C","D","E", "F" and "G" and halfkeys > + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). > > What's the %00 here? :) Null byte as specified in the id3 documentation, I guess i should not mention it. > > @@ +89,3 @@ > + * represented with "A","B","C","D","E", "F" and "G" and halfkeys > + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). > + * Off key is represented with an "o" only. > > What about non-major/minor keys? Keys that are not on any of the half keys but > between? Thinking about non-western music here. > > @@ +90,3 @@ > + * represented with "b" and "#". Minor is represented as "m" (e.g. "Dbm" $00). > + * Off key is represented with an "o" only. > + */ > > Add Since: 1.2 marker her
Created attachment 251809 [details] [review] tag: add musical-key tag
Comment on attachment 251809 [details] [review] tag: add musical-key tag Maybe add that this might be extended in the future to also handle non-major/minor keys And is it really the key in which the song starts or the main key of the song? These can be different.
The documentation specify it is the key at which the sound starts however in pratice it can be the main key of the whole song (I personnaly use it as the main key of the song since it helps harmonic mixing of two songs). I guess some comparaison should be made on different samples to figure how people use this tag. Unfortunately I have no "harmonic ears" so it could take some time.
Ok, so let's keep it as is but please add a documentation comment about extending this in the future to non-major/minor keys :)
Created attachment 252405 [details] [review] 251809: tag: add musical-key tag
commit d10dab0884ef07264e47ca57392f81c686051221 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Wed Aug 14 16:18:59 2013 +0100 taglist: handle publisher and interpreted-by tags https://bugzilla.gnome.org/show_bug.cgi?id=705999 commit 45edbacd239b7a89a27966eb5d93b03562cea8b5 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Wed Aug 14 16:20:45 2013 +0100 tag: id3: handle publisher, interpreted-by and musical-key tags https://bugzilla.gnome.org/show_bug.cgi?id=705999 commit 541c061ee81c07b1f36869e897cb2cbe4b34effc Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Aug 15 11:03:47 2013 +0100 tag: add musical-key tag https://bugzilla.gnome.org/show_bug.cgi?id=705999 commit f0eda4b54c4d9d11b3826c921f11251e30633474 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Aug 15 11:45:34 2013 +0100 id3mux: handle publisher, interpreted-by and musical-key tags https://bugzilla.gnome.org/show_bug.cgi?id=705999