[Bug 766236] rtp j2k payload/depayload messes up colours in sample pattern

[GStreamer (GNOME Bugzilla)]

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

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

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

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

Some more minor things :)

::: gst/videoparsers/gstjpeg2000parse.c
@@ +192,3 @@
+  if (x1 < x0 || y1 < y0) {
+    GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE,
+        ("Nonsensical image dimensions %d,%d,%d,%d", x0, y0, x1, y1), NULL);

Swap the (NULL) and the error message. This is more something to be shown to a
developer (-> debug) than something for the user

@@ +214,3 @@
+    GST_ERROR_OBJECT (jpeg2000parse, "Unsupported number of components %d",
+        numcomps);
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here, and maybe should be an
GST_ELEMENT_ERROR()?

@@ +220,3 @@
+  s = gst_caps_get_structure (current_caps, 0);
+
+  if (!current_caps) {

The gst_caps_get_structure() above should be below the if, otherwise it will
crash/assert

@@ +235,3 @@
+  gst_caps_unref (current_caps);
+
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

This should probably be compno < numcomps?

@@ +261,3 @@
+  jpeg2000parse->height = height;
+
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

And here compno < numcomps

@@ +272,3 @@
+
+  /* we do not set sampling field for sub-sampled RGB or monochrome */
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

And here

@@ +352,3 @@
+          gst_caps_new_simple (gst_structure_get_name (s), "width",
G_TYPE_INT,
+          jpeg2000parse->width, "height", G_TYPE_INT, jpeg2000parse->height,
+          "colorspace", G_TYPE_STRING, colorspace, NULL);

To reduce code duplication you could first create the caps without sampling,
and then do a gst_caps_set_simple() just for that

@@ +356,3 @@
+
+    sink_caps =
+        gst_pad_get_current_caps (GST_BASE_PARSE_SINK_PAD (jpeg2000parse));

You already got these (and unreffed them again) as current_caps above

@@ +396,3 @@
+
+    ret = gst_base_parse_finish_frame (parse, frame, frame_size);
+    return ret;

You forget to unmap the buffer here

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