GNOME Bugzilla – Bug 767402
OpenJPEG decoder: use new sampling field to determine channel order for j2c and jpc streams
Last modified: 2016-06-21 08:43:43 UTC
Currently, there is now way of knowing from a j2c or jpc stream what the channel order is. jp2 streams will store this information in the channel definition box. However, using the "sampling" field, the channel order can be determined.
This only applies to sRGB streams.
Created attachment 329628 [details] [review] use sampling field to determine RGB channel order
(In reply to boxerab@gmail.com from comment #1) > This only applies to sRGB streams. For the other cases it could use sampling instead of colorspace to know the component type. Also for the future it could potentially use the sampling for distinguishing YCbCr-X:X:X and YCrCb-X:X:X
Review of attachment 329628 [details] [review]: ::: ext/openjpeg/gstopenjpegdec.c @@ +688,3 @@ + if (self->sampling) + format = + reverse_rgb_channels (self->sampling) ? GST_VIDEO_FORMAT_BGRA : You seem to have forgotten to add this function :)
(In reply to Sebastian Dröge (slomo) from comment #4) > Review of attachment 329628 [details] [review] [review]: > > ::: ext/openjpeg/gstopenjpegdec.c > @@ +688,3 @@ > + if (self->sampling) > + format = > + reverse_rgb_channels (self->sampling) ? > GST_VIDEO_FORMAT_BGRA : > > You seem to have forgotten to add this function :) It's up there at the top. Just a small helper method :)
Created attachment 329639 [details] [review] use sampling field to determine RGB channel order now also using sampling field, if present, to determine jpeg 2000 colour space
Review of attachment 329639 [details] [review]: ::: ext/openjpeg/gstopenjpegdec.c @@ +208,3 @@ } + if ((sampling = gst_structure_get_string (s, "sampling"))) { Should we add sampling instead of colorspace to the caps? A parser can always be used. Alternatively we should warn below if sampling is not provided because everything else is guesswork then @@ +224,3 @@ } + self->sampling = gst_structure_get_string (s, "sampling"); You need to g_strdup() here. The returned string is only valid as long as you have a strong reference to the structure
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 329639 [details] [review] [review]: > > ::: ext/openjpeg/gstopenjpegdec.c > @@ +208,3 @@ > } > > + if ((sampling = gst_structure_get_string (s, "sampling"))) { > > Should we add sampling instead of colorspace to the caps? A parser can > always be used. Good idea.
Created attachment 329685 [details] [review] use sampling field to determine RGB channel order require sampling field in sink caps
Review of attachment 329685 [details] [review]: Why do you have the unrelated gstopenjpegenc.h changes in here? Also don't run gst-indent on headers, it does ugly things with headers :) ::: ext/openjpeg/gstopenjpegdec.c @@ +715,3 @@ + GST_VIDEO_FORMAT_RGBA; + else + format = GST_VIDEO_FORMAT_ARGB; How can self->sampling be NULL now? And why would it be BGRA or RGBA with sampling, but ARGB otherwise? :)
(In reply to Sebastian Dröge (slomo) from comment #10) > Review of attachment 329685 [details] [review] [review]: > > Why do you have the unrelated gstopenjpegenc.h changes in here? Also don't > run gst-indent on headers, it does ugly things with headers :) Thanks, will fix this. > > ::: ext/openjpeg/gstopenjpegdec.c > @@ +715,3 @@ > + GST_VIDEO_FORMAT_RGBA; > + else > + format = GST_VIDEO_FORMAT_ARGB; > > How can self->sampling be NULL now? Good point, let me check on this And why would it be BGRA or RGBA with > sampling, but ARGB otherwise? :) Well, ARGB is the default when we don't know the channel order. Otherwise, we can use the sampling to decide on the order.
Created attachment 329746 [details] [review] use sampling field to determine RGB channel order cleaned up patch
Created attachment 329747 [details] [review] use sampling field to determine RGB channel order self-sampling is never null.
Created attachment 329748 [details] [review] use sampling field to determine RGB channel order require sampling field for JP2 streams. This can be ensured by placing a j2k parser in front of the decoder.
Review of attachment 329748 [details] [review]: ::: ext/openjpeg/gstopenjpegdec.c @@ +86,2 @@ "image/x-jpc, " + GST_RTP_J2K_SAMPLING_LIST "; " "image/jp2, " GST_RTP_J2K_SAMPLING_LIST) For JP2 we don't really need anything, right? And adding this restriction here can only make sense (if at all) after the parser actually supports it :)
Created attachment 329815 [details] [review] use sampling field to determine RGB channel order Thanks for the review - here are the changes.
Review of attachment 329815 [details] [review]: Just one minor thing left :) ::: ext/openjpeg/gstopenjpegdec.c @@ +205,3 @@ } + self->sampling = g_strdup (gst_structure_get_string (s, "sampling")); As you g_strdup() this, you will have to free the string when needed (i.e. when shutting down and when set_format is called another time).
Thanks for the review. Changes arriving shortly.
Created attachment 329972 [details] [review] use sampling field to determine RGB channel order changes following latest review.
commit 74dcb59025615960dd114d52ca1b0fa3d08b6237 Author: Aaron Boxer <boxerab@gmail.com> Date: Mon Jun 13 22:29:39 2016 -0400 openjpegdec: use sampling field to determine RGB channel https://bugzilla.gnome.org/show_bug.cgi?id=767402