After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766236 - rtp j2k payload/depayload messes up colours in sample pattern
rtp j2k payload/depayload messes up colours in sample pattern
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 639231 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-05-10 17:05 UTC by Aaron Boxer
Modified: 2016-06-10 13:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstrtpj2k: set colour space correctly (2.42 KB, patch)
2016-05-13 19:09 UTC, Aaron Boxer
none Details | Review
gstrtpj2k: set colour space and sampling fields in caps (9.50 KB, patch)
2016-05-20 02:20 UTC, Aaron Boxer
none Details | Review
gstrtpj2k: set colour space and sampling fields in caps (9.99 KB, patch)
2016-05-20 13:10 UTC, Aaron Boxer
none Details | Review
set colour space and sampling correctly (10.49 KB, patch)
2016-05-20 19:19 UTC, Aaron Boxer
none Details | Review
set sampling field in OpenJPEG encoder (4.41 KB, patch)
2016-05-20 19:26 UTC, Aaron Boxer
none Details | Review
Set sampling field in OpenJPEG encoder (4.07 KB, patch)
2016-05-20 21:33 UTC, Aaron Boxer
none Details | Review
Set sampling field in OpenJPEG encoder (5.21 KB, patch)
2016-05-21 19:55 UTC, Aaron Boxer
none Details | Review
set colour space and sampling correctly (9.70 KB, patch)
2016-05-21 19:57 UTC, Aaron Boxer
none Details | Review
set colour space and sampling correctly (9.70 KB, patch)
2016-05-21 21:54 UTC, Aaron Boxer
none Details | Review
set sampling field in OpenJPEG encoder (5.11 KB, patch)
2016-05-22 00:45 UTC, Aaron Boxer
none Details | Review
set sampling field in OpenJPEG encoder (5.81 KB, patch)
2016-05-23 03:17 UTC, Aaron Boxer
none Details | Review
set sampling field in openjpeg encoder (6.19 KB, patch)
2016-05-23 12:53 UTC, Aaron Boxer
none Details | Review
set sampling field in openjpeg encoder (6.31 KB, patch)
2016-05-23 12:57 UTC, Aaron Boxer
none Details | Review
Prototype jpeg 2000 parser (17.65 KB, patch)
2016-05-28 19:41 UTC, Aaron Boxer
none Details | Review
Prototype jpeg 2000 parser (15.02 KB, patch)
2016-05-28 21:00 UTC, Aaron Boxer
none Details | Review
set sampling field in OpenJPEG encoder (8.38 KB, patch)
2016-06-01 23:07 UTC, Aaron Boxer
none Details | Review
Created basic parser for j2k streams (14.28 KB, patch)
2016-06-01 23:09 UTC, Aaron Boxer
none Details | Review
Created basic parser for j2k streams (16.18 KB, patch)
2016-06-02 02:10 UTC, Aaron Boxer
none Details | Review
trying to improve parser (10.21 KB, patch)
2016-06-02 14:30 UTC, Aaron Boxer
none Details | Review
trying to improve parser (11.05 KB, patch)
2016-06-02 19:48 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (17.76 KB, patch)
2016-06-03 02:24 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (18.28 KB, patch)
2016-06-03 11:56 UTC, Aaron Boxer
reviewed Details | Review
jpeg 2000 parser (19.02 KB, patch)
2016-06-03 12:55 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (18.53 KB, patch)
2016-06-03 13:19 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (18.58 KB, patch)
2016-06-03 14:18 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.05 KB, patch)
2016-06-06 12:40 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.29 KB, patch)
2016-06-06 13:02 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.40 KB, patch)
2016-06-06 14:58 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.42 KB, patch)
2016-06-06 15:31 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.28 KB, patch)
2016-06-07 12:09 UTC, Aaron Boxer
none Details | Review
jpeg 2000 parser (20.31 KB, patch)
2016-06-07 12:19 UTC, Aaron Boxer
committed Details | Review
set sampling field in payloader (8.76 KB, patch)
2016-06-07 15:45 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (10.33 KB, patch)
2016-06-07 20:47 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (10.33 KB, patch)
2016-06-07 20:49 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (10.69 KB, patch)
2016-06-08 14:19 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (10.83 KB, patch)
2016-06-08 14:31 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (5.97 KB, patch)
2016-06-08 15:34 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (5.97 KB, patch)
2016-06-09 12:49 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (14.75 KB, patch)
2016-06-09 19:59 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (14.31 KB, patch)
2016-06-09 20:11 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (14.41 KB, patch)
2016-06-09 20:13 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.70 KB, patch)
2016-06-09 20:22 UTC, Aaron Boxer
none Details | Review
set sampling field in payloader (11.04 KB, patch)
2016-06-10 09:59 UTC, Aaron Boxer
committed Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.96 KB, patch)
2016-06-10 10:21 UTC, Aaron Boxer
needs-work Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.96 KB, patch)
2016-06-10 10:27 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.64 KB, patch)
2016-06-10 11:17 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.20 KB, patch)
2016-06-10 11:32 UTC, Aaron Boxer
none Details | Review
jpeg2000 parse: require either sampling field or colorspace field in sink caps (15.58 KB, patch)
2016-06-10 11:59 UTC, Aaron Boxer
committed Details | Review

Description Aaron Boxer 2016-05-10 17:05:05 UTC
When I run the following pipeline:

gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink


the background of the pattern is green and the ball is pink.

Without rtp, the background is white and the ball is black.


I run this using master branch of openjpeg, gstreamer, gst-plugins-base, gst-plugins-good and gst-plugins-bad.
Comment 1 Aaron Boxer 2016-05-10 17:05:48 UTC
OS is XUbuntu
Comment 2 Tim-Philipp Müller 2016-05-10 17:22:02 UTC
Somewhere the colorspace signalling got lost:

rtpj2kpay0.sink: caps = image/x-jpc, colorspace=(string)sRGB, ...

rtpj2kdepay0.src: caps = image/x-jpc, colorspace=(string)sYUV, ...
Comment 3 Aaron Boxer 2016-05-11 01:56:25 UTC
not sure how to trouble shoot this.....
Comment 4 Sebastian Dröge (slomo) 2016-05-11 06:30:47 UTC
Check the payloader and depayloader code to see where it gets the colorspace from and how it writes it to the stream (or is it only part of the caps?). There must be the problem somewhere.
Comment 5 Sebastian Dröge (slomo) 2016-05-11 06:54:56 UTC
The depayloader statically sets it to sYUV, the payloader does not read it at all. Is there a way to write it into the stream or is it supposed to be part of the SDP, if neither then the payloader should only accept sYUV (or whatever the spec mandates).
Comment 6 Aaron Boxer 2016-05-11 18:26:10 UTC
Yes, can verify that if I statically set to sRGB, then the colour is correct.

I don't think the stream stores the colour space info, so it can't be written there.
Comment 7 Sebastian Dröge (slomo) 2016-05-11 20:59:08 UTC
https://tools.ietf.org/html/rfc5371#section-6

It should be in the "sampling" field in the SDP, and it is actually a required field. So it should be added by the payloader to the application/x-rtp caps, and parsed by the depayloader from the caps and put into the srcpad caps again. And both elements should require the field to be present in the sinkpad caps.

Do you want to prepare a patch for that?
Comment 8 Aaron Boxer 2016-05-12 00:44:16 UTC
Sorry, have no idea how to do this. Is there an example of this in another plugin?
Comment 9 Sebastian Dröge (slomo) 2016-05-12 06:33:58 UTC
If you want to take a look, take gst_rtp_opus_depay_setcaps() and gst_rtp_opus_pay_setcaps() as examples. Basically you get the first structure of the caps, parse the relevant field from it, then put that on the output caps and possibly transform it to the correct format while doing so.

Let me know, also if you have more questions. Otherwise I'll make a patch for that at some point :)
Comment 10 Aaron Boxer 2016-05-12 12:51:31 UTC
Thanks, I will give it a try.
Comment 11 Aaron Boxer 2016-05-12 12:55:11 UTC
Are there test patterns for the various colour spaces that need to be supported? So I can test it out?
Comment 12 Sebastian Dröge (slomo) 2016-05-13 07:13:32 UTC
Just videotestsrc with the default pattern is good enough :)
Comment 13 Aaron Boxer 2016-05-13 19:09:08 UTC
Created attachment 327819 [details] [review]
gstrtpj2k: set colour space correctly
Comment 14 Sebastian Dröge (slomo) 2016-05-14 07:46:50 UTC
Comment on attachment 327819 [details] [review]
gstrtpj2k: set colour space correctly

You actually need to convert the strings. In GStreamer we use only sRGB, sYUV and GRAY. The RTP RFC defines RGB, BGR, RGBA, BGRA, YCbCr-*:*:*, GRAYSCALE.

This might need either addition of new fields to the caps in the encoder/decoder (to know component order and subsampling), or that information needs to be parsed from the stream in the payloader.
Comment 15 Aaron Boxer 2016-05-14 16:24:41 UTC
The stream contains the following information in the main header:

1) subsampling specification
2) whether J2K multi component transform (MCT) was performed or not

MCT is the J2K transform from RGB to YUV.  So, if MCT == 0, then we
can assume (I think) that the space is YUV.  

Combining these two fields, we can deduce the colour space.

