GNOME Bugzilla – Bug 654388
[tags] API: move id3 parsing from id3demux to tag lib
Last modified: 2011-09-12 18:57:33 UTC
ID3 parsing code in id3demux is pretty independent already, and it turns out also needed elsewhere, e.g. in qtdemux (see e.g. http://www.mp4ra.org/specs.html). AFAICS, it could be moved practically verbatim, give or take some renaming away from id3demux mentioning, and maybe some other remaining todo stuff ?
+1 the idea. Is this something you plan to do?
That is the idea, yes, but AFAIK it takes some reviewing/commenting before stashing in new API. Will come up with suggested patch.
Ooh, nice, please do. That's been somewhere on my todo list as well. I think a minimal API like there is now inside id3demux is probably not too contentious. Might get trickier if we ever want to expose the individual frames though, but that's not needed at this point, is it?
Created attachment 191961 [details] [review] tag: id3: add id3v2 parsing Proposed patch performing plain-and-simple move (give or take some renaming and minor API adjustments to align somewhat with existing tag functions).
Created attachment 191963 [details] [review] id3demux: use -base provided id3 tag parsing And for reference, this would be the corresponding effect on id3demux. Maybe the remaining idv3 docs/specs in there should also be (re)moved ?
Comment on attachment 191961 [details] [review] tag: id3: add id3v2 parsing > gst-libs/gst/tag/Makefile.am: Do we need to add $(LIBZ) or whatever the right variable is to _LIBADD here? >gst-libs/gst/tag/tag.h: >+/** >+ * GST_TAG_ID3V2_FRAME: >+ * >+ * Stores unprocessed ID3v2 frames. (buffer) >+ * >+ * Since: 0.10.36 >+ */ >+#define GST_TAG_ID3V2_FRAME "private-id3v2-frame" Should we add a FIXME 0.11: so we don't forget to remove the 'private-' prefix in 0.11 now that it's public? >+GstTagList * gst_tag_list_from_id3v2_tag (GstBuffer * buffer); Wonder if (const guint8 * data, gsize data_len) would be better? (I think both are used for other functions in this header) >+guint gst_tag_calc_id3v2_tag_size (GstBuffer * buffer); Maybe make it _get_id3v2_tag_size()? It's not really a calculation, is it? Or maybe we don't need this function at all? (Anyone processing ID3v2 tags can be assumed to know about the header and check the header anyway, no?)
Oh also: we should make sure to move the complete git history of these files in this case (I can help with that if required). There are lots of little corner-cases that won't make sense without the history.
Created attachment 192199 [details] [review] tag: id3: add id3v2 parsing Updated patch.
(In reply to comment #6) > (From update of attachment 191961 [details] [review]) > > gst-libs/gst/tag/Makefile.am: > > Do we need to add $(LIBZ) or whatever the right variable is to _LIBADD here? > Done. > >gst-libs/gst/tag/tag.h: > >+/** > >+ * GST_TAG_ID3V2_FRAME: > >+ * > >+ * Stores unprocessed ID3v2 frames. (buffer) > >+ * > >+ * Since: 0.10.36 > >+ */ > >+#define GST_TAG_ID3V2_FRAME "private-id3v2-frame" > > Should we add a FIXME 0.11: so we don't forget to remove the 'private-' prefix > in 0.11 now that it's public? > Done. > >+GstTagList * gst_tag_list_from_id3v2_tag (GstBuffer * buffer); > > Wonder if (const guint8 * data, gsize data_len) would be better? (I think both > are used for other functions in this header) > Most seem to use a GstBuffer for the actual input data/blob though (and maybe guint8* for some auxiliary stuff), but easily changed otherwise. > >+guint gst_tag_calc_id3v2_tag_size (GstBuffer * buffer); > > Maybe make it _get_id3v2_tag_size()? It's not really a calculation, is it? Or > maybe we don't need this function at all? (Anyone processing ID3v2 tags can be > assumed to know about the header and check the header anyway, no?) Turned into a _get, as it might otherwise lead to duplication for the "anyone"s (or maybe make it an optional guint * tag_size output parameter for _from_id3v2_tag ?). Presumably the history tricks are described in release docs, though routine in doing so is another matter ... ;)
Created attachment 192203 [details] [review] id3demux: use -base provided id3 tag parsing Updated patch.
I'm happyen enough with that (new public API). > Presumably the history tricks are described in release docs, though routine in > doing so is another matter ... ;) Yes, the 'moving plugins' document in gstreamer/docs/random has some hints. Basically, create patches for the set of files in -good (without the ugly [MOVED FROM GOOD] prefix), then run sed over the *patch files to fix up the file paths from e.g. gst/id3demux/blah.c to e.g. gst-libs/gst/tag/gstblah.c (make sure to use /g or run it multiple times to catch multiple occurances per line). The run git am /path/to/*patch in -base to commit the series. Let me know if you'd prefer me to do this.
Ok, pushed with miniscule changes, incl. history. Kept the private tag private for now (don't see any reason to expose it as API). Not entirely sure about the GST_TAG_ID3V2_HEADER_SIZE define - the header size is 6 bytes instead of 10 bytes for the older (<2,3) ID3v2 tags afaik.
> Not entirely sure about the GST_TAG_ID3V2_HEADER_SIZE define - the header size > is 6 bytes instead of 10 bytes for the older (<2,3) ID3v2 tags afaik. Ok, that's just me being confused, the 6 bytes vs. 10 bytes for the different versions was the *frame* header size, the header of the ID3v2 tag itself is always 10 bytes.