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 727403 - mpegts: add linkage, data_broadcast_id, private_data_specifier and parental_rating descriptors
mpegts: add linkage, data_broadcast_id, private_data_specifier and parental_r...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-31 18:19 UTC by Stefan Ringel
Modified: 2014-04-16 07:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add linkage descriptor (9.81 KB, patch)
2014-03-31 18:19 UTC, Stefan Ringel
needs-work Details | Review
add parental rating descriptor (3.88 KB, patch)
2014-03-31 18:19 UTC, Stefan Ringel
needs-work Details | Review
add private data specifier descriptor (3.15 KB, patch)
2014-03-31 18:20 UTC, Stefan Ringel
needs-work Details | Review
add data broadcast id descriptor (3.29 KB, patch)
2014-03-31 18:20 UTC, Stefan Ringel
needs-work Details | Review
add parental rating descriptor (4.47 KB, patch)
2014-04-01 13:58 UTC, Stefan Ringel
needs-work Details | Review
add data broadcast id descriptor (3.35 KB, patch)
2014-04-01 13:58 UTC, Stefan Ringel
none Details | Review
add private data specifier descriptor (3.15 KB, patch)
2014-04-01 13:59 UTC, Stefan Ringel
none Details | Review
add private data specifier descriptor (3.42 KB, patch)
2014-04-03 18:18 UTC, Stefan Ringel
accepted-commit_now Details | Review
add data broadcast id descriptor (3.35 KB, patch)
2014-04-03 18:18 UTC, Stefan Ringel
accepted-commit_now Details | Review
add linkage descriptor (10.68 KB, patch)
2014-04-09 08:12 UTC, Stefan Ringel
needs-work Details | Review
add parental rating descriptor (4.48 KB, patch)
2014-04-09 08:13 UTC, Stefan Ringel
needs-work Details | Review
add private data specifier descriptor (3.42 KB, patch)
2014-04-09 08:13 UTC, Stefan Ringel
accepted-commit_now Details | Review
add data broadcast id descriptor (3.36 KB, patch)
2014-04-09 08:14 UTC, Stefan Ringel
accepted-commit_now Details | Review
add linkage descriptor (11.00 KB, patch)
2014-04-10 14:07 UTC, Stefan Ringel
committed Details | Review
add parental rating descriptor (4.48 KB, patch)
2014-04-10 14:07 UTC, Stefan Ringel
committed Details | Review
add private data specifier descriptor (3.44 KB, patch)
2014-04-10 14:08 UTC, Stefan Ringel
committed Details | Review
add data broadcast id descriptor (3.36 KB, patch)
2014-04-10 14:08 UTC, Stefan Ringel
committed Details | Review

Description Stefan Ringel 2014-03-31 18:19:32 UTC
Created attachment 273353 [details] [review]
add linkage descriptor

mpegts: add linkage, data_broadcast_id, private_data_specifier and parental_rating descriptors
Comment 1 Stefan Ringel 2014-03-31 18:19:59 UTC
Created attachment 273354 [details] [review]
add parental rating descriptor
Comment 2 Stefan Ringel 2014-03-31 18:20:28 UTC
Created attachment 273355 [details] [review]
add private data specifier descriptor
Comment 3 Stefan Ringel 2014-03-31 18:20:53 UTC
Created attachment 273356 [details] [review]
add data broadcast id descriptor
Comment 4 Edward Hervey 2014-04-01 05:24:18 UTC
Review of attachment 273354 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.h
@@ +510,3 @@
+ * @country_code: This 24-bit field identifies a country using the 3-character
+ * code as specified in ISO 3166
+ * @rating:

Needs description. And either provide a pre-calculated minimum-age (for rating values between 0x01 and 0x0f) or enough description to make that field meaningful.
Comment 5 Edward Hervey 2014-04-01 05:26:28 UTC
Review of attachment 273355 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +1170,3 @@
+ * @descriptor: a %GST_MTS_DESC_DVB_PRIVATE_DATA_SPECIFIER #GstMpegTsDescriptor
+ * @private_data_specifier: (out): the private data specifier id
+ * registered by http://www.dvbservices.com/

A private data specifier descriptor can also contain extra bytes (after the 32bit id).

There should be extra (guint8**, gsize *) arguments to return that.
Comment 6 Edward Hervey 2014-04-01 05:27:09 UTC
Review of attachment 273356 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +1241,3 @@
+ * @descriptor: a %GST_MTS_DESC_DVB_DATA_BROADCAST_ID #GstMpegTsDescriptor
+ * @data_broadcast_id: (out): the data broadcast id
+ * @id_selector_bytes: (out): the selector bytes, if present

