[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 #329046|none                        |reviewed
             status|                            |

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

::: gst/videoparsers/gstjpeg2000parse.c
@@ +40,3 @@
+GST_STATIC_PAD_TEMPLATE ("src", GST_PAD_SRC,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("image/x-jpc,"

Ok, so as a next step let's consider adding the other caps :) As a patch on top
of this one, a separate one.

@@ +160,3 @@
+    return GST_FLOW_ERROR;
+  gst_byte_reader_init (&reader, map.data, map.size);
+

in general you probably want to add many GST_DEBUG_OBJECT() and similar lines
here so that debugging the code later is easier, so that you can know what goes
on inside the element by just enabling debug logs.

@@ +207,3 @@
+  }
+  jpeg2000parse->width = width;
+  jpeg2000parse->height = height;

You update the state here, but below could again "goto beach" to request more
data. The next time when you get more data, you would believe that the
dimensions did not actually change

@@ +220,3 @@
+  if (numcomps > GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS) {
+    /* TODO log error */
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +231,3 @@
+
+  /* colorspace is a required field */
+  colourspace = gst_structure_get_string (s, "colorspace");

Inconsistent language: colourspace and colorspace

@@ +233,3 @@
+  colourspace = gst_structure_get_string (s, "colorspace");
+  if (!colourspace)
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +251,3 @@
+
+    if (dx != jpeg2000parse->dx[compno] || dy != jpeg2000parse->dy[compno]) {
+      subsampling_changed = TRUE;

Same here as with the dimensions

@@ +259,3 @@
+    /* we do not support sub-sampled RGB or monochrome */
+    if (strcmp (colourspace, "sYUV") && (dx > 1 || dy > 1)) {
+      /* TODO log error                */

Yes, everywhere :) Take a look at GST_ELEMENT_ERROR() and how it's used
elsewhere, and use it on all errors/early returns. And add a few debug log
lines in other places too please :)

@@ +260,3 @@
+    if (strcmp (colourspace, "sYUV") && (dx > 1 || dy > 1)) {
+      /* TODO log error                */
+      return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +269,3 @@
+
+    /* TODO log error */
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +279,3 @@
+      for (i = 0; i < 4; ++i) {
+        if (jpeg2000parse->dx[i] > 1 || jpeg2000parse->dy[i] > 1) {
+          return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +355,3 @@
+
+    if (!gst_pad_set_caps (GST_BASE_PARSE_SRC_PAD (parse), caps))
+      ret = GST_FLOW_NOT_NEGOTIATED;

You probably want to get out here instead of continuing with the code below

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