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

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

Mostly style things now :)

::: gst/videoparsers/gstjpeg2000parse.c
@@ +27,3 @@
+
+#include <gst/base/base.h>
+#include <gst/pbutils/pbutils.h>

I think this header is unneeded :)

@@ +108,3 @@
+  }
+
+

Double empty line here

@@ +151,3 @@
+  gboolean dimensions_changed = FALSE;
+  guint8 dx[GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS];
+  guint8 dy[GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS];       //
sub-sampling factors

No // comments please

@@ +159,3 @@
+
+  if (!gst_buffer_map (frame->buffer, &map, GST_MAP_READ)) {
+    GST_ERROR_OBJECT (jpeg2000parse, "Unable to map buffer");

GST_ERROR_OBJECT() and GST_ELEMENT_ERROR() are different things btw. The first
is just doing debug output at "error" level, the second posts an error message
on the bus for the application to get more information about any error

@@ +209,3 @@
+  if (x1 < x0 || y1 < y0) {
+    GST_ERROR_OBJECT (jpeg2000parse, "Nonsensical image dimensions
%d,%d,%d,%d",
+        x0, y0, x1, y1);

... which means that these should be GST_ELEMENT_ERROR() instead

@@ +300,3 @@
+          GST_ERROR_OBJECT (jpeg2000parse,
+              "Sub-sampled YUVA images not supported");
+          ret = GST_FLOW_NOT_NEGOTIATED;

Maybe for all these unsupported cases we should output caps without the
sampling field? Elements that need the sampling field (RTP payloader!) will
have it on its sink template caps, so negotiation will fail there anyway... but
other elements that don't care and somehow decode this to something we can
handle will still work

@@ +331,3 @@
+    }
+  } else if (!strcmp (colorspace, "GRAY")) {
+

You in general have many empty lines at the beginning or end of blocks. We
usually don't do that :) Only in the middle of blocks to structure things

@@ +424,3 @@
+    ret = gst_base_parse_finish_frame (parse, frame, frame_size);
+
+    jpeg2000parse->found_frame = FALSE;

This should probably also be set to FALSE when flushing. But why do you need it
at all? You could just check in the beginning of handle_frame() if the data
starts with the magic, that's about as complicated as reading the boolean and
keeps the state you have to carry around a bit smaller

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