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

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

Some comments below, just took a very short look

::: docs/plugins/inspect/plugin-videoparsersbad.xml
@@ +162,3 @@
+      <class>Codec/Parser/Video/Image</class>
+      <description>Parses JPEG 2000 files</description>
+      <author>Olivier Crete <olivier.crete at collabora.com></author>

This looks wrong :) You might want to regenerate the XML file

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

It should also handle j2c and jp2 later, that's on your list?

@@ +159,3 @@
+    GstRtpSampling samplingEnum = GST_RTP_SAMPLING_NONE;
+
+    current_caps = gst_pad_get_current_caps (GST_BASE_PARSE_SINK_PAD (parse));

You leak these

@@ +198,3 @@
+
+
+    if (numcomps >= 3 && !strcmp (colourspace, "sYUV")) {

There's also AYUV here

@@ +219,3 @@
+      jpeg2000parse->dx = dy;
+
+      if (dx == 1 && dy == 1) {

RGB(A) can in theory also be subsampled, but we don't support that. Probably
should check for that and error out below

@@ +269,3 @@
+
+  if (offset == -1) {
+    *skipsize = gst_byte_reader_get_remaining (&reader) - 2;

If you don't find the EOC, you probably want to wait for more data instead of
throwing away all you have?

@@ +274,3 @@
+    ret =
+        gst_base_parse_finish_frame (parse, frame,
+        gst_byte_reader_get_remaining (&reader));

The second argument is the number of bytes that you finish (that is: how much
you consumed, not necessarily how much you forward), not the number of bytes
that are still leftover

@@ +280,3 @@
+beach:
+
+  gst_buffer_unmap (frame->buffer, &map);

You should unmap before you finish

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