But, I would need to set the video caps after I start reading the stream.
How would I do that?
Comment 16 Sebastian Dröge (slomo) 2016-05-15 07:47:35 UTC
I think that will be annoying to do in the payloader/depayloader. Would be better to add this information to the caps in the decoder/encoder... and write a jpeg2000parse element that extracts it from a stream if not given.

You could however set the caps on the srcpad only after parsing some packets, instead of doing it directly in set_caps().
Comment 17 Aaron Boxer 2016-05-15 20:22:30 UTC
OK, thanks. I need to spend some time RTFMing the gstreamer docs, then I will give this a try :)
Comment 18 Aaron Boxer 2016-05-19 15:24:20 UTC
When I add the "sampling" field to the caps in the payloader, the video stream errors out with "reason not negotiated (-4)"

To troubleshoot this, I am debugging the stream using the following:

GST_DEBUG=*:6 gst-launch-1.0 ....   2>&1 | grep -B50 --max-count=1 not-negotiated
Comment 19 Aaron Boxer 2016-05-19 16:03:52 UTC
Never mind, fixed this.  Patch coming shortly.
Comment 20 Aaron Boxer 2016-05-20 02:20:57 UTC
Created attachment 328239 [details] [review]
gstrtpj2k: set colour space and sampling fields in caps
Comment 21 Sebastian Dröge (slomo) 2016-05-20 06:23:11 UTC
Review of attachment 328239 [details] [review]:

::: 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?
Comment 22 Aaron Boxer 2016-05-20 13:09:51 UTC
Thanks for the review.  I am attaching an updated patch that addresses most of the issues you raised. But, if we do this colour space parsing in another element, this involves parsing the stream twice for sources that do not set the "sampling" field. Is this not inefficient?
Comment 23 Aaron Boxer 2016-05-20 13:10:28 UTC
Created attachment 328258 [details] [review]
gstrtpj2k: set colour space and sampling fields in caps
Comment 24 Aaron Boxer 2016-05-20 19:19:58 UTC
Created attachment 328287 [details] [review]
set colour space and sampling correctly

Here is a small tweak to the last patch.
Comment 25 Aaron Boxer 2016-05-20 19:26:02 UTC
Created attachment 328288 [details] [review]
set sampling field in OpenJPEG encoder
Comment 26 Aaron Boxer 2016-05-20 21:33:56 UTC
Created attachment 328293 [details] [review]
Set sampling field in OpenJPEG encoder
Comment 27 Aaron Boxer 2016-05-20 22:03:34 UTC
Sebastian, you were right about inadvisability of setting caps after parsing the stream. When I do this, and pass in an RGB test signal, the output seems to switch sYUV. 

I will create a new element that will pull from stream and set in caps for payloader to read.

Since I have added the sampling field in our  OpenJPEG encoder, this new element should be a no-op if gstreamer is producing the jpeg 2000 stream. 

If another server streams JPC video to gstreamer, then we will have to parse the stream twice. I guess it shouldn't be a big deal.
Comment 28 Sebastian Dröge (slomo) 2016-05-21 06:34:24 UTC
That element should be based on GstBaseParse and called jpeg2000parse, and it should also accept frames coming in not as one frame per buffer but in any number of bytes per buffer. Basically like jpegparse, but based on GstBaseParse to prevent most of the problems jpegparse currently has. Take a look at e.g. pngparse for an example.
Comment 29 Sebastian Dröge (slomo) 2016-05-21 06:39:24 UTC
Review of attachment 328293 [details] [review]:

::: ext/openjpeg/gstopenjpeg.h
@@ +56,3 @@
+#define GST_RTP_J2K_GRAYSCALE "GRAYSCALE"
+
+#define GST_RTP_J2K_SAMPLING_LIST "sampling = (string) {\"RGB\", \"BGR\", \"RGBA\", \"BGRA\", \"YCbCr-4:4:4\",\"YCbCr-4:2:2\", \"YCbCr-4:2:0\", \"YCbCr-4:1:1\", \"GRAYSCALE\"}"

You could in theory build the list out of the other #defines above to prevent copying them

::: ext/openjpeg/gstopenjpegenc.c
@@ +640,2 @@
     colorspace = "sYUV";
+    sampling = GST_RTP_J2K_YBR444;

You should take a look at the component subsampling values here. AYUV/Y444 are 4:4:4, I422/Y42B is 4:2:2, I420 is 4:2:0, Y41B is 4:1:1

@@ +643,2 @@
     colorspace = "sRGB";
+    sampling = GST_RTP_J2K_RGB;

Does the order matter here, if so you also need to check those
Comment 30 Aaron Boxer 2016-05-21 18:59:17 UTC
Thanks for the review. 

I will provide a patch shortly.

But, after more thought, I don't think we can infer what sampling to set
from the code stream. MCT is optional, so RGB may be coded without MCT,
while YUV may be coded with MCT.

I think the best solution is to require the sampling field for any source feeding into the rtp j2k payloader.  I will leave the current header parsing so we can
do a sanity check against the stream if we want to in the future.

So, I won't create a new element to parse the stream.
Comment 31 Aaron Boxer 2016-05-21 19:02:34 UTC
What do you think?
Comment 32 Aaron Boxer 2016-05-21 19:55:09 UTC
Created attachment 328322 [details] [review]
Set sampling field in OpenJPEG encoder

Now handling subsampling.
Comment 33 Aaron Boxer 2016-05-21 19:57:03 UTC
Created attachment 328323 [details] [review]
set colour space and sampling correctly
Comment 34 Aaron Boxer 2016-05-21 19:58:12 UTC
I think these last two patches fix the bug the right way, by adding support for the sampling field in the j2k encoder, and in the payloader.
Comment 35 Aaron Boxer 2016-05-21 21:54:04 UTC
Created attachment 328326 [details] [review]
set colour space and sampling correctly

changed C++ style comment to C99
Comment 36 Aaron Boxer 2016-05-22 00:45:35 UTC
Created attachment 328331 [details] [review]
set sampling field in OpenJPEG encoder
Comment 37 Aaron Boxer 2016-05-22 13:53:44 UTC
Not sure if I am handling the colour spaces properly:

  switch (state->info.finfo->format) {
    case GST_VIDEO_FORMAT_ARGB64:
    case GST_VIDEO_FORMAT_ARGB:
    case GST_VIDEO_FORMAT_AYUV64:
    case GST_VIDEO_FORMAT_AYUV:
      sampling = GST_RTP_J2K_RGBA;
      break;


RFC does not support ARGB format, so what should we do in this case?
Comment 38 Nicolas Dufresne (ndufresne) 2016-05-22 14:24:33 UTC
For opaque but padded RGB format, you'll bind BGRx/xBGR/RGBx/xRGB. Only use ARGB with such formats if you can guaranty the padded value is fully opaque. Using J2K_RGBA to expose any packed format seems rather strange.
Comment 39 Aaron Boxer 2016-05-22 14:51:01 UTC
Thanks. So, for ARGB64, for example, are you saying that we should not send this over RTP, because RFC does not support this format ?
Comment 40 Nicolas Dufresne (ndufresne) 2016-05-22 21:05:26 UTC
If there is no rfc, not even a draft or defacto standard, you should make the negotiation fail. Using a format that does not match will likely cause issues.
Comment 41 Aaron Boxer 2016-05-22 21:20:36 UTC
Thanks, Nicolas. Actually, for these packed formats, the encoder unpacks
them and stores in separate colour channels. So, it is appropriate to use RGBA
for ARGBA64, although I have to look into managing the colour channel reversal.
Comment 42 Aaron Boxer 2016-05-23 03:16:30 UTC
The encoder also re-orders the alpha channel, so that it is the last channel.
So, it is appropriate to tread ARGB64 as RGBA, once the image is encoded.
Comment 43 Aaron Boxer 2016-05-23 03:17:36 UTC
Created attachment 328361 [details] [review]
set sampling field in OpenJPEG encoder

sampling should now be correct, with packed formats, alpha channel, etc.
Comment 44 Sebastian Dröge (slomo) 2016-05-23 07:01:20 UTC
Review of attachment 328361 [details] [review]:

::: ext/openjpeg/gstopenjpegenc.c
@@ +642,3 @@
+    case GST_VIDEO_FORMAT_AYUV64:
+    case GST_VIDEO_FORMAT_AYUV:
+      sampling = GST_RTP_J2K_RGBA;

Shouldn't AYUV be YBRA444 or something?

@@ +649,3 @@
+    case GST_VIDEO_FORMAT_Y444_10LE:
+    case GST_VIDEO_FORMAT_Y444_10BE:
+    case GST_VIDEO_FORMAT_Y444:

Do we need to distinguish between 8 bit per component, 10 bit, 16 bit?

@@ +651,3 @@
+    case GST_VIDEO_FORMAT_Y444:
+    case GST_VIDEO_FORMAT_YUV9:
+      sampling = GST_RTP_J2K_YBR444;

YUV9 is 4:1:0

@@ +660,3 @@
+      break;
+    case GST_VIDEO_FORMAT_Y41B:
+      sampling = GST_RTP_J2K_YBR410;

Y41B is 4:1:1
Comment 45 Aaron Boxer 2016-05-23 12:45:40 UTC
Thanks.

The RFC doesn't mention YUV with alpha, so I am not going to set the sampling
field for this format.

Regarding different bit depths, the code stream itself will store this information.
Comment 46 Aaron Boxer 2016-05-23 12:53:56 UTC
Created attachment 328382 [details] [review]
set sampling field in openjpeg encoder
Comment 47 Aaron Boxer 2016-05-23 12:57:27 UTC
Created attachment 328383 [details] [review]
set sampling field in openjpeg encoder
Comment 48 Aaron Boxer 2016-05-23 12:58:25 UTC
I think this bug has been fixed ! :)
Comment 49 Sebastian Dröge (slomo) 2016-05-25 06:59:00 UTC
(In reply to boxerab@gmail.com from comment #45)
> Thanks.
> 
> The RFC doesn't mention YUV with alpha, so I am not going to set the sampling
> field for this format.

Makes sense

> Regarding different bit depths, the code stream itself will store this
> information.

So the RFC cares about subsampling and stuff, but not about the component depths? That seems weird, it's as important for negotiating a compatible format :) Maybe they assume 8 bit and don't allow anything else?

The subsampling is also stored in the stream itself, no?

(In reply to boxerab@gmail.com from comment #48)
> I think this bug has been fixed ! :)

I think to really consider it fixed we should also have a jpeg2000 parsers. Otherwise the payloader won't work very well anymore in many pipelines and there's no easy fix for that.
Comment 50 Aaron Boxer 2016-05-25 11:58:06 UTC
(In reply to Sebastian Dröge (slomo) from comment #49)

> 
> > Regarding different bit depths, the code stream itself will store this
> > information.
> 
> So the RFC cares about subsampling and stuff, but not about the component
> depths? That seems weird, it's as important for negotiating a compatible
> format :) Maybe they assume 8 bit and don't allow anything else?


My guess is that the RFC assumes that bit depth is taken care of by the J2K design: JPEG 2000 images are encoded bit plane by bit plane, so if an image is 12 bits, but the decoder only supports 8, then it can easily decode the most significant 8 bit planes, and ignore the rest. 


> 
> The subsampling is also stored in the stream itself, no?

Yes.

> 
> (In reply to boxerab@gmail.com from comment #48)
> > I think this bug has been fixed ! :)
> 
> I think to really consider it fixed we should also have a jpeg2000 parsers.
> Otherwise the payloader won't work very well anymore in many pipelines and
> there's no easy fix for that.

Well, I don't believe the parser can generally determine the sampling field from the code stream alone. There are cases where the JP2 file format stores a colour box, so this combined with the subsampling could determine it. But this box is optional.

Can you tell me about the pipelines that will not work if we don't have a parser ?  i.e. we don't have a gstreamer encoder in the pipeline, but we are streaming J2K images ?
Comment 51 Aaron Boxer 2016-05-25 23:47:54 UTC
So, it turns out that the colour box is mandatory for JPEG 2000 jp2 images (code stream plus file format), so if the source
is a motion jpeg 2000 file, or still jpeg 2000 files with jp2 file format, then 
a filter could determine the sampling field. If the source is j2k or jpc files that contain the code stream only, and nothing else, then we won't be able to determine the sampling.
Comment 52 Sebastian Dröge (slomo) 2016-05-26 05:51:27 UTC
Ok, so we should make this caps field a requirement in general, which OTOH needs modifications to all demuxers and other elements that currently support JPEG2000 :)

