GNOME Bugzilla – Bug 766236
rtp j2k payload/depayload messes up colours in sample pattern
Last modified: 2016-06-10 13:56:16 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.
OS is XUbuntu
Somewhere the colorspace signalling got lost: rtpj2kpay0.sink: caps = image/x-jpc, colorspace=(string)sRGB, ... rtpj2kdepay0.src: caps = image/x-jpc, colorspace=(string)sYUV, ...
not sure how to trouble shoot this.....
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.
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).
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.
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?
Sorry, have no idea how to do this. Is there an example of this in another plugin?
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 :)
Thanks, I will give it a try.
Are there test patterns for the various colour spaces that need to be supported? So I can test it out?
Just videotestsrc with the default pattern is good enough :)
Created attachment 327819 [details] [review] gstrtpj2k: set colour space correctly
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.
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?
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().
OK, thanks. I need to spend some time RTFMing the gstreamer docs, then I will give this a try :)
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
Never mind, fixed this. Patch coming shortly.
Created attachment 328239 [details] [review] gstrtpj2k: set colour space and sampling fields in caps
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?
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?
Created attachment 328258 [details] [review] gstrtpj2k: set colour space and sampling fields in caps
Created attachment 328287 [details] [review] set colour space and sampling correctly Here is a small tweak to the last patch.
Created attachment 328288 [details] [review] set sampling field in OpenJPEG encoder
Created attachment 328293 [details] [review] Set sampling field in OpenJPEG encoder
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.
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.
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
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.
What do you think?
Created attachment 328322 [details] [review] Set sampling field in OpenJPEG encoder Now handling subsampling.
Created attachment 328323 [details] [review] set colour space and sampling correctly
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.
Created attachment 328326 [details] [review] set colour space and sampling correctly changed C++ style comment to C99
Created attachment 328331 [details] [review] set sampling field in OpenJPEG encoder
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?
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.
Thanks. So, for ARGB64, for example, are you saying that we should not send this over RTP, because RFC does not support this format ?
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.
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.
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.
Created attachment 328361 [details] [review] set sampling field in OpenJPEG encoder sampling should now be correct, with packed formats, alpha channel, etc.
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
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.
Created attachment 328382 [details] [review] set sampling field in openjpeg encoder
Created attachment 328383 [details] [review] set sampling field in openjpeg encoder
I think this bug has been fixed ! :)
(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.
(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 ?
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.
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)
Sounds like a good plan :)
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
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.
(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 ?
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.
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
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 ?
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 :)
(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 ?
"LGPL v2.1 or later" same as everything else please.
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.
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.
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.
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.
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.
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?
(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 ......
(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.
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.
/we get the subsampling..../
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?
Copy that information for now :)
Created attachment 328916 [details] [review] set sampling field in OpenJPEG encoder
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!
Created attachment 328918 [details] [review] Created basic parser for j2k streams
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
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)
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?
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@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
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 <olivier.crete@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.
(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).
Created attachment 328960 [details] [review] trying to improve parser This patch results in sigsev: spinning error. Would you mind reviewing my changes please ?
Created attachment 328998 [details] [review] trying to improve parser added debug output
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
Can you paste a backtrace of the crash you get?
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
Created attachment 329013 [details] [review] jpeg 2000 parser This version displays a single video frame, then crashes. Progress! Not sure how to debug.
Note: I have left printf statements in for debugging; will be commented out once this is working.
Running with valgrind, I don't get the crash, and it runs, albeit slowly.
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.
thanks! working now.
Created attachment 329038 [details] [review] jpeg 2000 parser Working now! But, just supporting jpc at the moment. Would you mind reviewing this patch please ?
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?
(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.
Created attachment 329040 [details] [review] jpeg 2000 parser Pass through frame rate from upstream.
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
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.
Created attachment 329046 [details] [review] jpeg 2000 parser reject unsupported sub-sampling for YUV stream
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
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 :)
Created attachment 329191 [details] [review] jpeg 2000 parser A few more minor tweaks to the code.
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
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.
Actually, never mind, I think this is not an issue: the negotiation will just fail if sampling is missing.
Created attachment 329202 [details] [review] jpeg 2000 parser removed sampling field from static source caps, since we don't always set the sampling field.
(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 :)
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
*** Bug 639231 has been marked as a duplicate of this bug. ***
> > @@ +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.
Created attachment 329262 [details] [review] jpeg 2000 parser The latest chapter in the jpeg 2000 parse saga.
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?
Actually, it looks like my bad: I was unmapping *after* finish_frame. Will update the patch.
Created attachment 329263 [details] [review] jpeg 2000 parser Plugged the leak :)
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).
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
Review of attachment 328326 [details] [review]: This one still has some comments pending, see above
https://bugzilla.gnome.org/show_bug.cgi?id=766236#c80 to be specific
Support for other formats (e.g. jp2) should be handled in different bugs :)
(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!!!!!!
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.
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
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.
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)
The parser should probably also require either the sampling field, or the colorspace field. It can reconstruct the other one from the existing one :)
(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)?
It is with JP2, but not otherwise AFAIU. Aaron?
(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.
(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.
using our new parser, as soon as we support JP2 :)
(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 ?
(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 #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.
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.
Created attachment 329339 [details] [review] set sampling field in payloader woops, obsoleted the wrong patch.
(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).
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
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
another todo: add warning in depayloader if sampling field not present
Created attachment 329388 [details] [review] set sampling field in payloader added warning in depayloader in event of missing sampling field
Created attachment 329402 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps
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
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)
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?
(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
Created attachment 329460 [details] [review] set sampling field in payloader corrections from last patch review.
Created attachment 329504 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps changes based on latest discussion and review.
Created attachment 329517 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps cleanup
Created attachment 329518 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps missed some changes from the header file
Created attachment 329519 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps fixed a compile error
(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 :)
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?
Created attachment 329542 [details] [review] set sampling field in payloader updated to correct patch
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
(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.
(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
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.
Created attachment 329551 [details] [review] jpeg2000 parse: require either sampling field or colorspace field in sink caps removed C++-style comment
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
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 :)
(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?
(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
ahhh, ok, I get it.
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.
never mind, found the bug :)
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
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() :)
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.
Exactly one month and 170 comments later, I feel we are close to j2k rtp nirvana.
C'mon guys, don't start flagging now, we can make it to 200!
Review of attachment 329566 [details] [review]: Sorry Tim :) I'll merge this in a bit Let's continue everything else in other bug reports
(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 :)
(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.