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 767512 - jpeg2000parse: support j2c and jp2 file formats
jpeg2000parse: support j2c and jp2 file formats
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Windows
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-10 20:23 UTC by Aaron Boxer
Modified: 2016-07-01 13:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
support j2c format (6.78 KB, patch)
2016-06-12 17:54 UTC, Aaron Boxer
none Details | Review
support j2c format (6.43 KB, patch)
2016-06-12 18:11 UTC, Aaron Boxer
none Details | Review
support j2c format (6.91 KB, patch)
2016-06-12 18:54 UTC, Aaron Boxer
none Details | Review
support j2c format (6.89 KB, patch)
2016-06-12 19:10 UTC, Aaron Boxer
none Details | Review
support j2c format (6.97 KB, patch)
2016-06-12 19:32 UTC, Aaron Boxer
none Details | Review
support j2c format (8.47 KB, patch)
2016-06-13 12:55 UTC, Aaron Boxer
none Details | Review
support j2c format (8.46 KB, patch)
2016-06-13 14:16 UTC, Aaron Boxer
reviewed Details | Review
support j2c format (8.46 KB, patch)
2016-06-13 20:34 UTC, Aaron Boxer
none Details | Review
support j2c format (9.98 KB, patch)
2016-06-13 21:02 UTC, Aaron Boxer
none Details | Review
support j2c format (12.20 KB, patch)
2016-06-14 01:59 UTC, Aaron Boxer
none Details | Review
support j2c format (12.58 KB, patch)
2016-06-14 02:14 UTC, Aaron Boxer
none Details | Review
support j2c format (12.59 KB, patch)
2016-06-14 02:18 UTC, Aaron Boxer
none Details | Review
support j2c format (12.20 KB, patch)
2016-06-14 12:38 UTC, Aaron Boxer
none Details | Review
support j2c format (12.30 KB, patch)
2016-06-14 12:58 UTC, Aaron Boxer
none Details | Review
support j2c format (12.30 KB, patch)
2016-06-14 22:28 UTC, Aaron Boxer
none Details | Review
support j2c format (12.55 KB, patch)
2016-06-17 13:05 UTC, Aaron Boxer
committed Details | Review
code clean up; use enums for sampling and color space, rather than strings. (19.90 KB, patch)
2016-06-18 23:57 UTC, Aaron Boxer
none Details | Review
code clean up; use enums for sampling and color space, rather than strings. (21.82 KB, patch)
2016-06-20 03:19 UTC, Aaron Boxer
none Details | Review
code (21.89 KB, patch)
2016-06-20 03:29 UTC, Aaron Boxer
none Details | Review
code clean up; use enums for sampling and color space, rather than strings. (21.90 KB, patch)
2016-06-20 11:11 UTC, Aaron Boxer
needs-work Details | Review
improved parsing of jpc magic and j2c box (5.74 KB, patch)
2016-06-30 12:18 UTC, Aaron Boxer
committed Details | Review

Description Aaron Boxer 2016-06-10 20:23:47 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.
Comment 1 Sebastian Dröge (slomo) 2016-06-11 09:18:15 UTC
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
Comment 2 Aaron Boxer 2016-06-12 01:29:25 UTC
By "without caps" do you mean no colorspace or sampling field ?
Comment 3 Sebastian Dröge (slomo) 2016-06-12 06:56:24 UTC
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).
Comment 4 Aaron Boxer 2016-06-12 14:11:00 UTC
(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?
Comment 5 Aaron Boxer 2016-06-12 17:54:01 UTC
Created attachment 329642 [details] [review]
support j2c format
Comment 6 Aaron Boxer 2016-06-12 18:11:42 UTC
Created attachment 329643 [details] [review]
support j2c format

remove jp2 from caps until we actually support this format.
Comment 7 Aaron Boxer 2016-06-12 18:12:58 UTC
(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 ?
Comment 8 Aaron Boxer 2016-06-12 18:54:16 UTC
Created attachment 329645 [details] [review]
support j2c format

check for "jp2c" box id in stream, to validate j2c stream
Comment 9 Sebastian Dröge (slomo) 2016-06-12 19:03:38 UTC
(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
Comment 10 Aaron Boxer 2016-06-12 19:10:13 UTC
Created attachment 329646 [details] [review]
support j2c format

fine-tuned "jp2c" detection
Comment 11 Aaron Boxer 2016-06-12 19:32:56 UTC
Created attachment 329648 [details] [review]
support j2c format

one last tweak :)
Comment 12 Sebastian Dröge (slomo) 2016-06-13 06:51:21 UTC
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 :)
Comment 13 Aaron Boxer 2016-06-13 12:55:47 UTC
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 :)
Comment 14 Aaron Boxer 2016-06-13 14:00:35 UTC
> @@ +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.
Comment 15 Aaron Boxer 2016-06-13 14:16:09 UTC
Created attachment 329701 [details] [review]
support j2c format

