GNOME Bugzilla – Bug 730926
tags: add GST_TAG_PRIVATE_DATA and expose ID3 private frame ("PRIV") data
Last modified: 2015-11-20 20:31:20 UTC
While parsing the metadata of the media, "private frames" are not handled. So, private frame tag and data are added to unknown/unhandled "ID3v2 frame". And from application it is not possible to retrieve "priv" tag data. 4.27. Private frame This frame is used to contain information from a software producer that its program uses and does not fit into the other frames. The frame consists of an 'Owner identifier' string and the binary data. The 'Owner identifier' is a null-terminated string with a URL [URL] containing an email address, or a link to a location where an email address can be found, that belongs to the organisation responsible for the frame. Questions regarding the frame should be sent to the indicated email address. The tag may contain more than one "PRIV" frame but only with different contents. <Header for 'Private frame', ID: "PRIV"> Owner identifier <text string> $00 The private data <binary data>
Created attachment 277441 [details] [review] Addition of tag "GST_TAG_PRIVATE_DATA" for handling PRIV frames Patch for proposed changes "gstreamer" project. Please review.
Created attachment 277442 [details] [review] Handling "PRIV" frames Patch for proposed changes in "gst-plugins-base" project. Metadata added as GstBuffer. Tested with app http://gstreamer.freedesktop.org/data/doc/gstreamer/head/manual/html/chapter-metadata.html#section-tags-read
Looks good. Could you rebase this with master please? It doesn't apply right now. Will test and merge once it does.
Comment on attachment 277441 [details] [review] Addition of tag "GST_TAG_PRIVATE_DATA" for handling PRIV frames This is missing a 'Since' marker. How is the "owner" string expressed here if there are multiple such frames? gst_tag_merge_strings_with_comma() makes no sense here.
Thanks for the review. Should the 'Since' marker be 1.4 or 1.5? Without gst_tag_merge_strings_with_comma, only 1 PRIV frame data will be collected. Since there could be more than 1 PRIV frames, each with 'owner identifier' text, I used gst_tag_merge_strings_with_comma. Please let me know. And, I will submit new patch after re-basing to latest.
The Since marker should be 1.6 (next stable release this will be in). I think you misunderstood my comment about gst_tag_merge_strings_with_comma(). The key point here is that the type of the tag is not a string, so if I'm not confused then gst_tag_merge_strings_with_comma() makes no sense and won't work. You have also not addressed my question about where "owner" is expressed.
Ok, I got what you meant about gst_tag_merge_strings_with_comma. May be 'gst_tag_merge_use_first' can be used? I read the PRIV frame data into buffer. So, when app reads the buffer, first x bytes contains the null-terminated string (owner string) followed by binary data. Is it ok or should the tag be changed to STRING and have only owner string? Or do you suggest any other method?
Yes, gst_tag_merge_use_first() is right. The owner string should not be at the beginning of the buffer data. You call this GST_TAG_PRIVATE_DATA, which is quite generic, so it should not contain ID3-specific formatting. We may want to use it for other private data too (I'm actually wondering if we don't have something for that already, I have a vague memory we extracted similar things from matroska). The buffer should just contain the actual private data, nothing else. The tag type should probably be changed to a GstSample. Then the data can go into the GstBuffer, and any extra information like the origin (id3 tag) and owner can go into the GstStructure.
Created attachment 286854 [details] [review] add tag for private data
Created attachment 286855 [details] [review] Handle priv frame id3 tag Thanks for the input. New patches attached. Please review.
Created attachment 286862 [details] [review] Handle priv frame id3 tag Small correction to previous patch
Comment on attachment 286854 [details] [review] add tag for private data Thanks, this is almost ready now, but I think there should be a note in the docs for the tag that indicates the use of the info structure in the sample to distinguish different sources for this tag, and what the structure name would be for a PRIV frame from an ID3 tag (e.g. "ID3PrivateFrame").
Comment on attachment 286862 [details] [review] Handle priv frame id3 tag >+ owner_len = strlen ((gchar *) work->parse_data) + 1; Running strlen() on untrusted data is not a good idea, since we can't specify a max_length to search. What if there is no 0 in the data? I think you should use memchr() here, and then also check that there's actually any payload data in addition to the owner string. >+ owner_data = >+ gst_structure_new (work->frame_id, "owner", G_TYPE_STRING, >+ work->parse_data, NULL); Ok, but I think the structure name should be more expressive than just "PRIV", I would suggest "ID3PrivateFrame". And I would call this owner_info instead of owner_data, since "info" is the name/terminology used by gst_sample_new(). Rest looks fine, thanks for the update.
Created attachment 287325 [details] [review] add tag for private data
Created attachment 287326 [details] [review] Handle priv frame id3 tag Thanks. I included your suggestions. Please see if it ok.
Ping..
Thanks, pushed with a small change in the parse function (don't do "if (owner) len=... else return" but "if (!owner) return; len= ..."). Sorry it took so long. Also added a unit test. If you feel also like making id3mux in -bad write the PRIVATE_DATA tag, that'd be great! commit 9fbecd6b6d37af9883ca58a91360a5db6a9f0102 Author: Ravi Kiran K N <ravi.kiran@samsung.com> Date: Mon Sep 29 14:03:13 2014 +0530 tags: add GST_TAG_PRIVATE_DATA Can be used to represent private data that may be contained in tags, such as ID3v2 PRIV frames. https://bugzilla.gnome.org/show_bug.cgi?id=730926 commit df5725e68331ca50e9d610c5d99d23fbf7c046a0 Author: Ravi Kiran K N <ravi.kiran@samsung.com> Date: Mon Sep 29 14:17:39 2014 +0530 id3v2frames: Handle private frames Handle PRIV ID3 tag having owner information (string) and binary data, add to tag messages list. https://bugzilla.gnome.org/show_bug.cgi?id=730926