Add experimental JPEG 2000 support with OpenJPEG #61

stweil · 2016-03-17T20:01:13Z

This pull request is for broader tests and further discussion.

boxerab · 2016-03-30T13:05:24Z

+1 for this PR

ahankinson · 2016-03-30T14:02:00Z

I'm wondering if OpenJPEG support should be enabled with a flag on the configure command rather than as an environment variable.

ruven · 2016-03-30T14:52:55Z

I'm wondering if OpenJPEG support should be enabled with a flag on the configure command rather than as an environment variable.

Yes, I agree. Apart from for development testing purposes, I don't really see the need to be able to switch between codecs in this way. So, it would make more sense for the choice of JPEG2000 codec to be determined at the configure stage.

ahankinson · 2016-03-30T15:09:59Z

src/OpenJPEGImage.cc


-	if(!(fsrc = fopen(filename.c_str(), "rb"))) throw string("ERROR :: OpenJPEG :: openImage() :: failed to open file for reading");	
+	filename = getFileName( currentX, currentY ); // Get file name


My C++ is rusty, but shouldn't there be a type definition string here?

Oh, sorry -- apparently this was superceded...

stweil · 2016-03-30T15:35:35Z

I consider OpenJPEG support to be experimental. If there are no Kakadu files available, the build simply produces a server working with OpenJPEG (no need to set an environment variable). If Kakadu files are available, then this must be the default for compatibility reasons and because it is still much faster, but of course it must also be possible to test OpenJPEG. That's why I added the environment variable.

Would it be better to select Kakadu / OpenJPEG via an URL option? Then it would be possible to compare both on the fly (without running two servers or restarting a single server with different environment settings)?

ruven · 2016-03-30T15:51:48Z

but of course it must also be possible to test OpenJPEG

It's much cleaner to do this by enabling (or disabling) the choice of decoder through the configure script. Sure, at the moment, Kakadu should be the default if available. If you want to test, you can always build a server with each decoder and run them in parallel, allowing you to compare them.

stweil · 2016-05-18T20:07:07Z

@ruven, which coding style do you prefer for iipsrv? I noticed that the new files for OpenJPEG seem to use a different style (indentation with tabs, function brackets at line endings and more differences) and would like to add a patch which fixes the coding style.

stweil · 2016-05-20T06:17:51Z

I rebased the PR and added three patches which fix compiler warnings.

stweil · 2016-05-20T08:23:02Z

Now a patch which formats the code was added.

calvinbutcher · 2016-05-26T12:05:17Z

+1 for this PR

stweil · 2016-05-26T18:16:51Z

@acdha, was your last emoji intentional? What is wrong with the pull request?

acdha · 2016-05-26T18:39:56Z

@stweil Sorry – I've been developing an allergy to "+1" comments but I should have stayed quiet about it

stweil · 2016-05-26T18:50:41Z

@ruven, are you planning own code for OpenJPEG support, or do you want to use the code from this PR? If the later: is there anything which should be changed before pulling? I'm asking because I have more modifications for that issue in my queue. I can add them here if this is useful.

ahankinson · 2016-05-28T14:35:57Z

Has the environment variable / compile flag issue been addressed?

stweil · 2016-05-28T15:31:10Z

The current code still supports builds which support both Kakadu and OpenJPEG, with preference for Kakadu at run time and possible selection of OpenJPEG via environment. As without my PR, Kakadu needs to be configured explicitly. OpenJPEG is compiled automatically if it is found. The advantage of the current code is that Kakadu users will get Kakadu as usual and all other users will get OpenJPEG automatically. This also ensures best code coverage for the compilation (no code rotting) without the need for multiple builds.

I have new commits in my local tree which add a configure option for OpenJPEG as well. Thus both Kakadu and OpenJPEG have to be enabled during configuration.

boxerab · 2016-05-28T17:40:45Z

