[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 #353330|none                        |needs-work
             status|                            |

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

::: gst/mpegtsdemux/tsdemux.c
@@ +1494,3 @@
+        DEN_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br);
+        NUM_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br);
+        color_specification = gst_byte_reader_get_uint8_unchecked (&br);

You should check that you actually have that much data available instead of
blindly reading

@@ +1501,3 @@
+        (void) vertical_size;
+        (void) max_bit_rate;
+        (void) max_buffer_size;

G_GNUC_UNUSED seems better for that

@@ +2664,3 @@
+  GstBufferList *buffer_list = NULL;
+
+

Double empty line

@@ +2821,3 @@
+
+  buffer = gst_buffer_new_wrapped (packet_data, packet_size);
+  gst_buffer_list_add (buffer_list, buffer);

Why a buffer list? Why could there be multiple buffers here (and would each
buffer be a complete JPC frame?)?

::: gst/mpegtsmux/mpegtsmux.c
@@ +758,3 @@
+    j2k_private_data *map_data_p = NULL;
+    GstMemory *private_data =
+        gst_allocator_alloc (NULL, sizeof (j2k_private_data), NULL);

Why a GstMemory here instead of just using g_malloc()? You're not sending the
memory downstream in a buffer or similar

@@ +764,3 @@
+      GstAllocator *default_allocator =
+          gst_allocator_find (GST_ALLOCATOR_SYSMEM);
+      gst_allocator_free (default_allocator, private_data);

gst_memory_unref()

::: gst/mpegtsmux/mpegtsmux_jpeg2000.c
@@ +108,3 @@
+  guint8 seconds = (guint8) (buf->pts / 1e9);
+  guint8 minutes = (guint8) (seconds / 60);
+  guint8 hours = (guint8) (minutes / 60);

Won't these overflow the 8 bits usually? And instead of 1e9 better use
GST_SECOND

@@ +137,3 @@
+  GST_DEBUG_OBJECT (mux, "Creating buffer of size: %lu", out_size);
+
+  wr = gst_byte_writer_new ();

Initialize on the stack, maybe also with some expected size

@@ +140,3 @@
+
+  /* Elementary stream header box 'elsm' == 0x656c736d */
+  if (gst_byte_writer_put_int32_be (wr, 0x656c736d)) {

These generally can't fail, and if they do you want to go out immediately.
Please reduce the level of indentation here and below :)

@@ +278,3 @@
+  /*  Free prepare data memory object */
+  GstAllocator *default_allocator = gst_allocator_find (GST_ALLOCATOR_SYSMEM);
+  gst_allocator_free (default_allocator, (GstMemory *) prepare_data);

gst_memory_unref()

::: gst/mpegtsmux/mpegtsmux_jpeg2000.h
@@ +3,3 @@
+ *  Authors: Jan Schmidt <jan at fluendo.com>
+ *           Kapil Agrawal <kapil at fluendo.com>
+ *           Julien Moutte <julien at fluendo.com>

I don't think any of these people contributed to this new file, or the
corresponding .c file :)

::: gst/mpegtsmux/tsmux/tsmuxstream.c
@@ +798,3 @@
+      guint8 level = stream->profile_and_level & 0xF;
+      guint32 max_buffer_size = 0;
+      GstByteWriter *writer = gst_byte_writer_new ();

Initialize this on the stack ideally

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