GNOME Bugzilla – Bug 767512
jpeg2000parse: support j2c and jp2 file formats
Last modified: 2016-07-01 13:10:58 UTC
j2c format is j2k raw code stream preceded by a "contiguous code stream" box, while jp2 is the full jpeg 2000 file format. A part 1 compliant jp2 file allows us to unequivocally determine the sampling field for the stream.
It should also allow input without caps and do some kind of autodetection for what it gets there, on a best-effort basis for plain codestreams and properly for jp2
By "without caps" do you mean no colorspace or sampling field ?
No, I mean getting into handle_frame() with gst_pad_get_current_caps(sinkpad) returning NULL. You could then look at the bytes you get and guess what it is, at least for the JP2 case (for which something like filesrc location=test.jp2 ! jpeg2000parse ! ... should be relatively common).
(In reply to Sebastian Dröge (slomo) from comment #3) > No, I mean getting into handle_frame() with > gst_pad_get_current_caps(sinkpad) returning NULL. Under what circumstances would that happen?
Created attachment 329642 [details] [review] support j2c format
Created attachment 329643 [details] [review] support j2c format remove jp2 from caps until we actually support this format.
(In reply to boxerab@gmail.com from comment #4) > (In reply to Sebastian Dröge (slomo) from comment #3) > > No, I mean getting into handle_frame() with > > gst_pad_get_current_caps(sinkpad) returning NULL. > > Under what circumstances would that happen? ahhh, I think I see - so for filesrc plugin, no video caps are being set ?
Created attachment 329645 [details] [review] support j2c format check for "jp2c" box id in stream, to validate j2c stream
(In reply to boxerab@gmail.com from comment #7) > (In reply to boxerab@gmail.com from comment #4) > > (In reply to Sebastian Dröge (slomo) from comment #3) > > > No, I mean getting into handle_frame() with > > > gst_pad_get_current_caps(sinkpad) returning NULL. > > > > Under what circumstances would that happen? > > ahhh, I think I see - so for filesrc plugin, no video caps are being set ? Yes, or other generic sources that don't know anything about the stream
Created attachment 329646 [details] [review] support j2c format fine-tuned "jp2c" detection
Created attachment 329648 [details] [review] support j2c format one last tweak :)
Review of attachment 329648 [details] [review]: ::: gst/videoparsers/gstjpeg2000parse.c @@ +112,3 @@ + "image/x-j2c," + GST_RTP_J2K_SAMPLING_LIST ";" + "image/x-j2c, " "colorspace = (string) { sRGB, sYUV, GRAY }") And for JP2 we don't need any fields :) @@ +220,3 @@ + + current_caps = gst_pad_get_current_caps (GST_BASE_PARSE_SINK_PAD (parse)); + if (!current_caps) { Here we would have to handle autodetection later @@ +238,3 @@ + codec_format = GST_JPEG2000_PARSE_J2C; + } else if (gst_structure_has_name (current_caps_struct, "image/x-jpc")) { + codec_format = GST_JPEG2000_PARSE_JPC; It would make sense to do all the caps handling in ::set_sink_caps(), which is called whenever the sink caps are actually changing. Then you wouldn't have to parse this for every single frame @@ +253,3 @@ goto beach; } else { + if (codec_format == GST_JPEG2000_PARSE_J2C) { Might make sense to do this before the J2K parsing, so that the box is parsed and then the J2K magic is also tried to be detected afterwards @@ +283,3 @@ + if (codec_format == GST_JPEG2000_PARSE_J2C) + *skipsize -= GST_JPEG2000_PARSE_SIZE_OF_J2C_MAGIC - + GST_JPEG2000_PARSE_SIZE_OF_J2K_MAGIC; Why? Please add a comment here :)
Created attachment 329693 [details] [review] support j2c format changes from last review. I would like to save JP2 and autodetect changes for other patches - just want to add j2c support in this patch :)
> @@ +253,3 @@ > goto beach; > } else { > + if (codec_format == GST_JPEG2000_PARSE_J2C) { > > Might make sense to do this before the J2K parsing, so that the box is > parsed and then the J2K magic is also tried to be detected afterwards Looking into this now.
Created attachment 329701 [details] [review] support j2c format improved magic parsing
(In reply to boxerab@gmail.com from comment #13) > Created attachment 329693 [details] [review] [review] > support j2c format > > changes from last review. > > I would like to save JP2 and autodetect changes for other patches - just > want to add j2c support in this patch :) Sure, one step at a time and no mega-commits :) One commit should only contain one functional change.
Created attachment 329722 [details] [review] support j2c format corrected some typos
Review of attachment 329701 [details] [review]: Looks good, unless you want to change the comment below as part of this commit I'll merge it later or tomorrow :) Let me know ::: gst/videoparsers/gstjpeg2000parse.c @@ +239,3 @@ + /* check for "jp2c" */ + j2c_box_id_offset = gst_byte_reader_masked_scan_uint32 (&reader, 0xffffffff, + GST_MAKE_FOURCC ('j', 'p', '2', 'c'), 0, Isn't the box followed by the size of the whole frame? We could make use of that :)
(In reply to Sebastian Dröge (slomo) from comment #18) > Review of attachment 329701 [details] [review] [review]: > > Looks good, unless you want to change the comment below as part of this > commit I'll merge it later or tomorrow :) Let me know > > ::: gst/videoparsers/gstjpeg2000parse.c > @@ +239,3 @@ > + /* check for "jp2c" */ > + j2c_box_id_offset = gst_byte_reader_masked_scan_uint32 (&reader, > 0xffffffff, > + GST_MAKE_FOURCC ('j', 'p', '2', 'c'), 0, > > Isn't the box followed by the size of the whole frame? We could make use of > that :) Thanks. Actually, the size of the frame comes just before the fourcc.
Created attachment 329723 [details] [review] support j2c format added frame size sanity check for j2c streams
I meant not a sanity check, but we could use it to know the frame size. And then only parse all data of the frame once we have that much, instead of potentially parsing the same data over and over again until we have enough
Created attachment 329739 [details] [review] support j2c format for j2c, use parsed frame size to avoid repeated j2k marker parsing
(In reply to Sebastian Dröge (slomo) from comment #21) > I meant not a sanity check, but we could use it to know the frame size. And > then only parse all data of the frame once we have that much, instead of > potentially parsing the same data over and over again until we have enough Good idea. We could also do this with jpc - only need to look for EOC marker.
Created attachment 329740 [details] [review] support j2c format avoid unnecessary stream parsing for jpc streams
Created attachment 329741 [details] [review] support j2c format corrected a comment. I think this patch is ready :)
Review of attachment 329741 [details] [review]: ::: gst/videoparsers/gstjpeg2000parse.c @@ +160,3 @@ + jpeg2000parse->codec_format = GST_JPEG2000_PARSE_NO_CODEC; + jpeg2000parse->frame_size = 0; + jpeg2000parse->parsed_frame = FALSE; Keeping this context now has the disadvantage that you will have to reset it on DISCONT, flushing, after every frame, etc. It might be simpler to instead read the frame size on every run (which should be cheap), but to go out early if we don't have that much data yet As you can see, your code became much more complicated because of this already and that seems to be unneeded. @@ +513,3 @@ + +found_frame: + if (!jpeg2000parse->frame_size) { You might have read the frame size from the box, but we might not actually have enough data for that available yet. In which case we can't finish the frame yet but have to wait a bit more
Created attachment 329777 [details] [review] support j2c format simplified frame size handling for j2c : simply bail out if not enough data
(In reply to Sebastian Dröge (slomo) from comment #26) > Review of attachment 329741 [details] [review] [review]: > > ::: gst/videoparsers/gstjpeg2000parse.c > @@ +160,3 @@ > + jpeg2000parse->codec_format = GST_JPEG2000_PARSE_NO_CODEC; > + jpeg2000parse->frame_size = 0; > + jpeg2000parse->parsed_frame = FALSE; > > Keeping this context now has the disadvantage that you will have to reset it > on DISCONT, flushing, after every frame, etc. It might be simpler to instead > read the frame size on every run (which should be cheap), but to go out > early if we don't have that much data yet > > As you can see, your code became much more complicated because of this > already and that seems to be unneeded. > > @@ +513,3 @@ > + > +found_frame: > + if (!jpeg2000parse->frame_size) { > > You might have read the frame size from the box, but we might not actually > have enough data for that available yet. In which case we can't finish the > frame yet but have to wait a bit more Yes, I was making things way too complicated :) Fixed now.
One more fix on the way :)
Created attachment 329786 [details] [review] support j2c format fixed a few last bugs
yes, I think this is "in the can" :) Ready for review when you have time.
Created attachment 329827 [details] [review] support j2c format could not resist making one more tweak - sanity check on frame size for j2c format.
Review of attachment 329827 [details] [review]: Thanks :) ::: gst/videoparsers/gstjpeg2000parse.c @@ +246,3 @@ + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, + ("Missing contiguous code stream box for j2c stream")); + ret = GST_FLOW_NOT_NEGOTIATED; GST_FLOW_ERROR (the data is broken, it's not that we can't handle this specific one) @@ +264,3 @@ + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, + ("Corrupt contiguous code stream box for j2c stream")); + ret = GST_FLOW_NOT_NEGOTIATED; and here @@ +271,3 @@ + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, + ("Missing contiguous code stream box size for j2c stream")); + ret = GST_FLOW_NOT_NEGOTIATED; here @@ +292,3 @@ + /* J2C has 8 bytes preceding J2K magic */ + if (jpeg2000parse->codec_format == GST_JPEG2000_PARSE_J2C) + *skipsize -= GST_JPEG2000_PARSE_SIZE_OF_J2C_PREFIX_BYTES; Probably want to assert that magic_offset >= J2C_PREFIX_BYTES @@ +529,1 @@ gst_caps_unref (current_caps); Why do we still get the current caps here? You could just parse it all in set_caps() and be done with it :)
Thanks for the review. > > ::: gst/videoparsers/gstjpeg2000parse.c > @@ +246,3 @@ > + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, > + ("Missing contiguous code stream box for j2c stream")); > + ret = GST_FLOW_NOT_NEGOTIATED; > > GST_FLOW_ERROR (the data is broken, it's not that we can't handle this > specific one) > > @@ +264,3 @@ > + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, > + ("Corrupt contiguous code stream box for j2c stream")); > + ret = GST_FLOW_NOT_NEGOTIATED; > > and here > > @@ +271,3 @@ > + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, > + ("Missing contiguous code stream box size for j2c stream")); > + ret = GST_FLOW_NOT_NEGOTIATED; > > here > Yes, good idea. > @@ +292,3 @@ > + /* J2C has 8 bytes preceding J2K magic */ > + if (jpeg2000parse->codec_format == GST_JPEG2000_PARSE_J2C) > + *skipsize -= GST_JPEG2000_PARSE_SIZE_OF_J2C_PREFIX_BYTES; > > Probably want to assert that magic_offset >= J2C_PREFIX_BYTES > Good idea. > @@ +529,1 @@ > gst_caps_unref (current_caps); > > Why do we still get the current caps here? You could just parse it all in > set_caps() and be done with it :) Will try that, thanks.
> > @@ +529,1 @@ > gst_caps_unref (current_caps); > > Why do we still get the current caps here? You could just parse it all in > set_caps() and be done with it :) If I do that, I need to store the state and manage the strings for colorspace and sampling. For now, I would prefer leaving things the way they are, and getting this committed :)
Created attachment 329953 [details] [review] support j2c format Changes after latest code review.
commit 81e3b998bee7d87c55327a288a6b1e4fc2d1ffaf Author: Aaron Boxer <boxerab@gmail.com> Date: Sun Jun 12 13:53:18 2016 -0400 jpeg2000parse: support j2c format https://bugzilla.gnome.org/show_bug.cgi?id=767512
Thanks :)
Created attachment 330010 [details] [review] code clean up; use enums for sampling and color space, rather than strings. Simplified the code, and fixed a few bugs. Getting ready to add JP2 support.
Test pipeline for jp2: uridecodebin uri=file:///path/to/your/file ! imagefreeze ! videoconvert ! ximagesink
Correction: the following pipeline is good for testing: multifilesrc location=FOO.jp2 num-buffers=1 caps=image/jp2 ! openjpegdec ! imagefreeze ! videoconvert ! ximagesink
(In reply to boxerab@gmail.com from comment #39) > Created attachment 330010 [details] [review] [review] > code clean up; use enums for sampling and color space, rather than strings. > > Simplified the code, and fixed a few bugs. > Getting ready to add JP2 support. Please don't review this until I make a few more fixes :)
Created attachment 330037 [details] [review] code clean up; use enums for sampling and color space, rather than strings. OK, ready for review, when you have time. Because gstopenjpegenc.c depends on gstjpeg2000sampling.c, I had to modify the Makefile for the openjpeg plugin. Once this patch is committed, I will be ready to add JP2 parsing.
Created attachment 330038 [details] [review] code two more small fixes
Created attachment 330060 [details] [review] code clean up; use enums for sampling and color space, rather than strings. change two string arrays to static storage
Review of attachment 330060 [details] [review]: ::: ext/openjpeg/Makefile.am @@ +1,3 @@ plugin_LTLIBRARIES = libgstopenjpeg.la +libgstopenjpeg_la_SOURCES = gstopenjpegdec.c gstopenjpegenc.c ../../gst/videoparsers/gstjpeg2000sampling.c gstopenjpeg.c Why not let the openjpeg plugin link to libgstcodecparsers instead? ::: gst/videoparsers/gstjpeg2000sampling.c @@ +22,3 @@ + + +static const gchar *GstRtpJ2kSamplingStrings[GST_RTP_J2K_UNKNOWN_SAMPLING] = { You can leave the number of elements undefined @@ +56,3 @@ + case GST_RTP_J2K_NO_SAMPLING: + case GST_RTP_J2K_UNKNOWN_SAMPLING: + return 0; NULL, not 0 @@ +86,3 @@ + case GST_RTP_J2K_GRAYSCALE: + return "GRAYSCALE"; + break; Should get a default case @@ +99,3 @@ + +GstColorSpace +gst_get_color_space (const gchar * colorspace_string) g_return_val_if_fail (colorspace_string != NULL, UNKNOWN); @@ +128,3 @@ + case GST_GRAY: + return "GRAY"; + break; default case @@ +130,3 @@ + break; + } + return 0; NULL ::: gst/videoparsers/gstjpeg2000sampling.h @@ +53,3 @@ + GST_RTP_J2K_RGBA, + GST_RTP_J2K_BGRA, + GST_RTP_J2K_YBRA, YBRA was our addition, not from the standard, right? Also it should probably be YBRA4444 We should somehow mark our additions that are not in the RTP standard @@ +60,3 @@ + GST_RTP_J2K_GRAYSCALE, + GST_RTP_J2K_UNKNOWN_SAMPLING, + GST_RTP_J2K_NO_SAMPLING What's the difference between no and unknown sampling? For extensibility it would be better to put these two at the very front, you can then add new samplings in a backwards compatible way without having these two cases in the middle @@ +78,3 @@ + GST_GRAY, + GST_UNKNOWN_COLOR_SPACE, + GST_NO_COLOR_SPACE Same here about the unknown ones, and please namespace them properly. @@ +82,3 @@ + +const gchar *gst_get_color_space_string (GstColorSpace colorspace); +GstColorSpace gst_get_color_space (const gchar * colorspace_string); These should be namespaced with j2k or similar. In general the namespacing should be consistent and this should probably go into codecparsers (and later pbutils/codecutils I guess)
Please put this into a new bug report, thanks :)
There is something here that looks strange: + /* check for missing contiguous code stream box size */ + if (j2c_box_id_offset < GST_JPEG2000_PARSE_SIZE_OF_J2C_BOX_SIZE) { + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, + ("Missing contiguous code stream box size for j2c stream")); + ret = GST_FLOW_ERROR; + goto beach; + } + + /* check that we have enough bytes for the J2C box size */ + if (j2c_box_id_offset < GST_JPEG2000_PARSE_SIZE_OF_J2C_BOX_SIZE) { + *skipsize = gst_byte_reader_get_size (&reader) - num_prefix_bytes; + goto beach; + } These two checks are identical, but shouldn't be.
(In reply to Luis de Bethencourt from comment #48) > There is something here that looks strange: > > + /* check for missing contiguous code stream box size */ > + if (j2c_box_id_offset < GST_JPEG2000_PARSE_SIZE_OF_J2C_BOX_SIZE) { > + GST_ELEMENT_ERROR (jpeg2000parse, STREAM, DECODE, NULL, > + ("Missing contiguous code stream box size for j2c stream")); > + ret = GST_FLOW_ERROR; > + goto beach; > + } > + > + /* check that we have enough bytes for the J2C box size */ > + if (j2c_box_id_offset < GST_JPEG2000_PARSE_SIZE_OF_J2C_BOX_SIZE) { > + *skipsize = gst_byte_reader_get_size (&reader) - num_prefix_bytes; > + goto beach; > + } > > > These two checks are identical, but shouldn't be. Thanks. Yes, I have a patch to address this issue and a couple of other small things in the parser.
Created attachment 330647 [details] [review] improved parsing of jpc magic and j2c box
Review of attachment 330647 [details] [review]: Merged. Thanks Aaron :) commit 154698389dc8c3a7efc8813f1584eaf698af5fdc Author: Aaron Boxer <boxerab@gmail.com> Date: Thu Jun 30 07:47:50 2016 -0400 gstjpeg2000parse: improved parsing of jpc magic and j2c box https://bugzilla.gnome.org/show_bug.cgi?id=767512