Unfortunately, OpenJPEG doesn't have true region decoding, so it will always be quite slow.
True region decoding only decodes code blocks that lie within a region, plus boundary, and ignores everything else. One solution is to tile the image with relatively small tiles : 256x256 for example. But, there may be artifacts.

stweil · 2016-05-28T18:04:22Z

For whole images, the current OpenJPEG support for iipsrv is slow because of incomplete / old code (missing functions regionDecoding and old signature of OpenJPEGImage::getRegion). Depending on the image size, an improvement by a factor of 100 and more can be reached when fixing that.

These files from git@github.com:moravianlibrary/iipsrv-openjpeg.git are needed to support JPEG 2000 using libopenjpeg instead of Kakadu. Information from the original commit: commit 9262068b8606c2a0699ee74fad4ec991d34c7715 Author: Daniel Secik <dsecik@gmail.com> Date: Tue Jun 9 18:04:46 2015 +0200 Initial commit Signed-off-by: Stefan Weil <sw@weilnetz.de>

* Remove whitespace at line endings * Remove empty line at end of file Signed-off-by: Stefan Weil <sw@weilnetz.de>

See this discussion: moravianlibrary/iipsrv-openjpeg#1 Signed-off-by: Stefan Weil <sw@weilnetz.de>

* Update OpenJPEGImage::openImage for latest iipsrv. Signed-off-by: Stefan Weil <sw@weilnetz.de>

* Add check for OpenJPEG JPEG2000 library (code mainly from moravianlibrary/iipsrv-openjpeg). * Use Kakadu by default (if available). OpenJPEG can be enforced by setting the environment variable USE_OPENJPEG=1. Signed-off-by: Stefan Weil <sw@weilnetz.de>

