GNOME Bugzilla – Bug 727403
mpegts: add linkage, data_broadcast_id, private_data_specifier and parental_rating descriptors
Last modified: 2014-04-16 07:22:16 UTC
Created attachment 273353 [details] [review] add linkage descriptor mpegts: add linkage, data_broadcast_id, private_data_specifier and parental_rating descriptors
Created attachment 273354 [details] [review] add parental rating descriptor
Created attachment 273355 [details] [review] add private data specifier descriptor
Created attachment 273356 [details] [review] add data broadcast id descriptor
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.
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.
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
(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?
(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.
(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).
(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].
(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
Created attachment 273405 [details] [review] add parental rating descriptor
Created attachment 273406 [details] [review] add data broadcast id descriptor
Created attachment 273407 [details] [review] add private data specifier descriptor
(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 :)
(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.
Created attachment 273541 [details] [review] add private data specifier descriptor
Created attachment 273542 [details] [review] add data broadcast id descriptor
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.
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.
Created attachment 273858 [details] [review] add linkage descriptor
Created attachment 273859 [details] [review] add parental rating descriptor
Created attachment 273860 [details] [review] add private data specifier descriptor
Created attachment 273861 [details] [review] add data broadcast id descriptor
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.
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
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;
Sure, just make sure you don't leak memory then (the ptr array).
Created attachment 273991 [details] [review] add linkage descriptor
Created attachment 273992 [details] [review] add parental rating descriptor
Created attachment 273993 [details] [review] add private data specifier descriptor
Created attachment 273994 [details] [review] add data broadcast id descriptor
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