If you return bytes, you also need an output argument specifying the size of id_selector_bytes
Comment 7 Stefan Ringel 2014-04-01 13:12:39 UTC
(In reply to comment #4)
> Review of attachment 273354 [details] [review]:
> 
> ::: gst-libs/gst/mpegts/gst-dvb-descriptor.h
> @@ +510,3 @@
> + * @country_code: This 24-bit field identifies a country using the 3-character
> + * code as specified in ISO 3166
> + * @rating:
> 
> Needs description. And either provide a pre-calculated minimum-age (for rating
> values between 0x01 and 0x0f) or enough description to make that field
> meaningful.

0x00 to 0x0f : age = rating + 3 (without Brasil)
0x10 to 0xff : broadcaster defined (Brasil use hi-nibble as content and lo-nibble as age).

Is that o.k. as description?
Comment 8 Stefan Ringel 2014-04-01 13:13:33 UTC
(In reply to comment #6)
> Review of attachment 273356 [details] [review]:
> 
> ::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
> @@ +1241,3 @@
> + * @descriptor: a %GST_MTS_DESC_DVB_DATA_BROADCAST_ID #GstMpegTsDescriptor
> + * @data_broadcast_id: (out): the data broadcast id
> + * @id_selector_bytes: (out): the selector bytes, if present
> 
> If you return bytes, you also need an output argument specifying the size of
> id_selector_bytes

yes, you've right.
Comment 9 Edward Hervey 2014-04-01 13:15:40 UTC
(In reply to comment #7)
> 0x00 to 0x0f : age = rating + 3 (without Brasil)
> 0x10 to 0xff : broadcaster defined (Brasil use hi-nibble as content and
> lo-nibble as age).
> 
> Is that o.k. as description?

Provided you also say it's the age rating, that would be a minimum.

We could also have an additional pre-processed variant (which returns the actual age or 0 if undefined).
Comment 10 Stefan Ringel 2014-04-01 13:19:24 UTC
(In reply to comment #5)
> Review of attachment 273355 [details] [review]:
> 
> ::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
> @@ +1170,3 @@
> + * @descriptor: a %GST_MTS_DESC_DVB_PRIVATE_DATA_SPECIFIER
> #GstMpegTsDescriptor
> + * @private_data_specifier: (out): the private data specifier id
> + * registered by http://www.dvbservices.com/
> 
> A private data specifier descriptor can also contain extra bytes (after the
> 32bit id).
> 
> There should be extra (guint8**, gsize *) arguments to return that.

That is not correct. Look the specification EN 300 468 v1.13.1 section 6.2.31. :

6.2.31 Private data specifier descriptor
This descriptor (see table 83) is used to identify the specifier of any private descriptors or private fields within descriptors.
Table 83: Private data specifier descriptor

       Syntax            Number of bits     Identifier

private_data_specifier_descriptor(){
       descriptor_tag          8            uimsbf
       descriptor_length       8            uimsbf
       private_data_specifier  32           uimsbf
}

Semantics for the private data specifier descriptor:

private_data_specifier: The assignment of values for this field is given in TS 101 162 [i.1].
Comment 11 Stefan Ringel 2014-04-01 13:36:58 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > 0x00 to 0x0f : age = rating + 3 (without Brasil)
> > 0x10 to 0xff : broadcaster defined (Brasil use hi-nibble as content and
> > lo-nibble as age).
> > 
> > Is that o.k. as description?
> 
> Provided you also say it's the age rating, that would be a minimum.
> 
> We could also have an additional pre-processed variant (which returns the
> actual age or 0 if undefined).

o.k. we decoding the lo-byte as age (rating + 3). More decoding, if only we know the different.

Brasil (lo and hi nibble)

0000  Reserved
0001  L
0010  10
0011  12
0100  14
0101  16
0110  18
0111 - 1111 Reserved
Comment 12 Stefan Ringel 2014-04-01 13:58:00 UTC
Created attachment 273405 [details] [review]
add parental rating descriptor
Comment 13 Stefan Ringel 2014-04-01 13:58:35 UTC
Created attachment 273406 [details] [review]
add data broadcast id descriptor
Comment 14 Stefan Ringel 2014-04-01 13:59:13 UTC
Created attachment 273407 [details] [review]
add private data specifier descriptor
Comment 15 Edward Hervey 2014-04-03 05:50:36 UTC
(In reply to comment #10)
> > 
> > There should be extra (guint8**, gsize *) arguments to return that.
> 
> That is not correct. Look the specification EN 300 468 v1.13.1 section 6.2.31.
> :
> 

Sure... but... from real-world dumps I can *guarantee* you that some streams have extra bytes after that :)
Comment 16 Stefan Ringel 2014-04-03 12:46:47 UTC
(In reply to comment #15)
> (In reply to comment #10)
> > > 
> > > There should be extra (guint8**, gsize *) arguments to return that.
> > 
> > That is not correct. Look the specification EN 300 468 v1.13.1 section 6.2.31.
> > :
> > 
> 
> Sure... but... from real-world dumps I can *guarantee* you that some streams
> have extra bytes after that :)

Have it the same descriptor tag? Give me an example of section with this descriptor. So I can decode myself.
Comment 17 Stefan Ringel 2014-04-03 18:18:01 UTC
Created attachment 273541 [details] [review]
add private data specifier descriptor
Comment 18 Stefan Ringel 2014-04-03 18:18:35 UTC
Created attachment 273542 [details] [review]
add data broadcast id descriptor
Comment 19 Sebastian Dröge (slomo) 2014-04-08 09:32:51 UTC
Review of attachment 273353 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +466,3 @@
+  res->linkage_type = *data;
+  data += 1;
+

Inside all the ifs below you don't check if you have enough data available for reading.
Comment 20 Sebastian Dröge (slomo) 2014-04-08 09:33:57 UTC
Review of attachment 273405 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +976,3 @@
+        g_slice_new0 (GstMpegTsDVBParentalRatingItem);
+    g_ptr_array_add (*rating, item);
+

You need to check here in each iteration if you have enough data available.
Comment 21 Stefan Ringel 2014-04-09 08:12:39 UTC
Created attachment 273858 [details] [review]
add linkage descriptor
Comment 22 Stefan Ringel 2014-04-09 08:13:17 UTC
Created attachment 273859 [details] [review]
add parental rating descriptor
Comment 23 Stefan Ringel 2014-04-09 08:13:44 UTC
Created attachment 273860 [details] [review]
add private data specifier descriptor
Comment 24 Stefan Ringel 2014-04-09 08:14:07 UTC
Created attachment 273861 [details] [review]
add data broadcast id descriptor
Comment 25 Sebastian Dröge (slomo) 2014-04-10 07:12:05 UTC
Review of attachment 273858 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +470,3 @@
+      GstMpegTsDVBLinkageMobileHandOver *hand_over;
+
+      g_return_val_if_fail (end - data < 1, FALSE);

Don't use g_return_val_if_fail() here. The g_return*_if_fail() assertions are meant to catch programming errors, not invalid/corrupted data... and can be compiled out like normal assertions too.
Comment 26 Sebastian Dröge (slomo) 2014-04-10 07:12:46 UTC
Review of attachment 273859 [details] [review]:

::: gst-libs/gst/mpegts/gst-dvb-descriptor.c
@@ +1014,3 @@
+
+    if (g_strcmp0 (item->country_code, "BRA") == 0) {
+      // brasil

No C99 comments

@@ +1039,3 @@
+      }
+    } else
+      item->rating = (*data & 0xf) + 3;

Put some {} brackets around this
Comment 27 Stefan Ringel 2014-04-10 07:34:10 UTC
Review of attachment 273858 [details] [review]:

Is it true to return false if invalid/corrupted data? So I can use: if (end - data < 2) return FALSE;
Comment 28 Sebastian Dröge (slomo) 2014-04-10 07:37:21 UTC
Sure, just make sure you don't leak memory then (the ptr array).
Comment 29 Stefan Ringel 2014-04-10 14:07:01 UTC
Created attachment 273991 [details] [review]
add linkage descriptor
Comment 30 Stefan Ringel 2014-04-10 14:07:47 UTC
Created attachment 273992 [details] [review]
add parental rating descriptor
Comment 31 Stefan Ringel 2014-04-10 14:08:09 UTC
Created attachment 273993 [details] [review]
add private data specifier descriptor
Comment 32 Stefan Ringel 2014-04-10 14:08:36 UTC
Created attachment 273994 [details] [review]
add data broadcast id descriptor
Comment 33 Sebastian Dröge (slomo) 2014-04-16 07:22:05 UTC
commit ace60abef5e5a90b216e30f67142a957fa964b5e
Author: Stefan Ringel <linuxtv@stefanringel.de>
Date:   Thu Apr 10 16:00:50 2014 +0200

    mpegts: add data broadcast id descriptor
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727403

commit 065abf6d542a7dcf86678a8aa6975e78562a64bd
Author: Stefan Ringel <linuxtv@stefanringel.de>
Date:   Thu Apr 10 15:59:50 2014 +0200

    mpegts: add private data specifier descriptor
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727403

commit 8407ee9ee91c5d8308a9efe9c40f58cbf36f54ec
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Apr 16 09:20:37 2014 +0200

    dvb: Minor code style fix

commit e71c264dbb30b165537cf6e03f48239069756cda
Author: Stefan Ringel <linuxtv@stefanringel.de>
Date:   Thu Apr 10 15:58:55 2014 +0200

    mpegts: add parential rating descriptor
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727403

commit eb246e2bfd15b83d7a261aefa2b79e99731bbf4c
Author: Stefan Ringel <linuxtv@stefanringel.de>
Date:   Thu Apr 10 15:58:04 2014 +0200

    mpegts: add linkage descriptor
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727403