Fix a jpx inclusion tag tree bug #5727
Conversation
|
@jpambrun Please provide a PDF file that can be used to test the PR. Edit: Please make sure that you read the style guide when submitting patches. |
| @@ -1125,6 +1125,11 @@ var JpxImage = (function JpxImageClosure() { | |||
| zeroBitPlanesTree = new TagTree(width, height); | |||
| precinct.inclusionTree = inclusionTree; | |||
| precinct.zeroBitPlanesTree = zeroBitPlanesTree; | |||
| for(var l=0; l < layerNumber; l++){ | |||
| if(readBits(1)!== 0){ | |||
There was a problem hiding this comment.
Nit: these lines are missing whitespaces, it should be
for (var l = 0; l < layerNumber; l++) {
if (readBits(1) !== 0) {
|
I use your decoder in another project unrelated to PDFs. I suppose images with multiples layers are very uncommon in PDFs and code-blocks not included in layers 0 must be even more uncommon. I suppose I could try to generate a PDF with a carefully crafted JPEG2000 with there characteristics, but it would mean a lot of work. The files I have now are 16 bits signed grayscale medical images (I had to slightly modify the coder to accommodate that) so I can't just embed them in a PDF. Kakadu and Matlab have no issue reading those files. I can resubmit with correct spacing, but it's a question of whether you want to support JPEG2000 features that are not strictly PDF related. It is currently not faithful to the specification though. |
|
i think adding minor features which might not be used by a pdf, but makes decoding work according to spec are fine, as long as it does not slow down pdf rendering. |
|
PDF.js's JPEG2000 decoder is fantastic. I'm sure most of you know that the DICOM medical image format is the other big use casse of j2k. I think we will see significant shift towards web technologies in the near future in the medical domain. Both large standardisation bodies, DICOM and HL7, are already adopting RESTful apis for their next revisions and the deployement benefits of web applications are now very clear to the diagnostic imaging community. Consequently, many applications will need a cross browser j2k decoder. Your implementation appears to be the best option and I hope we will be able to contribute bug fixes as we explore use cases that are outside of the PDF scope. |
|
For me personally, I don't mind us adding reasonable amounts of code to e.g. For this particular PR, there is just one small issue, as I see it: We still have no way of confirming that it works correctly. Please note that I don't doubt that it does work, but from experience it's usually not a good idea to land unverified PRs. @jpambrun Also, please squash the commits. |
| @@ -1125,6 +1125,11 @@ var JpxImage = (function JpxImageClosure() { | |||
| zeroBitPlanesTree = new TagTree(width, height); | |||
| precinct.inclusionTree = inclusionTree; | |||
| precinct.zeroBitPlanesTree = zeroBitPlanesTree; | |||
| for (var l=0; l < layerNumber; l++) { | |||
There was a problem hiding this comment.
Nit: there's still not enough whitespace around all operators. Please add spaces before and after =.
The current code only works when every code-block includes at least some coding pass in layer 0. Otherwise, the tag tree will have x preceding 0, where x is the number of layers before the code-block is first included (See section B.10.2 and B.10.4 of ITU-T Rec. T.800). Such file appears severely corrupted as everything read beyond that point is misaligned. This commit purge the preceding zeros when the inclusionTree is constructed and throw an error if the number of preceding zeros does not match the current layer (Tag tree is invalid or the code-block was supposed to be included in a earlier packet).
|
I appreciate your need to confirm that it works correctly. I will try to produce and embed a 8bit grayscale image with this layer configuration in a PDF. However, I cannot do regression testing. I made the mistake of using Github's online code editor and had trouble squashing them. Sorry abbout that. |
|
I have created this file that shows the issue in PDF.js, but not in chrome and zathura. https://dl.dropboxusercontent.com/u/168338/j2kInclusionTreeBug.pdf |
|
I have been asked if I could share the modifications that I have made to support 16bit signed image.So I have made a fork here: https://github.com/jpambrun/jpx-medical. I'm not familiar with the etiquette and hope it's ok. I have made a demo, http://jpambrun.github.io/, of what can be achieve with your j2k decoder and I hope it can illustrate the benefits of supporting other applications. In the demo, 500 medical images are streamed as needed in 3 quality layers from a standard compliant DICOM files. The first image is displayed in under a second and the first pass of all 500 images are decoded in about 10 seconds on a typical computer. It's very fast, even compared to native apps. |
|
I've tested the PDFs from #5649, and I now think that this PR may be incomplete because some of them trigger the added "JPX Error: Invalid tag tree". |
The current code only works when every code-block includes at least some coding pass in layer 0. Otherwise, the tag tree will have x preceding 0, where x is the number of layers before the code-block is first included (See section B.10.2 and B.10.4 of ITU-T Rec. T.800). Such file appears severely corrupted as everything read beyond that point is misaligned.
This commit purge the preceding zeros when the �inclusionTree is constructed and throw an error if the number of preceding zeros does not match the current layer (Tag tree is invalid or the code-block was supposed to be included in a earlier packet).