improved magic parsing
Comment 16 Sebastian Dröge (slomo) 2016-06-13 20:31:32 UTC
(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.
Comment 17 Aaron Boxer 2016-06-13 20:34:24 UTC
Created attachment 329722 [details] [review]
support j2c format

corrected some typos
Comment 18 Sebastian Dröge (slomo) 2016-06-13 20:34:32 UTC
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 :)
Comment 19 Aaron Boxer 2016-06-13 20:36:49 UTC
(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.
Comment 20 Aaron Boxer 2016-06-13 21:02:13 UTC
Created attachment 329723 [details] [review]
support j2c format

added frame size sanity check for j2c streams
Comment 21 Sebastian Dröge (slomo) 2016-06-13 22:02:09 UTC
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
Comment 22 Aaron Boxer 2016-06-14 01:59:05 UTC
Created attachment 329739 [details] [review]
support j2c format

for j2c, use parsed frame size to avoid repeated j2k marker parsing
Comment 23 Aaron Boxer 2016-06-14 02:00:54 UTC
(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.
Comment 24 Aaron Boxer 2016-06-14 02:14:14 UTC
Created attachment 329740 [details] [review]
support j2c format

avoid unnecessary stream parsing for jpc streams
Comment 25 Aaron Boxer 2016-06-14 02:18:25 UTC
Created attachment 329741 [details] [review]
support j2c format

corrected a comment. I think this patch is ready :)
Comment 26 Sebastian Dröge (slomo) 2016-06-14 06:30:26 UTC
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
Comment 27 Aaron Boxer 2016-06-14 12:38:46 UTC
Created attachment 329777 [details] [review]
support j2c format

simplified frame size handling for j2c : simply bail out if not enough data
Comment 28 Aaron Boxer 2016-06-14 12:39:17 UTC
(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.
Comment 29 Aaron Boxer 2016-06-14 12:42:14 UTC
One more fix on the way :)
Comment 30 Aaron Boxer 2016-06-14 12:58:56 UTC
Created attachment 329786 [details] [review]
support j2c format

fixed a few last bugs
Comment 31 Aaron Boxer 2016-06-14 13:01:41 UTC
yes, I think this is "in the can" :) Ready for review when you have time.
Comment 32 Aaron Boxer 2016-06-14 22:28:53 UTC
Created attachment 329827 [details] [review]
support j2c format

could not resist making one more tweak - sanity check on frame size for j2c format.
Comment 33 Sebastian Dröge (slomo) 2016-06-17 10:01:57 UTC
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 :)
Comment 34 Aaron Boxer 2016-06-17 12:56:53 UTC
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.
Comment 35 Aaron Boxer 2016-06-17 13:02:23 UTC
> 
> @@ +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 :)
Comment 36 Aaron Boxer 2016-06-17 13:05:39 UTC
Created attachment 329953 [details] [review]
support j2c format

Changes after latest code review.
Comment 37 Sebastian Dröge (slomo) 2016-06-17 20:09:01 UTC
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
Comment 38 Aaron Boxer 2016-06-17 20:28:54 UTC
Thanks :)
Comment 39 Aaron Boxer 2016-06-18 23:57:08 UTC
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.
Comment 40 Aaron Boxer 2016-06-19 20:24:53 UTC
Test pipeline for jp2:

uridecodebin uri=file:///path/to/your/file ! imagefreeze ! videoconvert ! ximagesink
Comment 41 Aaron Boxer 2016-06-19 22:02:47 UTC
Correction: the following pipeline is good for testing:


multifilesrc location=FOO.jp2 num-buffers=1 caps=image/jp2 !
openjpegdec ! imagefreeze ! videoconvert ! ximagesink
Comment 42 Aaron Boxer 2016-06-20 02:30:41 UTC
(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 :)
Comment 43 Aaron Boxer 2016-06-20 03:19:14 UTC
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.
Comment 44 Aaron Boxer 2016-06-20 03:29:49 UTC
Created attachment 330038 [details] [review]
code

two more small fixes
Comment 45 Aaron Boxer 2016-06-20 11:11:19 UTC
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
Comment 46 Sebastian Dröge (slomo) 2016-06-21 08:19:41 UTC
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)
Comment 47 Sebastian Dröge (slomo) 2016-06-21 08:20:20 UTC
Please put this into a new bug report, thanks :)
Comment 48 Luis de Bethencourt 2016-06-27 14:53:18 UTC
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.
Comment 49 Aaron Boxer 2016-06-28 23:39:48 UTC
(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.
Comment 50 Aaron Boxer 2016-06-30 12:18:21 UTC
Created attachment 330647 [details] [review]
improved parsing of jpc magic and j2c box
Comment 51 Luis de Bethencourt 2016-07-01 13:10:58 UTC
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