-
Notifications
You must be signed in to change notification settings - Fork 461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for high throughput (HTJ2K) decoding. #1374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a few comments from a very quick look at the code.
You'd also want to run ./scripts/prepare-commit.sh to reformat the code with the formatting rules we use. You'll need to build with -DWITH_ASTYLE=ON
src/lib/openjp2/j2k.c
Outdated
if ((l_tccp->cblksty & 0x80U) != 0 || (l_tccp->cblksty & 0x48U) == 0x48U) { | ||
/* For HT, we only support one mode, bit 6 set, meaning that "all code-blocks | ||
within the corresponding tile-component shall be HT code-blocks, and | ||
bit 3 is reset, meaning that "No vertically causal context". */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is Vertically causal context really invalid, or just not supported ? my reading of the spec is that it is valid (would supporting it would require a lot of changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vertically casual is a valid option, but it is not supported in my code yet. I am not sure how much work is needed. I would like to revisit this issue later, if possible.
src/lib/openjp2/fbc_dec.c
Outdated
|
||
if (cblk->numbps == 1 && num_passes > 1) | ||
{ | ||
// We do not have enough precision to decode SgnProp nor MagRef passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it an implementation limitation or something wrong with the code stream ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of protection of the block decoder against calls with incorrect configuration. In normal operation cblk->numbps cannot be smaller than 1, unless something is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some where in the email I received there was a message about 3 coding passes.
HT bitstreams usually have 1, 2, or 3 passes; these are a CUP, and possibly SPP and MRP passes. HT supports more than 3 coding passes, but these may occur in transcoding from normal JPEG2000 to HT; it is totally up to the transcoder to generate them or not. If they exist, only the last CUP, and if SPP and MRP exist after it, need to be decoded; the earlier passes are placeholder passes, if anyone needs to transcode from HT back to normal JPEG2000. I have not paid a lot of attention to the case of coding passes before that last CUP. I believe there are also some flags to signal this case.
As far as I know, Kakadu can decode the case of may passes, but I am not very sure if it can generate them.
src/lib/openjp2/fbc_dec.c
Outdated
} | ||
if (cblk->numbps == 0) | ||
{ | ||
// We do not have enough precision to decode the CUP pass with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it an implementation limitation or something wrong with the code stream ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with the above. If numbps == 0 means there is not bitplanes to decode.
Thank you for your comments. Please let me know if you have further questions. |
OpenHTJ2K version 0 no thread C++14
OpenJPH Ver 0.7.3 no thread
Error. Codec doesn't create PNG file
OK Codec creates PNG file
OK Codec creates PNG file |
@Jamaika1 I do not understand what you are trying to say. Could you please give more details, and perhaps the image. |
Wait minute. In moment I will add codecs in the forum for the curious in the latest GCC 12.0.0. |
@aous72 I get a crash when decoding a HTJ2K image generated by OpenJPH master: Given https://github.com/uclouvain/openjpeg-data/blob/master/input/nonregression/Bretagne1.ppm, ojph_compress -i ~/openjpeg/openjpeg/data/input/nonregression/Bretagne1.ppm -o /tmp/ojph_bretagne1.j2k and then with your openjpeg HTJ2K capable branch: $ valgrind bin/opj_decompress -i /tmp/ojph_bretagne1.j2k -o out.ppm [INFO] Start to read j2k main header (0). |
@rouault Thank you Even. |
I found another issue, or perhaps 2, with the ds0_ht_02_b11.j2k file from https://github.com/osamu620/OpenHTJ2K/blob/main/conformance_data/ds0_ht_02_b11.j2k:
So it seems there's an issue with the EPH / SOP markers (or perhaps a more general issue with reading the coding passes that is revealed with the markers), and a potential out-of-bound read when some illegal value is found in a code block. |
…d if Mb == 0; that is, when the codeblock has no bitplanes to decode (K_max==0). Also, a bug is fixed whereby the codeblock decoder can write below the last row of the codeblock when the number of rows is odd and larger than 2.
@rouault Thank you Even. @Jamaika1 Thank you. I hope this fixes the previous issue, the crash. My apologies, it is was a bit something wrong with my code, and a bit of a misunderstanding. The EPH issue is related to my packet header decoding; I should fix it in the next commit. I need to do some research for it. Kind regards, Edit: There is potentially another bug I need to work on. In my code, the codeblock decoder can read outside the region of the codeblock bitstream, by up to 8 bytes before or after the bitstream. It is easy to fix, and I will also commit it when I fix the EPH issue. Edit2: In my work with HTJ2K, I implemented a basic packet header parser that works in cases where the image is directly generated by a tool; I was not after conformance. These files have extra layers that, by themselves, are not needed to decode the bitstream, but they have to be parsed and skipped. Achieving conformance requires some time and some research on my side. I will work on this and come back to you. |
…ng (#1) Add dummy parsing of HTJ2K CAP and CPF marker to avoid annoying warning.
I suspect this is a bug in @osamu620 Osamu-san's code, or in my understanding -- I am assuming it is OpenHTJ2K. I need to discuss this with him. The configuration is also different than the lower files. Stiles={1563,1558} has rows,columns, while I have x,y. This is why you have two tiles instead of 1.
As you noticed the color transform is not performed because the original is in YUV.
Seems fine to me. Same problem with color transform, I think.
Proper encoder should keep the color icc profile of the original image. |
@aous72 @Jamaika1
|
@osamu620 |
Thanks. But here is not a good place to raise an issue. Please bring it to OpenHTJ2K repo If this issue has a relation with OpenHTJ2K library. |
…ometime to understand the quantities and what they mean, and therefore I want to leave a version of it for future work. I will simplify the code after this.
test htj2k
htj2k-dgrok.exe -v -i image_21447_24bit-jhc.j2c -o xxx1c.ppm -H 1 |
Hi @aous72 . Did you have a chance to make some progress on the conformance test files ? |
Hi @rouault. For conformance, no progress yet; this is the next step. |
…oding in HT mode. 2. HT block decoder should not read from outside a codeblock lenght -- this required modification of data reading algorithm, which improved them. 3. Improve messaging around unreasonable or illegal conditions that may occur during block decoding; two of these conditions are better moved to t2.c.
So this would be an overhaul of opj_t2_read_packet_header() ? I thought that HT only impacted codeblock decoding, not packet header. But I've obviously not dug into the specification too deep. Hum, but trying a bit ossfuzz (https://github.com/google/oss-fuzz), on that branch, I see we have other out-of-bounds reads.
Each time a change is one in the build recipee (at https://github.com/rouault/oss-fuzz/blob/openjpeg_test_htj2k/projects/openjpeg/Dockerfile ) or in the git branches pointed to it:
It outputs
and the reproducer is in |
@aous72 |
#2) Avoids the following issue: $ valgrind bin/opj_decompress -i ~/OpenHTJ2K/conformance_data/ds0_ht_02_b11.j2k -o out.ppm -threads 0 ==4037690== Invalid read of size 1 ==4037690== at 0x48589FA: opj_t1_ht_decode_cblk (fbc_dec.c:1262) ==4037690== by 0x48B28E5: opj_t1_clbl_decode_processor (t1.c:1690) ==4037690== by 0x4854A33: opj_thread_pool_submit_job (thread.c:835) ==4037690== by 0x48B37C3: opj_t1_decode_cblks (t1.c:1943) ==4037690== by 0x48BD668: opj_tcd_t1_decode (tcd.c:2000) ==4037690== by 0x48BCADF: opj_tcd_decode_tile (tcd.c:1654) ==4037690== by 0x487D348: opj_j2k_decode_tile (j2k.c:9759) ==4037690== by 0x4881CDA: opj_j2k_decode_tiles (j2k.c:11566) ==4037690== by 0x487B333: opj_j2k_exec (j2k.c:8903) ==4037690== by 0x4882AD1: opj_j2k_decode (j2k.c:11912) ==4037690== by 0x488EF5C: opj_decode (openjpeg.c:494) ==4037690== by 0x1103AC: main (opj_decompress.c:1547) ==4037690== Address 0x52884ef is 1 bytes before a block of size 2 alloc'd ==4037690== at 0x483B723: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4037690== by 0x483E017: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4037690== by 0x48C0676: opj_realloc (opj_malloc.c:244) ==4037690== by 0x48584E6: opj_t1_ht_decode_cblk (fbc_dec.c:1123) ==4037690== by 0x48B28E5: opj_t1_clbl_decode_processor (t1.c:1690) ==4037690== by 0x4854A33: opj_thread_pool_submit_job (thread.c:835) ==4037690== by 0x48B37C3: opj_t1_decode_cblks (t1.c:1943) ==4037690== by 0x48BD668: opj_tcd_t1_decode (tcd.c:2000) ==4037690== by 0x48BCADF: opj_tcd_decode_tile (tcd.c:1654) ==4037690== by 0x487D348: opj_j2k_decode_tile (j2k.c:9759) ==4037690== by 0x4881CDA: opj_j2k_decode_tiles (j2k.c:11566) ==4037690== by 0x487B333: opj_j2k_exec (j2k.c:8903) I've also simplified a bit the allocation of the concatenated code block buffer, to remove the OPJ_COMMON_CBLK_DATA_EXTRA that I believe is a trick only needed for regular code block decoding, not HT.
…uzzing, whereby s decoded vlc codeword indicates a significant sample outside of a codeblock.
@osamu620 Thank you Osamu for the offer. I will write to you privately, unless you want my question to be like a blog for other people to have a look at. In this case, I can put them in an issue on OpenHTJ2K. Hi Even, Thank you for the fuzz test. I merged your pull request and modified it a bit to do one more check. During the fuzzing, there was an out of memory failure, but it was not in my code. The report is
and the file is Thank you. Kind regards, |
This is indeed unrelated to your changes and already happens with openjpeg master. As openjpeg instantiates a data structure for each tile, and for the "current" decoded tile, it also instanciates all tile components, bands, precincts and codeblocks data structures, that can lead to huge allocations on big images. ossfuzz runs processes with a 2 GB heap limit, and crashes when it is reached. I guess a more clever design of openjpeg could probably most of those allocations, but that seems like a huge effort, and is out of scope of this PR |
I'm happy with the PR as it, and will merge it soon. I'm preparing adding a few test cases to add to the test suite. Has anyone seen a JP2 file with HTJ2K ? or know how to generate one |
What does fbc in fbc_dec.c means by the way ? May be rename it as htj2k_dec.c ? |
I've run a performance test with that branch, on a 8192x8192 3-channel 8-bit image, lossless conversion, with 1024x1024 tiles.
so a nice improvement but still far from the 30x promise :-) |
Hi Even, Thank you for running performance tests.
Excellent.
The latest demo version of Kakadu from kakadusoftware.com can generate them. There is also my code, OpenJPH, Osamu's version OpenHTJ2K, and Aaron's version in Grok.
Feel free to rename it. It stands for fast block coder. You can call it htj2k_dec.c or even ht_dec.c, which is easier on the tongue.
To get there, you need a lot optimization. The speed improvement comes from:
Performance is usually held back by the slowest part of the chain. I do not know what that is. You can try without any wavelet decompositions, so the wavelet transform is taken out. Thanks a lot. Kind regards, |
By the way, I put the license and copyright there, but this should not be an issue; feel free to change it. |
I meant a .jp2 file with JP2 boxes, not a raw JPEG2000 codestream, which I don't think openjph and openhtj2k can generate. Perhaps grok / kakadu indeed. Thanks for your thoughts on potential perf improvements. |
Quick test htjp2 EXIF htj2k-cgrok.exe -v -i L1004432.tiff -o image_21447_24bit-grok.jp2 -H 1 -M 64 -r 1 htj2k-dgrok.exe -v -i image_21447_24bit-grok.jp2 -o xxx1a.png -H 1 |
Superseded per #1381 that adds renaming of fbc_dec.c to ht_dec.c, adds a few test cases and squash the commits |
Testing decompression of new codecs HTJ2K Testing GROK htj2k-cgrok_ojph.exe -v -i image_21447_24bit.png -o image_21447_24bit-grokjph.j2k -H 1 -M 64 -r 1 htj2k-dgrok_ojph.exe -i image_21447_24bit-grokjph.j2k -o image_21447_24bit-grokjph.ppm -H 1 htj2k-dgrok_ojph.exe -i image_21447_24bit-grokjhc.j2k -o image_21447_24bit-grokjhc.ppm -H 1 htj2k-djhc.exe -i image_21447_24bit-grokjph.j2k -o image_21447_24bit-grokjph.ppm -num_threads 1 htj2k-djph.exe -i image_21447_24bit-grokjph.j2k -o image_21447_24bit-grokjph.ppm dopenhtj2k.exe -i image_21447_24bit-grokjph.j2k -o image_21447_24bit-grokjph.ppm Testing OpenJPH htj2k-djph.exe -i image_21447_24bit-jph.j2c -o image_21447_24bit-grokjph.ppm htj2k-djhc.exe -i image_21447_24bit-jph.j2c -o image_21447_24bit-grokjph.ppm -num_threads 1 Testing OpenHTJ2K htj2k-djhc.exe -i image_21447_24bit-jhc.j2c -o image_21447_24bit-grokjhc.ppm -num_threads 1 htj2k-djph.exe -i image_21447_24bit-jhc.j2c -o image_21447_24bit-grokjhc.ppm dopenhtj2k.exe -i image_21447_24bit-jph.j2c -o image_21447_24bit-grokjph.ppm |
@Jamaika1 Thank you for putting this in.
Assuming you did things correctly, @boxerab needs to look at these, as his code does not seem to decode what it creates.
Cheers, |
Hi Aous72, I didn't think about the choice of the photo. The first is better. Do not use thread higher than one. Codecs don't work under Windows 64bit even with the latest grok patches. My omissions htj2k-cgrok_ojhc.exe -v -i image_21447_24bit.png -o image_21447_24bit-grokjhc.j2k -H 1 -M 64 -r 1 |
Hi Lucas, I ran these tests in this order
Then these
Get the latest versions of the code. Do NOT modify, and test. Cheers, |
These are the tests I ran.
Then
Get latest code. Do NOT modify, and test. Cheers, |
For me the results aren't satisfactory for Windows 10. htj2k-cjph.exe -i image_21447_24bit.ppm -o image_21447_24bit-jph.j2c -reversible true htj2k-djhc.exe -i image_21447_24bit-jph.j2c -o image_21447_24bit-jph_hc.ppm -num_threads 1 How to decode grok and openjpeg photos with the latest versions? A separate topic. |
OpenJPH only creates and decodes Part 15. Also called HTJ2K. Aous. |
In that case the descriptions are wrong in help. There is no improvement. hmm... A month ago I added the original ojph_block_decoder.cpp and ojph_block_encoder.cpp files to grok_jph. The codec worked. Currently, we can dream about decoding. OpenJPH decodes OpenHTJ2k. Success. :) |
Question to the experts. |
It is corrupt (or incorrect) in a few codeblocks. The following command ignores incorrect codeblocks The best tool we have, and it is a reference for me, is available here. Kakadu |
OpenHTJ2K currently creates only Part 15 codestreams. It can decode both Part 1 and Part 15. I agree the help message in OpenHTJ2K should be improved. |
@osamu620 Cheers, |
Hello Everyone,
This is my contribution to the OpenJPEG library. I have integrated high throughput (HTJ2K) decoding to the library. This is achieved with minimal changes to the library.
Kind regards,
Aous