We should probably also extend it then by other samplings that are not covered by the RFC but that we still support (e.g. AYCbCr-4:4:4)
Comment 53 Aaron Boxer 2016-05-26 12:32:52 UTC
Sounds like a good plan :)
Comment 54 Aaron Boxer 2016-05-27 00:08:49 UTC
This work could get complicated: the J2K colour box can contain ICC colour profiles that need to be parsed. OpenJPEG uses LCMS library.

This feels more like a new feature than a bug fix. Can we close this issue,
commit the existing patches, and open an enhancement for a J2K filter element ?

The original issue I reported is fixed with the current set of patches.

Cheers,
Aaron
Comment 55 Sebastian Dröge (slomo) 2016-05-27 05:49:36 UTC
The problem is that the current patch makes it impossible to payload the JPEG2000 streams that come from anywhere else than openjpegenc. That's a regression compared to before.


Why are the ICC colour profiles relevant here? In the end what only really matters is the sampling, which should either be parsed from the codestream directly if available, and from all demuxers should be possible to add to the caps directly based on container knowledge.
Comment 56 Aaron Boxer 2016-05-27 11:55:28 UTC
(In reply to Sebastian Dröge (slomo) from comment #55)
> The problem is that the current patch makes it impossible to payload the
> JPEG2000 streams that come from anywhere else than openjpegenc. That's a
> regression compared to before.

Yes, that makes sense.

> 
> 
> Why are the ICC colour profiles relevant here? In the end what only really
> matters is the sampling, which should either be parsed from the codestream
> directly if available, and from all demuxers should be possible to add to
> the caps directly based on container knowledge.

Good point, ICC is for mapping to display device, so not necessary to worry about
it.

Would you be able to set up the boilerplate code for this filter?  Everything except the stream parsing, and leave the parsing to me ?
Comment 57 Sebastian Dröge (slomo) 2016-05-27 12:09:53 UTC
pngparse would be a good template, it does not contain much parsing code anyway and basically does the same: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/videoparsers/gstpngparse.c

It's parsing the stream for some properties to be put into the caps, specifically the width/height, while keeping over the framerate from the input for example. You want something similar, just for JPEG2000. It should also extract width/height from the stream, and additionally the colorspace and sampling as discussed.

Also see https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/jp2kdecimator , it contains code for parsing JPEG2000 streams (and probably much more of the stream than you actually need).


On top of that, we should look at the existing muxers/demuxers (+ other elements that handle JPEG2000) for checking if they can provide (or use) this information.
Comment 58 Aaron Boxer 2016-05-28 19:41:54 UTC
Created attachment 328674 [details] [review]
Prototype jpeg 2000 parser

I have created a prototype jpeg 2000 parser, based off of the png parser.
It does no parsing at the moment.

When I add it to my pipeline as follows:

gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink

I get the error 

"erroneous pipeline: could not link openjpegenc to jpeg2000 parse
Comment 59 Aaron Boxer 2016-05-28 21:00:07 UTC
Created attachment 328682 [details] [review]
Prototype jpeg 2000 parser

No errors now, but no bouncing ball either.  I want this parser to simply 
pass through the video at this stage, with no parsing. Any ideas on why this is not working ?
Comment 60 Tim-Philipp Müller 2016-05-28 21:16:49 UTC
Presumably because in your _handle_frame function you're not calling finish_frame(), it's all commented out. If _handle_frame() is not called your parse function is at fault.

Please also put your own name there as copyright and in the metadata, there's no need to keep the pngparse author here if you just copy the skeleton :)
Comment 61 Aaron Boxer 2016-05-28 23:07:46 UTC
(In reply to Tim-Philipp Müller from comment #60)
> Presumably because in your _handle_frame function you're not calling
> finish_frame(), it's all commented out. If _handle_frame() is not called
> your parse function is at fault.
> 
> Please also put your own name there as copyright and in the metadata,
> there's no need to keep the pngparse author here if you just copy the
> skeleton :)

Thanks. What about choice of license : what is the policy about FLOSS licensing ?
Comment 62 Tim-Philipp Müller 2016-05-28 23:24:32 UTC
"LGPL v2.1 or later" same as everything else please.
Comment 63 Aaron Boxer 2016-05-29 22:00:19 UTC
Alright, thanks.

I am not very familiar with the parse api: could someone please give me a code snippet that would:

A) simply pass through the source, including the source's caps

or 

B) search for first 0xFF byte in source, then pass through the rest of the source stream, including the source's caps  

This would give me a better idea of how to build the actual parser.
Comment 64 Sebastian Dröge (slomo) 2016-05-30 06:00:01 UTC
Take a look at https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/videoparsers/gstpngparse.c#n124

That's where all the parsing work is happening. You get data from the base class, parse it and then decide if you need more data or if you can finish the frame. That's all that there is about the baseparse API for your case.

The PNG parser first searches for the PNG_SIGNATURE in the stream and drops everything before that (that is, it searches for the beginning of a frame). And once it has that, it parses the frame and requests more data from the base class if it has not enough data yet. Once the full frame is there and all information is parsed, it finishes the frame and then starts again with looking for the signature.


Note that looking for the first 0xff byte is not enough. For a proper parser you need to look at all the tags of a frame, and then finish a frame only when you have all tags of a complete frame (but not any of the next). You should finish one frame at a time.
Comment 65 Aaron Boxer 2016-05-30 12:42:13 UTC
Thanks, I think I get it now.

Another question: this parser will only accept jp2 streams. Can you suggest a test pipeline that will stream a single jp2 image, repeatedly, over rtp, then depayload and display in window ? This will make it easy for me to see if the 
parser is working properly.
Comment 66 Sebastian Dröge (slomo) 2016-05-30 17:43:35 UTC
You can just test it with
> videotestsrc ! openjpegenc ! jp2kparse ! openjpegdec ! videoconvert ! autovideosink
for example :) Or
> filesrc location=test.jpg ! jpegdec ! imagefreeze ! openjpegenc ! ...
for a single frame over and over.
Comment 67 Aaron Boxer 2016-05-31 11:12:48 UTC
Thanks. Are there any work flows that you know of involving jpeg 2000 that involve jp2 files ? j2c and j2k indicate pure code stream files, while jp2 
use jpeg 2000 file format, which includes the "colour" box that can be used
to infer the sampling field.