gcc reports these warnings: src/OpenJPEGImage.cc:174:14: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for(; tmp_w > tile_width || tmp_h > tile_height; ++i){ ^ src/OpenJPEGImage.cc:174:36: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for(; tmp_w > tile_width || tmp_h > tile_height; ++i){ ^ As raster_width, raster_height, tile_width and tile_height all are unsigned values, tmp_w and tmp_h should also be unsigned. Signed-off-by: Stefan Weil <sw@weilnetz.de>

gcc reports these warnings: src/OpenJPEGImage.cc:241:7: warning: variable ‘edge_x’ set but not used [-Wunused-but-set-variable] bool edge_x = false; // Alter the tile size if it's in the last column ^ src/OpenJPEGImage.cc:247:7: warning: variable ‘edge_y’ set but not used [-Wunused-but-set-variable] bool edge_y = false; // Alter the tile size if it's in the bottom row Signed-off-by: Stefan Weil <sw@weilnetz.de>

Signed-off-by: Stefan Weil <sw@weilnetz.de>

boxerab · 2016-06-01T12:13:01Z

cool. Thanks.

calvinbutcher · 2016-06-01T13:22:02Z

Great, that's compiled now..

calvinbutcher · 2016-06-01T13:59:36Z

@stweil , @boxerab Thanks a lot, it works! I'll tidy up my docker images and they should be stable shortly. Thanks again :)

PS. Is it possible to get a tagged release of stweil/iipsrv and Grok?

ruven/iipsrv#61 (comment)

calvinbutcher · 2016-06-01T14:09:45Z

Now building and testing w/ Loris..

ruven/iipsrv#61 (comment)

stweil · 2016-06-01T14:36:26Z

@calvinbutcher, may I use your docker images on Monday next week at ELAG 2016?

calvinbutcher · 2016-06-01T15:01:06Z

@stweil By all means, though I'd keep an eye on the docker hub builds. The openjpeg builds are stable (https://hub.docker.com/r/bdlss/iipsrv-openjpeg-docker/builds/ and https://hub.docker.com/r/bdlss/loris-openjpeg-docker/builds/)

However, the iipsrv-openjpeg image relies on an untagged release of stweil/iipsrv. Only the loris-openjpeg image is stable with tagged releases of OpenJPEG and Loris.

If you can give me a tagged release of your iipsrv, I can put this in and test over the next day or so. It would simply make it more stable.

Regarding the rest, I'm still having a little teething trouble with the iipsrv-grok docker image but I hope this to be sorted very soon.

The loris-grok image: I still don't know yet if it's going to work. But again, I hope this will be sorted one way or the other in a day or two.

@boxerab a tagged release of Grok 1.0 as it stands would make these, when finished, more stable.
Please also remember that the iipsrv-grok build is relying on an untagged release of stweil/iipsrv (as mentioned above).

Sorry this reply is a bit wordy. In summary, tagged releases of Grok and stweil/iipsrv would help make these images more stable for use.

Finally, I'd steer clear of the validation as apparently version 1.0.0 is out of whack with IIIF 2.0. It fails 7 out of 21 tests.

stweil · 2016-06-01T15:04:11Z

Here is your tag: openjpeg-20160529.

calvinbutcher · 2016-06-01T15:08:16Z

Nice, thank you :) I'll put that in both openjpeg and grok builds now.. Done, awaiting builds.

calvinbutcher · 2016-06-01T15:13:35Z

@stweil just a heads up, someone here had some trouble rolling them out with vagrant. I'm using a virtualbox instance of Ubuntu 14.04 desktop. I've not had time to look into why.

calvinbutcher · 2016-06-01T15:33:24Z

OK, iipsrv-openjpeg and loris-openjpeg builds are stable.

I'm still working on the grok builds. Looks like Pillow doesn't like Grok. I just need to strip out the validation/Pillow requirement for iipsrv-grok and it'll be fine. That'll kill the loris-grok build, though.

ruven · 2016-06-01T16:26:23Z

I've done some preliminary testing with your openjpeg-work branch with iipsrv (changing the default tile size to 256 to make it comparable to the Kakadu implementation) and for an image of size 7185x6217, doing tile requests (JTL command), I get similar timing results to those reported above:

OpenJPEG: 428815
Grok: 313610
Kakadu: 112162

So, Grok is around 3x slower, while standard OpenJPEG is 4x slower than Kakadu. Where performance degrades significantly with respect to Kakadu is with larger images. However, for a larger image of size 15016x11741, I get:

OpenJPEG: Crash!
Grok: 2845229
Kakadu: 124437

So, standard OpenJPEG is unusable, whereas Grok (I guess because of the region decoding that @boxerab added) works, but is around 20x slower than Kakadu, which has a similar decode time to smaller images. Note that these are the decode times for the core process() command in the OpenJPEGImage and KakaduImage classes in iipsrv.

And to put all this into perspective, the same 15016x11741 image in TIFF format has a tile decode time of only 2445, so even Kakadu is significantly slower than TIFF by about a factor of 50!

stweil · 2016-06-01T17:30:42Z

OpenJPEG has problems with large images, see for example uclouvain/openjpeg#730. Can I get your test image somewhere?

boxerab · 2016-06-01T17:43:19Z

@ruven thanks for testing. For large images, what do you think is a good target speed ratio relative to Kakaudu? Something greater than 1 :) @calvinbutcher I will create a tag. @stweil Ruven's test image is only 165 MB, so this is below the 1GB cutoff. I did do a fair bit of memory optimization in Grok, so it does use quite a bit less memory than OpenJPEG.

stweil · 2016-06-01T18:47:57Z

Ruven's test image is only 165 MB, so this is below the 1GB cutoff.

That's why I am interested to know what caused the crash (and to fix it, of course).

calvinbutcher · 2016-06-02T10:23:35Z

@boxerab Thanks. If they're going to be used at ELAG, this will make the docker images more stable.

ruven · 2016-06-02T12:24:30Z

Can I get your test image somewhere?

This particular one, no. But there's nothing special about it. It was converted from tiff using Kakadu with parameters including a precinct size of 256x256, a code block size of 64x64 and RPCL ordering.

That's why I am interested to know what caused the crash (and to fix it, of course).

