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

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

::: gst/mpegtsdemux/tsdemux.c
@@ +1483,3 @@
+        guint desc_min_length = 24;
+
+        if (desc->length < desc_min_length) {

Why not a constant or integer literal?

@@ +2686,3 @@
+  int remaining = 0;
+
+  if (stream->current_size < header_size) {

Why not a constant or integer literal?

::: gst/mpegtsmux/mpegtsmux.c
@@ +755,3 @@
+    const GValue *vInterlace = gst_structure_get_value (s, "interlace-mode");
+    const GValue *vColorimetry = gst_structure_get_value (s, "colorimetry");
+    j2k_private_data *private_data = g_malloc (sizeof (j2k_private_data));

You seem to be leaking the private_data in error cases. Also better use
g_new0(j2k_private_data, 1) here.

@@ +757,3 @@
+    j2k_private_data *private_data = g_malloc (sizeof (j2k_private_data));
+    if (!private_data) {
+      GST_ERROR_OBJECT (pad, "Unable to allocate memory");

g_malloc(), g_new(), etc never return NULL. If allocation fails, they abort()
the process.

@@ +809,3 @@
+    private_data->interlace = FALSE;    /* will be set later. TODO: interlace
video mode doesn't work yet */
+    private_data->den = 0;      /* will be set later */
+    private_data->num = 0;      /* will be set later */

Is a fixed framerate required, or can it be 0/1 or 0/0?

@@ +812,3 @@
+    private_data->max_bitrate = max_rate;
+    private_data->Fic = 1;      /* TODO: get from caps */
+    private_data->Fio = 0;      /* TODO: get from caps */

What is the meaning of these?

@@ +825,3 @@
+    if (vInterlace) {
+      const char *interlace_mode = g_value_get_string (vInterlace);
+      private_data->interlace = g_str_equal (interlace_mode, "interleaved");

There's progressive, interleaved and mixed. None of these are "one field per
buffer", all of them are "one frame / two fields per buffer". So I think as of
now you can just handle interlaced like progressive. You will get both fields
encoded into the same frame, interleaved.

The special interlaced mode here is, AFAIU, for "one field per buffer"?

@@ +842,3 @@
+          color_spec = 3;
+        } else {
+          color_spec = 1;       /* RGB as default */

What is it that is these values mean? Color range, primaries, matrix, gamma
transfer function, all at once? You might want to use
gst_video_colorimetry_from_string() and then look specifically at the struct
returned

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

While these are only stored as guint8, you would overflow the seconds here
needlessly (they are % 60 later anyway), which would give wrong values for the
minutes. And same thing with minutes/hours. Best to store these in a guint64
during calculations, you make sure they're in the right range at the very end
anyway but during the calculations it would be good to not go completely off
because of integer overflows.

@@ +62,3 @@
+
+  /* ??? Hack for missing frame number index in buffer offset */
+  /* guint8 frame_number = private_data->frame_number % 60; */

You could just count frames, or not? Starting at 0, all frames that are muxed
here. AFAIU the hours/minutes/seconds/frames are a timecode here though, so
maybe better to create actual timecodes here instead of calculating something
from the PTS. You can use the GstVideoTimeCode API for that: Either you get
buffers with timecodes as input (as a meta on the buffers), or you just start
counting at 0 and then increase with every buffer (or you leave it all 0 if
there are no timecodes added upstream? is that valid?). Then you'll get valid
values for hours/minutes/seconds/frames, which you might not by just taking the
PTS.

@@ +70,3 @@
+  gst_byte_writer_put_int32_be (&wr, 0x656c736d);
+  /* Framerate box 'frat' == 0x66726174 */
+  gst_byte_writer_put_int32_be (&wr, 0x66726174);

Could also do _put_data(&wr, "frat", 4) if wanted, but doesn't really matter

@@ +84,3 @@
+  if (private_data->interlace) {
+    /* put size of second codestream */
+    gst_byte_writer_put_int32_be (&wr, gst_buffer_get_size (buf));

This seems wrong for interlacing, maybe put a FIXME/TODO comment somewhere
explaining what is missing for interlacing

@@ +122,3 @@
+  gst_buffer_map (buf, &buf_map, GST_MAP_READ);
+  gst_buffer_fill (out_buf, header_size, buf_map.data, buf_map.size);
+  gst_buffer_unmap (buf, &buf_map);

Could also use gst_buffer_copy_into() here (or just combine it with the
gst_buffer_copy_into() above). Set offset to header_size for that, and size to
-1.

::: gst/mpegtsmux/tsmux/tsmuxstream.h
@@ -248,1 @@
-guint64     tsmux_stream_get_pts         (TsMuxStream *stream);

Please don't run gst-indent over headers, only C/C++ source files

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