[Bug 753323] mpegts: add support for JPEG 2000 to mpegtsmux and tsdemux

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=753323

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #111 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 355293:
 --> (https://bugzilla.gnome.org/review?bug=753323&attachment=355293)

::: gst/mpegtsdemux/tsdemux.c
@@ +1522,3 @@
+
+
+        remaining_8b = gst_byte_reader_get_uint8_unchecked (&br);

Many empty new-lines, and you should probably check if there actually is
another uint8 to read here

@@ +1558,3 @@
+          default:
+            colorspace = "";
+            colorimetry_mode = "";

In this case, don't set the fields to empty strings but leave them empty or
don't create caps at all

@@ +1751,3 @@

+  if (name)
+    g_free (name);

g_free() is NULL-safe

@@ +2662,3 @@

+  if (stream->data) {
+    g_free (stream->data);

g_free() is NULL-safe

@@ +2672,3 @@
     GST_ERROR ("Failed to parse Opus access unit");
+    if (stream->data) {
+      g_free (stream->data);

g_free() is NULL-safe

@@ +2676,3 @@
+    }
+    stream->current_size = 0;
+    gst_buffer_list_unref (buffer_list);

gst_buffer_list_unref() isn't NULL-safe

@@ +2821,3 @@
+  }
+  /* Get reserved 8-bits */
+  if (!gst_byte_reader_get_uint8 (&reader, &b)) {

Instead of all the checks, you could also use the get_XXX_unchecked() variants
and in the beginning check once if you have enough data for everything

@@ +2859,1 @@
     g_free (stream->data);

g_free() is NULL-safe

::: gst/mpegtsmux/mpegtsmux.c
@@ +770,3 @@
+      }
+    } else {
+      GST_ERROR_OBJECT (pad, "Missing JPEG 2000 profile");

You probably want to put the profile into the caps of the sink pad template
then

@@ +808,3 @@
+    } else {
+      /*GST_ERROR_OBJECT (pad, "Missing main level");
+         goto not_negotiated; */

And the same for the main level

@@ +899,3 @@
+    /* Width and Height */
+    gst_structure_get_int (s, "width", &ts_data->stream->horizontal_size);
+    gst_structure_get_int (s, "height", &ts_data->stream->vertical_size);

Should check if those fields actually exist, or require them on the sink pad
template caps

::: gst/mpegtsmux/mpegtsmux_jpeg2000.c
@@ +136,3 @@
+  gst_byte_writer_put_uint8 (&wr, seconds);
+  gst_byte_writer_put_uint8 (&wr, 0x0);
+  /* private_data->frame_number++; */

Why commented out?

@@ +140,3 @@
+  /* private_data->HHMMSSFF */
+  /*
+     if(!gst_byte_writer_put_int32_be(&wr, 0x00))

What is this commented out block?

::: gst/videoparsers/gstjpeg2000parse.c
@@ +1,2 @@
 /* GStreamer JPEG 2000 Parser
+ * Copyright (C) <2016-2017> Grok Image Compession Inc.

Typo: CompRession

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.