In fact it was more a memory allocation problem than a seg fault type crash. I guess OpenJPEG tried to put the whole image into memory rather than just a region.

calvinbutcher · 2016-06-03T11:32:48Z

@stweil
https://github.com/bodleian/loris-openjpeg-docker
https://github.com/bodleian/iipsrv-openjpeg-docker
https://github.com/bodleian/iipsrv-grok-docker

The above are now stable with tagged releases of stweil/iipsrv and grok, so they should be reliable. Am still working on the loris-grok build but may be fixed by the end of play today.

boxerab · 2016-06-04T16:32:43Z

@stweil this image will give you similar characteristics to Ruven's test image. And, if you decode a small region and compare Kakadu to the open source codecs, Kakadu is truly orders of magnitude faster. So, there is still quite a bit of catching up to do.

boxerab · 2016-06-05T01:07:18Z

By the way, folks, I just discovered a bug in Grok's region decoding : the codec was performing the discrete wavelet transform for the entire image, rather than only for the region. This makes a big difference for large images. So, the latest version on master is now significantly faster when decoding a relatively small region inside a large single-tile image.

boxerab · 2016-07-08T00:58:33Z

Hi Ruven,

Do you think this PR is ready for merging, or is there more work to be done?
Just curious.

I think once it is merged in, we can get people testing it, and this will help identify areas for
improvement, mostly related to higher performance.

Thanks,
Aaron

ruven · 2016-07-08T10:02:54Z

I'm happy to merge this, but if I understood @stweil correctly, this PR is not in fact the latest version:

@ruven: The latest version (not the one in the PR) needs ./configure --enable-openjpeg. The test was made with two different iipsrv.fcgi files – no environment settings involved.

@stweil, are you ready to update or do a new pull request?

stweil · 2016-07-09T13:01:37Z

@ruven, my latest code is now in PR #74. It adds the --enable-openjpeg option for configure and region decoding for full images. The new code is nearly identical to my latest tagged version openjpeg-20160529, but uses the default tile size of 256 and disabled debug code.

boxerab · 2016-07-20T12:41:04Z

I guess this PR could be closed now ?

boxerab · 2016-07-20T12:42:44Z

@calvinbutcher hope you are enjoying the sun and cider. I just fixed a serious concurrency issue in Grok codec; should I move the tag so that docker image gets the bug-fixed version?

stweil · 2016-07-20T14:56:07Z

Closing this PR. It is superseded by PR #74 (which will hopefully be merged soon).

This was referenced Mar 17, 2016

Free jpeg2000 libraries? #14

Closed

Code needs update for newer OpenJPEG moravianlibrary/iipsrv-openjpeg#1

Open

stweil force-pushed the pr-openjpeg branch from c4e6a34 to d959f43 Compare March 18, 2016 13:08

ahankinson reviewed Mar 30, 2016
View reviewed changes

dsecik mentioned this pull request May 10, 2016

iipsrv-openjpeg w/ openjpeg 2.0 on Ubuntu headless VM 14.04.3 moravianlibrary/iipsrv-openjpeg#2

Open

stweil force-pushed the pr-openjpeg branch from d959f43 to 1266daf Compare May 20, 2016 06:15

stweil added 8 commits May 28, 2016 20:13

openjpeg: Fix some whitespace issues

f5c12a7

* Remove whitespace at line endings * Remove empty line at end of file Signed-off-by: Stefan Weil <sw@weilnetz.de>

openjpeg: Add copyright and license comment

c6dec5b

See this discussion: moravianlibrary/iipsrv-openjpeg#1 Signed-off-by: Stefan Weil <sw@weilnetz.de>

openjpeg: Update code for OpenJPEG support

f4eb3fc

* Update OpenJPEGImage::openImage for latest iipsrv. Signed-off-by: Stefan Weil <sw@weilnetz.de>

Enable OpenJPEG support

4dd95b3

