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

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

::: gst/rtp/gstrtpj2kdepay.c
@@ +195,3 @@
+  /* required field */
+  sampling = gst_structure_get_string (structure, "sampling");
+  if (!strcmp (sampling, "RGB"))

RGBA, BGR, etc should also be sRGB

@@ +197,3 @@
+  if (!strcmp (sampling, "RGB"))
+    colorspace = "sRGB";
+  else if (!strcmp (sampling, "YCbCr-4:4:4"))

The other YCbCr-* ones should also be sYUV, GRAYSCALE should become GRAY

@@ +200,3 @@
+    colorspace = "sYUV";
+  else
+    colorspace = "sRGB";

This changes behaviour, previously the fallback was sYUV and now it is sRGB.

::: gst/rtp/gstrtpj2kpay.c
@@ +77,3 @@
     GST_STATIC_CAPS ("application/x-rtp, "
+        "  media = (string) \"video\", " "  clock-rate = (int) 90000, "
+        /*"sampling = (string) {\"RGB\", \"BGR\", \"RGBA\", \"BGRA\",
\"YCbCr-4:4:4\",\"YCbCr-4:2:2\", \"YCbCr-4:2:0\", \"YCbCr-4:1:1\",
\"GRAYSCALE\"}, " */

Why commented out? We should always set this :)

@@ +181,3 @@
+  if (!pay->sampling_set && colorspace) {
+    if (!strcmp (colorspace, "sYUV"))
+      sampling = "YCbCr-4:4:4";

We're setting wrong information for sYUV and sRGB here now. It requires parsing
of the stream to know which of the many variants it is.

Also the problem here is that we need to know this at the time when the caps
are first set on the payloader source pad, that is when we get the caps from
upstream and not when the first data is parsed. Otherwise gst-rtsp-server for
example doesn't handle it ideally.

I think the only generic solution here is to a) get a jpeg2000parse element
that extracts this into the caps, b) let the encoders/decoders add this info to
the caps too. Then you can get it directly from the caps here, and if the field
is not there you would just fail.

@@ +289,3 @@
+          guint32 len = gst_rtp_j2k_pay_header_size (data, offset);
+          GST_LOG_OBJECT (pay, "found SIZ at %u", offset);
+          state->num_components = data[2 + 33]; /* assume little endian
machine */

Why does this assume little endian? You only read one byte, no?

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