[Bug 766236] rtp j2k payload/depayload messes up colours in sample pattern

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=766236

--- Comment #82 from boxerab at gmail.com <boxerab at gmail.com> ---
Thanks for the review.

(In reply to Sebastian Dröge (slomo) from comment #81)
> Review of attachment 328918 [details] [review]:
> 
> 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
> 

good idea


> ::: 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?
> 

I don't think our payloader will handle jp2.


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

Yes.

> @@ +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
> 

Thanks.

> @@ +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?
> 

Yes.

> @@ +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
> 

How do I know how much I consumed? What if the handle_frame method was called
multiple times: do I need to track how much I consumed over these multiple
method calls ?




> @@ +280,3 @@
> +beach:
> +
> +  gst_buffer_unmap (frame->buffer, &map);
> 
> You should unmap before you finish

Yes, thanks.

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