* Add check for OpenJPEG JPEG2000 library (code mainly from moravianlibrary/iipsrv-openjpeg). * Use Kakadu by default (if available). OpenJPEG can be enforced by setting the environment variable USE_OPENJPEG=1. Signed-off-by: Stefan Weil <sw@weilnetz.de>

openjpeg: Fix some compiler warnings (-Wunused-parameter)

4c9e946

Signed-off-by: Stefan Weil <sw@weilnetz.de>

calvinbutcher added a commit to bodleian/iipsrv-grok-docker that referenced this pull request Jun 1, 2016

updated compiles

e2464e5

ruven/iipsrv#61 (comment)

calvinbutcher added a commit to bodleian/loris-grok-docker that referenced this pull request Jun 1, 2016

updated compiles

13e5e38

ruven/iipsrv#61 (comment)

stweil mentioned this pull request Jul 9, 2016

Add JPEG 2000 support with OpenJPEG #74

Merged

stweil closed this Jul 20, 2016

stweil deleted the pr-openjpeg branch September 14, 2017 10:09


		if(!(fsrc = fopen(filename.c_str(), "rb"))) throw string("ERROR :: OpenJPEG :: openImage() :: failed to open file for reading");
		filename = getFileName( currentX, currentY ); // Get file name

Add experimental JPEG 2000 support with OpenJPEG #61

Add experimental JPEG 2000 support with OpenJPEG #61

Conversation

stweil commented Mar 17, 2016

boxerab commented Mar 30, 2016

ahankinson commented Mar 30, 2016

ruven commented Mar 30, 2016

ahankinson Mar 30, 2016

Choose a reason for hiding this comment

ahankinson Mar 30, 2016

Choose a reason for hiding this comment

stweil commented Mar 30, 2016

ruven commented Mar 30, 2016

stweil commented May 18, 2016

stweil commented May 20, 2016

stweil commented May 20, 2016

calvinbutcher commented May 26, 2016

stweil commented May 26, 2016 • edited Loading

acdha commented May 26, 2016

stweil commented May 26, 2016 • edited Loading

ahankinson commented May 28, 2016

stweil commented May 28, 2016

boxerab commented May 28, 2016

stweil commented May 28, 2016 • edited Loading

boxerab commented Jun 1, 2016

calvinbutcher commented Jun 1, 2016

calvinbutcher commented Jun 1, 2016 • edited Loading

calvinbutcher commented Jun 1, 2016

stweil commented Jun 1, 2016

calvinbutcher commented Jun 1, 2016 • edited Loading

stweil commented Jun 1, 2016

calvinbutcher commented Jun 1, 2016 • edited Loading

calvinbutcher commented Jun 1, 2016

calvinbutcher commented Jun 1, 2016

ruven commented Jun 1, 2016

stweil commented Jun 1, 2016

boxerab commented Jun 1, 2016

stweil commented Jun 1, 2016 • edited Loading

calvinbutcher commented Jun 2, 2016 • edited Loading

ruven commented Jun 2, 2016

calvinbutcher commented Jun 3, 2016

boxerab commented Jun 4, 2016

boxerab commented Jun 5, 2016 • edited Loading

boxerab commented Jul 8, 2016

ruven commented Jul 8, 2016

stweil commented Jul 9, 2016

boxerab commented Jul 20, 2016

boxerab commented Jul 20, 2016

stweil commented Jul 20, 2016

stweil commented May 26, 2016 •

edited

Loading

stweil commented May 26, 2016 •

edited

Loading

stweil commented May 28, 2016 •

edited

Loading

calvinbutcher commented Jun 1, 2016 •

edited

Loading

calvinbutcher commented Jun 1, 2016 •

edited

Loading

calvinbutcher commented Jun 1, 2016 •

edited

Loading

stweil commented Jun 1, 2016 •

edited

Loading

calvinbutcher commented Jun 2, 2016 •

edited

Loading

boxerab commented Jun 5, 2016 •

edited

Loading