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 730926 - tags: add GST_TAG_PRIVATE_DATA and expose ID3 private frame ("PRIV") data
tags: add GST_TAG_PRIVATE_DATA and expose ID3 private frame ("PRIV") data
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-29 09:51 UTC by RaviKiran
Modified: 2015-11-20 20:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Addition of tag "GST_TAG_PRIVATE_DATA" for handling PRIV frames (1.38 KB, patch)
2014-05-29 09:56 UTC, RaviKiran
needs-work Details | Review
Handling "PRIV" frames (2.38 KB, patch)
2014-05-29 09:58 UTC, RaviKiran
needs-work Details | Review
add tag for private data (1.35 KB, patch)
2014-09-23 08:23 UTC, RaviKiran
needs-work Details | Review
Handle priv frame id3 tag (2.88 KB, patch)
2014-09-23 08:24 UTC, RaviKiran
none Details | Review
Handle priv frame id3 tag (2.83 KB, patch)
2014-09-23 09:59 UTC, RaviKiran
needs-work Details | Review
add tag for private data (1.79 KB, patch)
2014-09-29 08:48 UTC, RaviKiran
committed Details | Review
Handle priv frame id3 tag (3.16 KB, patch)
2014-09-29 08:49 UTC, RaviKiran
committed Details | Review

Description RaviKiran 2014-05-29 09:51:13 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>
Comment 1 RaviKiran 2014-05-29 09:56:06 UTC
Created attachment 277441 [details] [review]
Addition of tag "GST_TAG_PRIVATE_DATA" for handling PRIV frames

Patch for proposed changes "gstreamer" project. Please review.
Comment 2 RaviKiran 2014-05-29 09:58:48 UTC
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
Comment 3 Luis de Bethencourt 2014-09-18 14:25:47 UTC
Looks good.

Could you rebase this with master please? It doesn't apply right now. Will test and merge once it does.
Comment 4 Tim-Philipp Müller 2014-09-18 18:46:29 UTC
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.
Comment 5 RaviKiran 2014-09-22 08:00:48 UTC
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.
Comment 6 Tim-Philipp Müller 2014-09-22 08:33:43 UTC
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.
Comment 7 RaviKiran 2014-09-22 09:58:10 UTC
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?
Comment 8 Tim-Philipp Müller 2014-09-22 10:19:30 UTC
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.
Comment 9 RaviKiran 2014-09-23 08:23:56 UTC
Created attachment 286854 [details] [review]
add tag for private data
Comment 10 RaviKiran 2014-09-23 08:24:53 UTC
Created attachment 286855 [details] [review]
Handle priv frame id3 tag

Thanks for the input. New patches attached. Please review.
Comment 11 RaviKiran 2014-09-23 09:59:41 UTC
Created attachment 286862 [details] [review]
Handle priv frame id3 tag

Small correction to previous patch
Comment 12 Tim-Philipp Müller 2014-09-25 21:59:52 UTC
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 13 Tim-Philipp Müller 2014-09-25 22:06:10 UTC
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.
Comment 14 RaviKiran 2014-09-29 08:48:34 UTC
Created attachment 287325 [details] [review]
add tag for private data
Comment 15 RaviKiran 2014-09-29 08:49:49 UTC
Created attachment 287326 [details] [review]
Handle priv frame id3 tag

Thanks. I included your suggestions. Please see if it ok.
Comment 16 RaviKiran 2015-02-10 04:55:21 UTC
Ping..
Comment 17 Tim-Philipp Müller 2015-11-20 20:30:45 UTC
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