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

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

Just a very very short review, I'll look in more detail next week :)

Generally looks good, thanks for your work!

::: gst/videoparsers/gstjpeg2000parse.c
@@ +175,3 @@
+
+    if (magic_offset == -1) {
+      /*printf ("\nNo Magic %d \n", gst_byte_reader_get_size (&reader)); */

For all the printf things you can use GST_DEBUG_OBJECT, GST_WARNING_OBJECT, etc

@@ +182,3 @@
+    } else {
+      /*printf ("\nFound Magic at %d \n", magic_offset); */
+      jpeg2000parse->magic_offset = magic_offset;

As you skip everything before the magic, this offset should always be 0, no? If
you don't let the base class skip everything before the magic for you, it would
become part of the output which we don't really want

@@ +223,3 @@
+
+  if (numcomps > GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS) {
+    /* TODO log error */

GST_ELEMENT_ERROR

@@ +228,3 @@
+
+
+  current_caps = gst_pad_get_current_caps (GST_BASE_PARSE_SINK_PAD (parse));

You leak the caps

@@ +275,3 @@
+  if (numcomps == 4 && !strcmp (colourspace, "sYUV")) {
+    sampling = GST_RTP_J2K_YBRA;
+    samplingEnum = GST_RTP_SAMPLING_YBRA;

It might be subsampled too here, should error out as unsupported or just not
write any sampling for now

@@ +293,3 @@
+    } else if (dx == 2 && dy == 1) {
+      sampling = GST_RTP_J2K_YBR422;
+      samplingEnum = GST_RTP_SAMPLING_YBR422;

else?

@@ +308,3 @@
+        sampling = GST_RTP_J2K_RGB;
+        samplingEnum = GST_RTP_SAMPLING_RGB;
+      }

RGB could be subsampled, which we also don't support currently here. The bayer
RGB formats are subsampled for example

@@ +320,3 @@
+    if (!gst_pad_set_caps (GST_BASE_PARSE_SRC_PAD (parse), caps)) {
+      /* TODO log error */
+      ret = GST_FLOW_ERROR;

GST_FLOW_NOT_NEGOTIATED

@@ +347,3 @@
+  }
+
+BEACH:

We usually name goto labels lowercase :) But beach is a good name

@@ +355,3 @@
+
+static GstFlowReturn
+gst_jpeg2000_parse_pre_push_frame (GstBaseParse * parse,

This can go away

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