openjpegenc only produces j2c streams, but it does set the sampling field, in my patches. 

For j2c and j2k streams that do not involve openjpegenc plugin, it will not 
be possible to figure out the sampling field.

To test this, I do need a jp2 work flow.

Also, do you know of any users who are using gstreamer for jpeg 2000 streaming? Would like to get them involved.
Comment 68 Sebastian Dröge (slomo) 2016-05-31 11:34:20 UTC
You could use multifilesrc with jp2 files for example.

The other producers (and consumers) of j2c/j2k code streams should provide the sampling and other things on the caps too. I think they can, it's mostly demuxers.

Also the sampling should be possible to detect from the width/height differences of the different planes, no? Just what the components are is unknown?
Comment 69 Aaron Boxer 2016-05-31 12:18:33 UTC
(In reply to Sebastian Dröge (slomo) from comment #68)
> You could use multifilesrc with jp2 files for example.

Thanks. I could use this for testing. Are there real work flows that involve jp2 ?

> 
> The other producers (and consumers) of j2c/j2k code streams should provide
> the sampling and other things on the caps too. I think they can, it's mostly
> demuxers.
> 
> Also the sampling should be possible to detect from the width/height
> differences of the different planes, no? Just what the components are is
> unknown?

sub-sampling and image dimensions are stored in the code stream, so there is no
problem getting this info.  But, how to determine RGB vs YUV ? I was going to use 
the colour box, only available for jp2 streams.

But, I don't think anyone streams jp2 files in the wild ......
Comment 70 Sebastian Dröge (slomo) 2016-05-31 12:52:05 UTC
(In reply to boxerab@gmail.com from comment #69)
> (In reply to Sebastian Dröge (slomo) from comment #68)
> > You could use multifilesrc with jp2 files for example.
> 
> Thanks. I could use this for testing. Are there real work flows that involve
> jp2 ?

I don't know, wouldn't surprise me though :)

> > The other producers (and consumers) of j2c/j2k code streams should provide
> > the sampling and other things on the caps too. I think they can, it's mostly
> > demuxers.
> > 
> > Also the sampling should be possible to detect from the width/height
> > differences of the different planes, no? Just what the components are is
> > unknown?
> 
> sub-sampling and image dimensions are stored in the code stream, so there is
> no problem getting this info.  But, how to determine RGB vs YUV ? I was going
> to use the colour box, only available for jp2 streams.

All containers I'm aware of that handle JPEG2000 (MOV/MP4 and MXF) store the colorspace, so that would be known. That doesn't help us with the component order (RGB vs BGR) though, in case that matters.
Comment 71 Aaron Boxer 2016-05-31 14:44:42 UTC
Alrighty. So, we get sampling from j2k stream, and colour space from source caps,
and we can infer the sampling. Channel order is stored in a jp2 box, but that won't be available from the containers, I am guessing.
Comment 72 Aaron Boxer 2016-05-31 14:45:05 UTC
/we get the subsampling..../
Comment 73 Aaron Boxer 2016-06-01 12:04:18 UTC
gst-plugins-good and gst-plugins-bad need to share a common header to describe the rtp sampling field, since the payloader is a good plugin and the j2k parser is a bad plugin.  Should I put this header in gst-plugins-base ? If so, where should I put it?
Comment 74 Sebastian Dröge (slomo) 2016-06-01 12:48:10 UTC
Copy that information for now :)
Comment 75 Aaron Boxer 2016-06-01 23:07:48 UTC
Created attachment 328916 [details] [review]
set sampling field in OpenJPEG encoder
Comment 76 Aaron Boxer 2016-06-01 23:09:30 UTC
Created attachment 328917 [details] [review]
Created basic parser for j2k streams

This parser is not finished, but it is currently parsing the sub sampling from the j2k stream. Would you mind please reviewing the code before I finish it up ?
Thanks!
Comment 77 Aaron Boxer 2016-06-02 02:10:12 UTC
Created attachment 328918 [details] [review]
Created basic parser for j2k streams
Comment 78 Aaron Boxer 2016-06-02 02:12:08 UTC
Note: when running this pipeline

gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink

it works but I get a gst_buffer_unmap assertion :  GST_IS_BUFFER failed
Comment 79 Sebastian Dröge (slomo) 2016-06-02 06:27:44 UTC
Review of attachment 328916 [details] [review]:

::: ext/openjpeg/gstopenjpegenc.c
@@ +689,3 @@
     g_return_val_if_reached (FALSE);
 
+  if (sampling) {

When could we ever have no sampling?

::: gst/videoparsers/gstjpeg2000sampling.h
@@ +50,3 @@
+#define GST_RTP_J2K_RGBA      "RGBA"
+#define GST_RTP_J2K_BGRA      "BGRA"
+#define GST_RTP_J2K_YBRA   	  "YCbCrA"

This should probably be YCbCrA-4:4:4:4. There is also 4:4:2:0 (GST_VIDEO_FORMAT_A420)
Comment 80 Sebastian Dröge (slomo) 2016-06-02 07:20:53 UTC
Review of attachment 328326 [details] [review]:

::: gst/rtp/gstrtpj2kcommon.h
@@ +52,3 @@
+#define GST_RTP_J2K_YBR420    "YCbCr-4:2:0"
+#define GST_RTP_J2K_YBR410    "YCbCr-4:1:0"
+#define GST_RTP_J2K_GRAYSCALE "GRAYSCALE"

These here are the standard ones, maybe in the header of the other one with YBRA we should specifically mark that one as non-standard (putting it at the bottom there, maybe having the defines contain a marker but not the string values).

::: gst/rtp/gstrtpj2kdepay.c
@@ +57,3 @@
     GST_STATIC_CAPS ("application/x-rtp, "
+        "media = (string) \"video\", " "clock-rate = (int) 90000, "
+        GST_RTP_J2K_SAMPLING_LIST "," "encoding-name = (string) \"JPEG2000\"")

This breaks ABI as it won't work with older versions anymore. Which might be OK as streams were just not standards compliant at all

Needs a second opinion but I think it's ok. If we don't require the sampling, then we can only invent a colorspace (sYUV as before) and have absolutely no idea for the sampling.

::: gst/rtp/gstrtpj2kpay.c
@@ +200,3 @@
+  guint32 num_components;
+  guint32 dx;
+  guint32 dy;

You only initialize these but never set them to anything meaningful

@@ +260,3 @@
+          guint32 len = gst_rtp_j2k_pay_header_size (data, offset);
+          GST_LOG_OBJECT (pay, "found SIZ at %u", offset);
+          state->num_components = GST_READ_UINT16_BE (data + offset + (2 + 34));

The value of num_components seems to be unused. You just set it

@@ +397,3 @@
+  state.dx = 1;                 /* no subsampling in x direction */
+  state.dy = 1;                 /* no subsamplingin y direction */
+  state.num_components = 3;     /* 3 components */

Maybe initialize this with the values from the caps?
Comment 81 Sebastian Dröge (slomo) 2016-06-02 07:33:22 UTC
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 &lt;olivier.crete@collabora.com&gt;</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
Comment 82 Aaron Boxer 2016-06-02 10:56:46 UTC
Thanks for the review.

(In reply to Sebastian Dröge (slomo) from comment #81)
> Review of attachment 328918 [details] [review] [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 &lt;olivier.crete@collabora.com&gt;</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.
Comment 83 Sebastian Dröge (slomo) 2016-06-02 11:26:47 UTC
(In reply to boxerab@gmail.com from comment #82)

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

But the parser could easily handle it to extract various information from an JPEG2000 image stream without decoding them :)

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

Unless you store data yourself somewhere, the whole buffer you have at each call is what you have available and with every call new data will be appended to the end of that buffer (unless you skip obviously, in which case data is dropped at the front). Once you call finish_frame(), this much data is removed from that buffer and on the next call you get the following data (i.e. the start of the next frame in your case).
Comment 84 Aaron Boxer 2016-06-02 14:30:17 UTC
Created attachment 328960 [details] [review]
trying to improve parser

This patch results in sigsev: spinning error. Would you mind reviewing my changes please ?
Comment 85 Aaron Boxer 2016-06-02 19:48:52 UTC
Created attachment 328998 [details] [review]
trying to improve parser

added debug output
Comment 86 Aaron Boxer 2016-06-02 20:26:21 UTC
Btw, my github branch with these changes can be found here:

https://github.com/GrokImageCompression/gst-plugins-bad/tree/j2k_sampling

in case you would like to try running the code.

Pipeline is:

gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink
Comment 87 Sebastian Dröge (slomo) 2016-06-02 20:44:21 UTC
Can you paste a backtrace of the crash you get?
Comment 88 Aaron Boxer 2016-06-02 20:56:32 UTC
well, I can't seem to get gdb to kick in. After the crash, the console offers me the option of running gdb, but when I type in gdb gst-launch-1.0, I see nothing
Comment 89 Aaron Boxer 2016-06-03 02:24:09 UTC
Created attachment 329013 [details] [review]
jpeg 2000 parser

This version displays a single video frame, then crashes. Progress!
Not sure how to debug.
Comment 90 Aaron Boxer 2016-06-03 02:24:44 UTC
Note: I have left printf statements in for debugging; will be commented out
once this is working.
Comment 91 Aaron Boxer 2016-06-03 02:26:49 UTC
Running with valgrind, I don't get the crash, and it runs, albeit slowly.
Comment 92 Sebastian Dröge (slomo) 2016-06-03 06:08:44 UTC
It's crashing in line 298 where it calls gst_caps_new_simple(), but the reason is that you corrupt the stack. You call gst_byte_reader_free() on a stack allocated byte reader, which will call free() on something on the stack. There's nothing to free/reset in a stack allocated byte reader.
Comment 93 Aaron Boxer 2016-06-03 11:20:27 UTC
thanks! working now.
Comment 94 Aaron Boxer 2016-06-03 11:56:25 UTC
Created attachment 329038 [details] [review]
jpeg 2000 parser

Working now! But, just supporting jpc at the moment. Would you mind reviewing this patch please ?
Comment 95 Sebastian Dröge (slomo) 2016-06-03 12:13:34 UTC
Probably not this week anymore, on Monday should be possible though :)

Does it already implement parsing of frames for JPC streams, including extraction of width, height and all the other things that we want in the caps? Does it passthrough any framerate given by upstream?
Comment 96 Aaron Boxer 2016-06-03 12:38:20 UTC
(In reply to Sebastian Dröge (slomo) from comment #95)
> Probably not this week anymore, on Monday should be possible though :)

Thanks, no problem, whenever you have time :)


> 
> Does it already implement parsing of frames for JPC streams, including
> extraction of width, height and all the other things that we want in the
> caps? Does it passthrough any framerate given by upstream?

Yes, everything is there, except for the frame rate. I can add that in.
Comment 97 Aaron Boxer 2016-06-03 12:55:18 UTC
Created attachment 329040 [details] [review]
jpeg 2000 parser

Pass through frame rate from upstream.
Comment 98 Sebastian Dröge (slomo) 2016-06-03 12:56:15 UTC
Review of attachment 329038 [details] [review]:

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
Comment 99 Aaron Boxer 2016-06-03 13:19:56 UTC
Created attachment 329043 [details] [review]
jpeg 2000 parser

Thanks for the quick review - much appreciated. I think I have addressed all of the issues.... have a great weekend and will discuss on Monday.
Comment 100 Aaron Boxer 2016-06-03 14:18:32 UTC
Created attachment 329046 [details] [review]
jpeg 2000 parser

reject unsupported sub-sampling for YUV stream
Comment 101 Sebastian Dröge (slomo) 2016-06-06 08:44:07 UTC
Review of attachment 329046 [details] [review]:

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

Ok, so as a next step let's consider adding the other caps :) As a patch on top of this one, a separate one.

@@ +160,3 @@
+    return GST_FLOW_ERROR;
+  gst_byte_reader_init (&reader, map.data, map.size);
+

in general you probably want to add many GST_DEBUG_OBJECT() and similar lines here so that debugging the code later is easier, so that you can know what goes on inside the element by just enabling debug logs.

@@ +207,3 @@
+  }
+  jpeg2000parse->width = width;
+  jpeg2000parse->height = height;

You update the state here, but below could again "goto beach" to request more data. The next time when you get more data, you would believe that the dimensions did not actually change

@@ +220,3 @@
+  if (numcomps > GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS) {
+    /* TODO log error */
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +231,3 @@
+
+  /* colorspace is a required field */
+  colourspace = gst_structure_get_string (s, "colorspace");

Inconsistent language: colourspace and colorspace

@@ +233,3 @@
+  colourspace = gst_structure_get_string (s, "colorspace");
+  if (!colourspace)
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +251,3 @@
+
+    if (dx != jpeg2000parse->dx[compno] || dy != jpeg2000parse->dy[compno]) {
+      subsampling_changed = TRUE;

Same here as with the dimensions

@@ +259,3 @@
+    /* we do not support sub-sampled RGB or monochrome */
+    if (strcmp (colourspace, "sYUV") && (dx > 1 || dy > 1)) {
+      /* TODO log error                */

Yes, everywhere :) Take a look at GST_ELEMENT_ERROR() and how it's used elsewhere, and use it on all errors/early returns. And add a few debug log lines in other places too please :)

@@ +260,3 @@
+    if (strcmp (colourspace, "sYUV") && (dx > 1 || dy > 1)) {
+      /* TODO log error                */
+      return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +269,3 @@
+
+    /* TODO log error */
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +279,3 @@
+      for (i = 0; i < 4; ++i) {
+        if (jpeg2000parse->dx[i] > 1 || jpeg2000parse->dy[i] > 1) {
+          return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here

@@ +355,3 @@
+
+    if (!gst_pad_set_caps (GST_BASE_PARSE_SRC_PAD (parse), caps))
+      ret = GST_FLOW_NOT_NEGOTIATED;

You probably want to get out here instead of continuing with the code below
Comment 102 Aaron Boxer 2016-06-06 12:40:32 UTC
Created attachment 329188 [details] [review]
jpeg 2000 parser

Thanks for the review. I think I've fixed all of the issues you raised. Will start working on the other formats in another patch :)
Comment 103 Aaron Boxer 2016-06-06 13:02:19 UTC
Created attachment 329191 [details] [review]
jpeg 2000 parser

A few more minor tweaks to the code.
Comment 104 Sebastian Dröge (slomo) 2016-06-06 13:17:22 UTC
Review of attachment 329191 [details] [review]:

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
Comment 105 Aaron Boxer 2016-06-06 14:58:14 UTC
Created attachment 329201 [details] [review]
jpeg 2000 parser

Thanks for the review! Here are my changes; now sampling field is not mandatory.
But, the RTPPayloader is relying on this field being present. So, we need to remove this from the payloader static sink caps, and do a runtime check, I guess.
Comment 106 Aaron Boxer 2016-06-06 14:59:57 UTC
Actually, never mind, I think this is not an issue: the negotiation will just fail if sampling is missing.
Comment 107 Aaron Boxer 2016-06-06 15:31:10 UTC
Created attachment 329202 [details] [review]
jpeg 2000 parser

removed sampling field from static source caps, since we don't always set the sampling field.
Comment 108 Sebastian Dröge (slomo) 2016-06-06 15:47:28 UTC
(In reply to boxerab@gmail.com from comment #106)
> Actually, never mind, I think this is not an issue: the negotiation will
> just fail if sampling is missing.

Exactly :)
Comment 109 Sebastian Dröge (slomo) 2016-06-07 07:24:08 UTC
Review of attachment 329202 [details] [review]:

Some more minor things :)

::: gst/videoparsers/gstjpeg2000parse.c
@@ +192,3 @@
+  if (x1 < x0 || y1 < y0) {
+    GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE,
+        ("Nonsensical image dimensions %d,%d,%d,%d", x0, y0, x1, y1), NULL);

Swap the (NULL) and the error message. This is more something to be shown to a developer (-> debug) than something for the user

@@ +214,3 @@
+    GST_ERROR_OBJECT (jpeg2000parse, "Unsupported number of components %d",
+        numcomps);
+    return GST_FLOW_NOT_NEGOTIATED;

You forget to unmap the buffer here, and maybe should be an GST_ELEMENT_ERROR()?

@@ +220,3 @@
+  s = gst_caps_get_structure (current_caps, 0);
+
+  if (!current_caps) {

The gst_caps_get_structure() above should be below the if, otherwise it will crash/assert

@@ +235,3 @@
+  gst_caps_unref (current_caps);
+
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

This should probably be compno < numcomps?

@@ +261,3 @@
+  jpeg2000parse->height = height;
+
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

And here compno < numcomps

@@ +272,3 @@
+
+  /* we do not set sampling field for sub-sampled RGB or monochrome */
+  for (compno = 0; compno < GST_JPEG2000_PARSE_MAX_SUPPORTED_COMPONENTS;

And here

@@ +352,3 @@
+          gst_caps_new_simple (gst_structure_get_name (s), "width", G_TYPE_INT,
+          jpeg2000parse->width, "height", G_TYPE_INT, jpeg2000parse->height,
+          "colorspace", G_TYPE_STRING, colorspace, NULL);

To reduce code duplication you could first create the caps without sampling, and then do a gst_caps_set_simple() just for that

@@ +356,3 @@
+
+    sink_caps =
+        gst_pad_get_current_caps (GST_BASE_PARSE_SINK_PAD (jpeg2000parse));

You already got these (and unreffed them again) as current_caps above

@@ +396,3 @@
+
+    ret = gst_base_parse_finish_frame (parse, frame, frame_size);
+    return ret;

You forget to unmap the buffer here
Comment 110 Sebastian Dröge (slomo) 2016-06-07 08:04:52 UTC
*** Bug 639231 has been marked as a duplicate of this bug. ***
Comment 111 Aaron Boxer 2016-06-07 11:03:31 UTC
> 
> @@ +396,3 @@
> +
> +    ret = gst_base_parse_finish_frame (parse, frame, frame_size);
> +    return ret;
> 
> You forget to unmap the buffer here

Thanks. I put this here intentionally, because if I try to unmap after finish_frame,
I get an assertion GST_IS_BUFFER(buffer) failed.  So, it looks like finish_frame also unmaps the buffer.
Comment 112 Aaron Boxer 2016-06-07 12:09:01 UTC
Created attachment 329262 [details] [review]
jpeg 2000 parser

The latest chapter in the jpeg 2000 parse saga.
Comment 113 Sebastian Dröge (slomo) 2016-06-07 12:09:55 UTC
Hm! That seems problematic and should be debugged :) Especially unmapping will not make the buffer invalid, and e.g. pngparse does the same: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/videoparsers/gstpngparse.c#n238

So there's something left to be done here, are you going to take a look?
Comment 114 Aaron Boxer 2016-06-07 12:15:45 UTC
Actually, it looks like my bad: I was unmapping *after* finish_frame. Will update the patch.
Comment 115 Aaron Boxer 2016-06-07 12:19:21 UTC
Created attachment 329263 [details] [review]
jpeg 2000 parser

Plugged the leak :)
Comment 116 Sebastian Dröge (slomo) 2016-06-07 12:29:37 UTC
Review of attachment 329263 [details] [review]:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +354,3 @@
+    if (sampling) {
+      GstStructure *src_caps_struct = gst_caps_get_structure (src_caps, 0);
+      gst_structure_set (src_caps_struct, "sampling", G_TYPE_STRING, sampling,

Changed this to gst_caps_set_simple() for consistency with the code below

@@ +391,3 @@
+      goto beach;
+
+    gst_buffer_unmap (frame->buffer, &map);

You leak current_caps here (fixed that while merging).
Comment 117 Sebastian Dröge (slomo) 2016-06-07 12:30:13 UTC
commit fffee978c2f850a1a08454ed27c960ff3c6f3a2c
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Wed Jun 1 19:02:33 2016 -0400

    jpeg2000parse: Add JPEG2000 parser element
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766236

commit eebb65e934eda77c9b73b1b47e113117b6157689
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Wed Jun 1 19:01:44 2016 -0400

    openjpeg: set sampling in the caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766236
Comment 118 Sebastian Dröge (slomo) 2016-06-07 12:34:06 UTC
Review of attachment 328326 [details] [review]:

This one still has some comments pending, see above
Comment 119 Sebastian Dröge (slomo) 2016-06-07 12:34:46 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=766236#c80 to be specific
Comment 120 Sebastian Dröge (slomo) 2016-06-07 12:35:11 UTC
Support for other formats (e.g. jp2) should be handled in different bugs :)
Comment 121 Aaron Boxer 2016-06-07 14:46:12 UTC
(In reply to Sebastian Dröge (slomo) from comment #117)
> commit fffee978c2f850a1a08454ed27c960ff3c6f3a2c
> Author: Aaron Boxer <boxerab@gmail.com>
> Date:   Wed Jun 1 19:02:33 2016 -0400
> 
>     jpeg2000parse: Add JPEG2000 parser element
>     
>     https://bugzilla.gnome.org/show_bug.cgi?id=766236
> 
> commit eebb65e934eda77c9b73b1b47e113117b6157689
> Author: Aaron Boxer <boxerab@gmail.com>
> Date:   Wed Jun 1 19:01:44 2016 -0400
> 
>     openjpeg: set sampling in the caps
>     
>     https://bugzilla.gnome.org/show_bug.cgi?id=766236


Hooray!!!!!!
Comment 122 Aaron Boxer 2016-06-07 15:45:26 UTC
Created attachment 329292 [details] [review]
set sampling field in payloader

Cleaned up the payloader. Now that we have a jpeg 2000 parser, the payloader does not need to do much parsing itself - it simply payloads the stream and copies the sampling field to the outgoing caps.
Comment 123 Sebastian Dröge (slomo) 2016-06-07 15:55:23 UTC
Review of attachment 329292 [details] [review]:

Almost good to go, thanks :)

::: gst/rtp/gstrtpj2kdepay.c
@@ +208,3 @@
+      "fields", G_TYPE_INT, 1,
+      "colorspace", G_TYPE_STRING, colorspace,
+      "sampling", G_TYPE_STRING, sampling, NULL);

We could also take width and height from the input caps and copy them  over, if they exist

::: gst/rtp/gstrtpj2kpay.c
@@ -150,3 @@
-  if (gst_structure_get_int (caps_structure, "width", &width)) {
-    pay->width = width;
-  }

And here the intention probably was to put these into the caps. They are optional, so if they exist we put them into the caps and otherwise not :) Seems better to do that instead of removing this code

@@ +177,3 @@
   gboolean multi_tile_part;
   gboolean bitstream;
+  guint32 next_sot;

Why?

::: gst/rtp/gstrtpj2kpay.h
@@ -43,3 @@
-
-  gint height;
-  gint width;

Storing the struct is of course not necessary anymore
Comment 124 Sebastian Dröge (slomo) 2016-06-07 16:10:18 UTC
Review of attachment 329292 [details] [review]:

::: gst/rtp/gstrtpj2kdepay.c
@@ +208,3 @@
+      "fields", G_TYPE_INT, 1,
+      "colorspace", G_TYPE_STRING, colorspace,
+      "sampling", G_TYPE_STRING, sampling, NULL);

And should probably also make use of the sampling field in the decoder... to distinguish between BGR and RGB for example.
Comment 125 Sebastian Dröge (slomo) 2016-06-07 16:16:44 UTC
Review of attachment 329292 [details] [review]:

::: gst/rtp/gstrtpj2kdepay.c
@@ +57,3 @@
     GST_STATIC_CAPS ("application/x-rtp, "
+        "media = (string) \"video\", " "clock-rate = (int) 90000, "
+        GST_RTP_J2K_SAMPLING_LIST "," "encoding-name = (string) \"JPEG2000\"")

We should keep the field optional here for backwards compat reasons... and fall back to YUV (with subsampling as parsed from the stream)
Comment 126 Sebastian Dröge (slomo) 2016-06-07 16:18:32 UTC
The parser should probably also require either the sampling field, or the colorspace field. It can reconstruct the other one from the existing one :)
Comment 127 Tim-Philipp Müller 2016-06-07 16:24:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #126)
> The parser should probably also require either the sampling field, or the
> colorspace field. It can reconstruct the other one from the existing one :)

Unless there's also in-stream signalling like with old jpeg (APPx headers)?
Comment 128 Sebastian Dröge (slomo) 2016-06-07 16:30:32 UTC
It is with JP2, but not otherwise AFAIU. Aaron?
Comment 129 Aaron Boxer 2016-06-07 18:39:57 UTC
(In reply to Sebastian Dröge (slomo) from comment #128)
> It is with JP2, but not otherwise AFAIU. Aaron?

(In reply to Sebastian Dröge (slomo) from comment #123)
> Review of attachment 329292 [details] [review] [review]:
> 
> Almost good to go, thanks :)
> 
> ::: gst/rtp/gstrtpj2kdepay.c
> @@ +208,3 @@
> +      "fields", G_TYPE_INT, 1,
> +      "colorspace", G_TYPE_STRING, colorspace,
> +      "sampling", G_TYPE_STRING, sampling, NULL);
> 
> We could also take width and height from the input caps and copy them  over,
> if they exist
> 
> ::: gst/rtp/gstrtpj2kpay.c
> @@ -150,3 @@
> -  if (gst_structure_get_int (caps_structure, "width", &width)) {
> -    pay->width = width;
> -  }
> 
> And here the intention probably was to put these into the caps. They are
> optional, so if they exist we put them into the caps and otherwise not :)
> Seems better to do that instead of removing this code
> 
> @@ +177,3 @@
>    gboolean multi_tile_part;
>    gboolean bitstream;
> +  guint32 next_sot;
> 
> Why?
> 
> ::: gst/rtp/gstrtpj2kpay.h
> @@ -43,3 @@
> -
> -  gint height;
> -  gint width;
> 
> Storing the struct is of course not necessary anymore

