[Bug 704881] Partial ATSC_user_data and CEA-708 Closed Captions Support

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=704881
  GStreamer | gst-plugins-bad | git master

Thiago Sousa Santos <thiagossantos> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #295145|none                        |needs-work
             status|                            |

--- Comment #63 from Thiago Sousa Santos <thiagossantos at gmail.com> 2015-01-29 13:47:58 UTC ---
Review of attachment 295145:
 --> (https://bugzilla.gnome.org/review?bug=704881&attachment=295145)

::: gst-libs/gst/codecparsers/gstmpegvideoparser.c
@@ +1113,3 @@
+ * Returns: %TRUE if the cc could be parsed correctly, %FALSE otherwize.
+ *
+ * Since: 1.5

Please use 1.6 here as it is the next stable release. 1.5 is going to be the
unstable release that will lead to 1.6

@@ +1119,3 @@
+    GstMpegVideoClosedCaption * cc)
+{
+  GstBitReader br;

It seems that you are reading data always aligned to byte boundary so I'd
recommend using GstByteReader that is more suitable for the task. Check the API
here:
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer-libs/html/gstreamer-libs-GstByteReader.html

@@ +1133,3 @@
+  memset (cc, 0, sizeof (GstMpegVideoClosedCaption));
+  gst_bit_reader_peek_bits_uint64 (&br, &temp64, 64);
+  ud_ID = (guint32) ((temp64 >> 32) & 0xFFFFFFFF);

Instead of reading 64 bits at once, it would make more sense to use:
ud_ID = gst_byte_reader_get_uint32_be_unchecked();

Reasons:
1) You only need 32 bits here
2) it will already skip the 32 bits and be ready to read from the next position
3) you already checked that the size is >= 8 above, so you can read the 32 bits
with the _unchecked variant to skip the size check.

@@ +1137,3 @@
+  // "GA94" or "DTG1" are the allowed UD identifiers for ATSC A/53 part 4
+  if (0x47413934 == ud_ID) {
+    user_data_type_code = (gint8) ((temp64 & 0xFF000000) >> 24);

here you can read another byte from the stream instead of doing these bitwise
operations. It makes the code more readable.

::: gst-libs/gst/codecparsers/gstmpegvideoparser.h
@@ +481,3 @@
+ * TODO:  atsc_reserved_user_data bit string following em_data
+ *
+ * Since: 1.5

Since: 1.6

@@ +502,3 @@
+ * The x-closedcaption type buffer carried data
+ *
+ * Since: 1.5

Since: 1.6

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.