Thanks, working on this now.
Comment 130 Aaron Boxer 2016-06-07 18:41:28 UTC
(In reply to Sebastian Dröge (slomo) from comment #128)
> It is with JP2, but not otherwise AFAIU. Aaron?

Yes, with JP2 we can reconstruct the colour space and sampling params from the stream itself, using our new parser. For J2C and JPC, we need to know the colour space.
Comment 131 Aaron Boxer 2016-06-07 18:42:01 UTC
using our new parser, as soon as we support JP2 :)
Comment 132 Aaron Boxer 2016-06-07 18:48:22 UTC
(In reply to Sebastian Dröge (slomo) from comment #126)
> The parser should probably also require either the sampling field, or the
> colorspace field. It can reconstruct the other one from the existing one :)

How do we specify that the sink pad requires one or the other of two fields ? I thought that if a field is specified in the static caps, then it must be present upstream ?
Comment 133 Aaron Boxer 2016-06-07 18:49:07 UTC
(In reply to Sebastian Dröge (slomo) from comment #125)
> Review of attachment 329292 [details] [review] [review]:
> 
> ::: gst/rtp/gstrtpj2kdepay.c
> @@ +57,3 @@
>      GST_STATIC_CAPS ("application/x-rtp, "
> +        "media = (string) \"video\", " "clock-rate = (int) 90000, "
> +        GST_RTP_J2K_SAMPLING_LIST "," "encoding-name = (string)
> \"JPEG2000\"")
> 
> We should keep the field optional here for backwards compat reasons... and
> fall back to YUV (with subsampling as parsed from the stream)

Good idea.
Comment 134 Aaron Boxer 2016-06-07 18:53:13 UTC
(In reply to Sebastian Dröge (slomo) from comment #125)
> Review of attachment 329292 [details] [review] [review]:
> 
> ::: gst/rtp/gstrtpj2kdepay.c
> @@ +57,3 @@
>      GST_STATIC_CAPS ("application/x-rtp, "
> +        "media = (string) \"video\", " "clock-rate = (int) 90000, "
> +        GST_RTP_J2K_SAMPLING_LIST "," "encoding-name = (string)
> \"JPEG2000\"")
> 
> We should keep the field optional here for backwards compat reasons... and
> fall back to YUV (with subsampling as parsed from the stream)

Good idea.(In reply to Sebastian Dröge (slomo) from comment #124)
> Review of attachment 329292 [details] [review] [review]:
> 
> ::: gst/rtp/gstrtpj2kdepay.c
> @@ +208,3 @@
> +      "fields", G_TYPE_INT, 1,
> +      "colorspace", G_TYPE_STRING, colorspace,
> +      "sampling", G_TYPE_STRING, sampling, NULL);
> 
> And should probably also make use of the sampling field in the decoder... to
> distinguish between BGR and RGB for example.

Btw, a JP2 stream can be parsed to determine the channel order.
Comment 135 Aaron Boxer 2016-06-07 20:47:47 UTC
Created attachment 329338 [details] [review]
set sampling field in payloader

Changes: sampling is now optional in depayloader. Also, it infers the colour space from the sampling field, if not present.
Comment 136 Aaron Boxer 2016-06-07 20:49:02 UTC
Created attachment 329339 [details] [review]
set sampling field in payloader

woops, obsoleted the wrong patch.
Comment 137 Sebastian Dröge (slomo) 2016-06-08 06:59:23 UTC
(In reply to boxerab@gmail.com from comment #132)
> (In reply to Sebastian Dröge (slomo) from comment #126)
> > The parser should probably also require either the sampling field, or the
> > colorspace field. It can reconstruct the other one from the existing one :)
> 
> How do we specify that the sink pad requires one or the other of two fields
> ? I thought that if a field is specified in the static caps, then it must be
> present upstream ?

You would put two structures in there, e.g.
  image/x-jpc,colorspace=(string){...},
  image/x-jpc,sampling=(string){...}

The caps with either of the two (or both) are compatible.

(In reply to boxerab@gmail.com from comment #134)

> > And should probably also make use of the sampling field in the decoder... to
> > distinguish between BGR and RGB for example.
> 
> Btw, a JP2 stream can be parsed to determine the channel order.

Yeah, but for others we have no idea :) Our colorspace field does not give enough information to properly decode all kinds of images, while the sampling field does. For 2.0 we should standardize on that one only, but for backwards compatibility reason we need to handle both (and in case of only colorspace fall back to guessing something).
Comment 138 Sebastian Dröge (slomo) 2016-06-08 07:05:07 UTC
Review of attachment 329339 [details] [review]:

::: gst/rtp/gstrtpj2kdepay.c
@@ +205,3 @@
+    if (!colorspace) {
+      res = FALSE;
+      goto cleanup;

This means that we need to require either of the two here, but the old code just assumed sYUV if nothing was given. We should probably keep this.

And if there is no sampling in the caps, we should warn loudly so that people are not too confused if the decoded frames look funny. In 2.0 all this should go away then :)

@@ +213,3 @@
+      sampling = GST_RTP_J2K_RGB;
+    } else {
+      sampling = GST_RTP_J2K_YBR444;

I think we shouldn't set an arbitrary sampling, but rather no sampling at all (and let a parser figure that out after the depayloader if needed)

::: gst/rtp/gstrtpj2kpay.c
@@ +154,2 @@
+  if (gst_structure_get_int (caps_structure, "width", &width)) {
+    gst_caps_set_simple (caps, "width", G_TYPE_INT, width, NULL);

You should provide these to gst_rtp_base_payload_set_outcaps(). What you do now is to override the values from the input / sinkpad caps
Comment 139 Aaron Boxer 2016-06-08 14:19:57 UTC
Created attachment 329387 [details] [review]
set sampling field in payloader

1. fixed a compile issue in depayloader
2. colorspace field set to sYUV in source caps if sampling and colorspace not present in sink caps
3. corrected width and height setting in payloder source caps

Todo

1. Use sampling field in decoder to determine channel ordering
2. have jpeg 2000 parser require either sampling field or colorspace field
Comment 140 Aaron Boxer 2016-06-08 14:21:39 UTC
another todo: add warning in depayloader if sampling field not present
Comment 141 Aaron Boxer 2016-06-08 14:31:14 UTC
Created attachment 329388 [details] [review]
set sampling field in payloader

added warning in depayloader in event of missing sampling field
Comment 142 Aaron Boxer 2016-06-08 15:34:41 UTC
Created attachment 329402 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps
Comment 143 Sebastian Dröge (slomo) 2016-06-09 06:53:22 UTC
Review of attachment 329388 [details] [review]:

::: gst/rtp/gstrtpj2kdepay.c
@@ +203,3 @@
+  } else {
+    GST_ELEMENT_ERROR (depayload, STREAM, DEMUX, NULL,
+        ("Non-compliant stream: sampling field missing. Frames my appear incorrect"));

This should be a warning, not an error :) An error would (in 99% of all applications) cause the pipeline to be stopped

@@ +212,3 @@
+        sampling = GST_RTP_J2K_GRAYSCALE;
+      } else if (!strcmp (colorspace, "sRGB")) {
+        sampling = GST_RTP_J2K_RGB;

It might be BGR, let's keep it empty

::: gst/rtp/gstrtpj2kpay.c
@@ -58,3 @@
     GST_STATIC_CAPS ("application/x-rtp, "
-        "  media = (string) \"video\", "
-        "  payload = (int) " GST_RTP_PAYLOAD_DYNAMIC_STRING ", "

You removed the payload field
Comment 144 Sebastian Dröge (slomo) 2016-06-09 06:57:00 UTC
Review of attachment 329402 [details] [review]:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +46,2 @@
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("image/x-jpc," GST_RTP_J2K_SAMPLING_LIST ";" "image/x-jpc")

Shouldn't we require either of the two? You now require none

@@ +232,1 @@
   colorspace = gst_structure_get_string (current_caps_struct, "colorspace");

colorspace might be NULL now, but you don't handle that below

@@ +337,2 @@
+    if (colorspace) {
+      gst_caps_set_simple (src_caps, "colorspace", G_TYPE_STRING, colorspace,

If we require either colorspace or sampling, we can always set colorspace

@@ +341,3 @@
+
+    /* if we weren't able to determine the sampling, try to get from sink caps */
+    if (!sampling) {

Above we were only guessing the sampling for most cases. I think it would be better to get the sampling from the caps in any case, and then only sanity check that value (and warn if there is a mismatch, and for YUV correct the subsampling at least)
Comment 145 Aaron Boxer 2016-06-09 11:52:06 UTC
Thanks. Let me summarize how I see things regarding sampling:

Parser

1. j2k parser does not require either sampling or colorspace. If sampling is present, we will still work out the subsampling from the stream and do a sanity check on sampling from caps, and we will also work out the colorspace. If only colorspace is present, we will generate the sampling field (assume RGB channel order for sRGB). 

2. j2k parser source caps always sets the sampling field

Payloader

j2k rtp payloader requires the sampling field in the sink caps. i.e. we always send out RFC-compliant streams.

Depayloader

j2k rtp depayloader requires either sampling or colorspace, for legacy reasons.
An older gstreamer instance may not be setting the sampling field.


Does this make sense?
Comment 146 Sebastian Dröge (slomo) 2016-06-09 12:01:21 UTC
(In reply to boxerab@gmail.com from comment #145)
> Thanks. Let me summarize how I see things regarding sampling:
> 
> Parser
> 
> 1. j2k parser does not require either sampling or colorspace. If sampling is
> present, we will still work out the subsampling from the stream and do a
> sanity check on sampling from caps, and we will also work out the
> colorspace. If only colorspace is present, we will generate the sampling
> field (assume RGB channel order for sRGB). 

It should require one of the two, because otherwise we know nothing (unless the input is JP2 which is not supported yet). If both are given, sampling takes preference.

If sampling is given, we need to sanity check that against what is in the stream and correct it otherwise (and complain). If no sampling is given (but colorspace is), we should give a warning and guess a sampling.

The parser should always set both on its srcpad caps.

> 2. j2k parser source caps always sets the sampling field

... and colorspace :)

> Payloader
> 
> j2k rtp payloader requires the sampling field in the sink caps. i.e. we
> always send out RFC-compliant streams.

ACK

> Depayloader
> 
> j2k rtp depayloader requires either sampling or colorspace, for legacy
> reasons.
> An older gstreamer instance may not be setting the sampling field.

Yes, and sampling takes preference if both are given
Comment 147 Aaron Boxer 2016-06-09 12:49:21 UTC
Created attachment 329460 [details] [review]
set sampling field in payloader

corrections from last patch review.
Comment 148 Aaron Boxer 2016-06-09 19:59:19 UTC
Created attachment 329504 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

changes based on latest discussion and review.
Comment 149 Aaron Boxer 2016-06-09 20:11:39 UTC
Created attachment 329517 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

cleanup
Comment 150 Aaron Boxer 2016-06-09 20:13:57 UTC
Created attachment 329518 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

missed some changes from the header file
Comment 151 Aaron Boxer 2016-06-09 20:22:38 UTC
Created attachment 329519 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

fixed a compile error
Comment 152 Sebastian Dröge (slomo) 2016-06-10 07:32:37 UTC
(In reply to boxerab@gmail.com from comment #147)
> Created attachment 329460 [details] [review] [review]
> set sampling field in payloader
> 
> corrections from last patch review.

You attached the wrong patch here, it was the one from the parser :)
Comment 153 Sebastian Dröge (slomo) 2016-06-10 07:39:01 UTC
Review of attachment 329519 [details] [review]:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +301,3 @@
+        GST_WARNING_OBJECT (jpeg2000parse,
+            "Sink caps sub-sampling %d,%d for channel %d does not match stream sub-sampling %d,%d",
+            dx_caps, dy_caps, compno, dx[compno], dy[compno]);

And in this case we should take the colorspace extracted from the sampling, and the subsampling we read from the stream :) That is, set the parsed_sampling variable accordingly so it is used below for the caps

@@ +362,3 @@
+  gst_caps_set_simple (src_caps, "colorspace", G_TYPE_STRING, colorspace, NULL);
+  gst_caps_set_simple (src_caps, "sampling", G_TYPE_STRING, source_sampling,
+      NULL);

If we always set them, it can be in the same gst_caps_new_simple() call above

::: gst/videoparsers/gstjpeg2000parse.h
@@ +60,3 @@
+gboolean gst_jpeg2000_parse_is_mono (const gchar * sampling);
+void gst_jpeg2000_parse_get_subsampling (const gchar * sampling, guint8 * dx,
+    guint8 * dy);

Why are they public, could just be static, no?
Comment 154 Aaron Boxer 2016-06-10 09:59:43 UTC
Created attachment 329542 [details] [review]
set sampling field in payloader

updated to correct patch
Comment 155 Sebastian Dröge (slomo) 2016-06-10 10:10:59 UTC
Review of attachment 329542 [details] [review]:

::: gst/rtp/gstrtpj2kdepay.c
@@ +227,2 @@
+  if (outcaps)
+    gst_caps_unref (outcaps);

There seems to be no case when outcaps could be NULL
Comment 156 Aaron Boxer 2016-06-10 10:13:32 UTC
(In reply to Sebastian Dröge (slomo) from comment #155)
> Review of attachment 329542 [details] [review] [review]:
> 
> ::: gst/rtp/gstrtpj2kdepay.c
> @@ +227,2 @@
> +  if (outcaps)
> +    gst_caps_unref (outcaps);
> 
> There seems to be no case when outcaps could be NULL

Yes, that is a relic from an older change. You can simply remove that null check.
Comment 157 Sebastian Dröge (slomo) 2016-06-10 10:15:44 UTC
(In reply to boxerab@gmail.com from comment #156)
> (In reply to Sebastian Dröge (slomo) from comment #155)
> > Review of attachment 329542 [details] [review] [review] [review]:
> > 
> > ::: gst/rtp/gstrtpj2kdepay.c
> > @@ +227,2 @@
> > +  if (outcaps)
> > +    gst_caps_unref (outcaps);
> > 
> > There seems to be no case when outcaps could be NULL
> 
> Yes, that is a relic from an older change. You can simply remove that null
> check.

That's what I did when merging, also other unrelated code style changes and some other unneeded things
Comment 158 Aaron Boxer 2016-06-10 10:21:06 UTC
Created attachment 329550 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

If stream sub sampling does not match sink sampling field, then we override the sink sampling with the sink sampling's colorspace and the stream subsampling.
Comment 159 Aaron Boxer 2016-06-10 10:27:31 UTC
Created attachment 329551 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

removed C++-style comment
Comment 160 Sebastian Dröge (slomo) 2016-06-10 10:27:36 UTC
Review of attachment 329550 [details] [review]:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +29,3 @@
+/* convenience methods */
+static gboolean gst_jpeg2000_parse_is_rgb (const gchar * sampling);
+//static gboolean gst_jpeg2000_parse_is_yuv (const gchar * sampling);

No C99/C++ comments please :)

@@ +317,3 @@
+        else
+          colorspace = "sYUV";
+        sink_sampling = NULL;

We might have no colorspace field in the caps, only a (wrong) sampling field. Below we would only set parsed_sampling if colorspace is given, so would potentially end up with none

@@ -342,3 @@
-set_caps:
-
-  if (dimensions_changed || subsampling_changed) {

We should only set caps if none were set before or they changed. You now set caps on every single frame, which is a performance problem
Comment 161 Sebastian Dröge (slomo) 2016-06-10 10:30:00 UTC
Review of attachment 329551 [details] [review]:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +32,3 @@
+static gboolean gst_jpeg2000_parse_is_mono (const gchar * sampling);
+static void gst_jpeg2000_parse_get_subsampling (const gchar * sampling,
+    guint8 * dx, guint8 * dy);

You don't need the declarations here at all btw


All other comments still apply from the previous patch :)
Comment 162 Aaron Boxer 2016-06-10 10:35:46 UTC
(In reply to Sebastian Dröge (slomo) from comment #160)
> Review of attachment 329550 [details] [review] [review]:
> 
> ::: gst/videoparsers/gstjpeg2000parse.c
> @@ +29,3 @@
> +/* convenience methods */
> +static gboolean gst_jpeg2000_parse_is_rgb (const gchar * sampling);
> +//static gboolean gst_jpeg2000_parse_is_yuv (const gchar * sampling);
> 
> No C99/C++ comments please :)
> 
> @@ +317,3 @@
> +        else
> +          colorspace = "sYUV";
> +        sink_sampling = NULL;
> 
> We might have no colorspace field in the caps, only a (wrong) sampling
> field. Below we would only set parsed_sampling if colorspace is given, so
> would potentially end up with none

Yes. If the subsampling for the sampling field is wrong, I still assume that the colorspace information in the field is correct, so I set the colorspace accordingly. Are you saying that if the subsampling is wrong in the sampling field, that the colorspace information is also suspect?
Comment 163 Sebastian Dröge (slomo) 2016-06-10 10:43:39 UTC
(In reply to boxerab@gmail.com from comment #162)

> Yes. If the subsampling for the sampling field is wrong, I still assume that
> the colorspace information in the field is correct, so I set the colorspace
> accordingly. Are you saying that if the subsampling is wrong in the sampling
> field, that the colorspace information is also suspect?

No, but there might be no colorspace information in the caps. The caps might only contain the (wrong) sampling field. And all we can do then is to use the colorspace information from the wrong sampling field and just fix up the subsampling part of it
Comment 164 Aaron Boxer 2016-06-10 10:44:41 UTC
ahhh, ok, I get it.
Comment 165 Aaron Boxer 2016-06-10 11:17:26 UTC
Created attachment 329559 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

In this patch, I only set source caps if something has changed. 

Unfortunately, I now get gst_caps_set_simple: assertion GST_IS_CAPS failed  errors.
Comment 166 Aaron Boxer 2016-06-10 11:20:41 UTC
never mind, found the bug :)
Comment 167 Aaron Boxer 2016-06-10 11:32:40 UTC
Created attachment 329564 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

Now set caps only if width,height, sampling or colorspace has changed
Comment 168 Sebastian Dröge (slomo) 2016-06-10 11:41:55 UTC
Review of attachment 329564 [details] [review]:

Almost there :)

::: gst/videoparsers/gstjpeg2000parse.c
@@ +141,3 @@
 
+  jpeg2000parse->sampling = "";
+  jpeg2000parse->colorspace = "";

NULL seems like a better choice

@@ +326,3 @@
+          sampling_colorspace = "sRGB";
+        else if (gst_jpeg2000_parse_is_yuv (sink_sampling))
+          sampling_colorspace = "sYUV";

else? :)

@@ +389,3 @@
+  if (width != jpeg2000parse->width ||
+      height != jpeg2000parse->height ||
+      strcmp (jpeg2000parse->sampling, source_sampling) ||

and then here g_strcmp0() :)
Comment 169 Aaron Boxer 2016-06-10 11:59:44 UTC
Created attachment 329566 [details] [review]
jpeg2000 parse: require either sampling field or colorspace field in sink caps

Make best effort to guess colorspace when stream subsampling doesn't match sampling field.
Comment 170 Aaron Boxer 2016-06-10 12:11:58 UTC
Exactly one month and 170 comments later, I feel we are close to j2k rtp nirvana.
Comment 171 Tim-Philipp Müller 2016-06-10 12:28:08 UTC
C'mon guys, don't start flagging now, we can make it to 200!
Comment 172 Sebastian Dröge (slomo) 2016-06-10 13:25:23 UTC
Review of attachment 329566 [details] [review]:

Sorry Tim :) I'll merge this in a bit

Let's continue everything else in other bug reports
Comment 173 Aaron Boxer 2016-06-10 13:54:55 UTC
(In reply to Sebastian Dröge (slomo) from comment #172)
> Review of attachment 329566 [details] [review] [review]:
> 
> Sorry Tim :) I'll merge this in a bit
> 
> Let's continue everything else in other bug reports

(In reply to Tim-Philipp Müller from comment #171)
> C'mon guys, don't start flagging now, we can make it to 200!

:) Just wait till my next bug :)
Comment 174 Aaron Boxer 2016-06-10 13:56:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #172)
> Review of attachment 329566 [details] [review] [review]:
> 
> Sorry Tim :) I'll merge this in a bit
> 
> Let's continue everything else in other bug reports

Thanks for the mentoring on this issue.