bioformats icon indicating copy to clipboard operation
bioformats copied to clipboard

New codec integration Jetraw

Open arvequina opened this issue 2 years ago • 8 comments

Adding option to decompress Jetraw compressed images into bioformats.

Opened issue -> https://github.com/Jetraw/Bio-Formats/issues/4

Tested with latest stable release v6.10.1

arvequina avatar Sep 13 '22 09:09 arvequina

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/jetraw-and-omero/69250/11

imagesc-bot avatar Sep 13 '22 09:09 imagesc-bot

Hi @arvequina, thank you for contributing this PR to the project. We have introduced a Contributor License Agreement, would you be able to sign and return the form following the instructions on https://ome-contributing.readthedocs.io/en/latest/cla.html

Also do you have any suitable sample files which could be used to test this PR with? If you need a suitable upload location for sample files then we recommend using Zenodo

Currently there a test failure in the ubuntu build, it looks as though the library has not been detected:

Error:  Tests run: 1431, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 133.516 s <<< FAILURE! - in TestSuite
Error:  testJETRAW(loci.formats.utests.tiff.TiffCompressionDecompressTest)  Time elapsed: 0.015 s  <<< FAILURE!
org.testng.TestException: 

Expected exception loci.formats.FormatException but got java.lang.UnsatisfiedLinkError: /tmp/libjetraw_bioformats_plugin.so1192135793525832682: libjetraw.so: cannot open shared object file: No such file or directory
	at loci.formats.utests.tiff.TiffCompressionDecompressTest.testJETRAW(TiffCompressionDecompressTest.java:193)

dgault avatar Sep 14 '22 08:09 dgault

Also regarding the UnsatisfiedLinkError, I see that there are 3 closed sourced binaries included in the PR. These are not something that we can include directly in Bio-Formats. Our suggestion would be to keep the codec class as part of Bio-Formats but provide these libraries separately as a runtime dependency. We have used this approach with other codecs, see the LuraWaveCodec for an example (https://github.com/ome/ome-codecs/blob/master/src/main/java/ome/codecs/LuraWaveCodec.java#L156). In that example the LuraWaveService class is used to throw a DependencyException if the libraries are not present (https://github.com/ome/ome-codecs/tree/master/src/main/java/ome/codecs/services)

dgault avatar Sep 14 '22 09:09 dgault

Hi @dgault,

Thanks for your answer, all your comments are doubts that I had before creating the PR. I introduced the JNI libraries within the JAR file because then it is much easier to find them at runtime in multiple operating systems, it gets a bit tricky to search in windows, macOS and linux when there is no fix location for it, and the JAR file of formats-bsd was an excellent option for this purpose. Is it completely impossible to add them? We want to avoid the user dealing with downloading separately the libraries and placing them in the correct location. We want to have a transparent process for the user and as simply as possible.

Do you have any suggestion or alternative?

The missing library is our C++ API that needs to be installed beforehand and obviously your CI server does not have it, and I doubt that you want to install it there.

Regarding images, I can send you a compressed tif file with Jetraw.

I will check the LuraWaveCoded as you suggested.

Regarding the tests are they mandatory to have the PR accepted? Because I was checking the recent integration of the ZSTD codec and I did not find the tests. As in your testing/building server you will never have Jetraw installed I do not really see the point of adding the tests, just the one checking the exitance of the library, if it is OK with you.

Thanks again @dgault

arvequina avatar Sep 14 '22 10:09 arvequina

Im afraid embedding closed sourced binaries is just not something that will be possible. In the past we have had both codecs such as LuraWaveCodec or format readers which are using the run time dependencies, it does add an extra install step for the user but the process seems to work well from the users side.

I think you can leave the tests as they are for now. If we are going to be dealing with runtime dependencies then the new test added will be correct and should end up throwing the correct exception if the libraries are missing.

dgault avatar Sep 15 '22 11:09 dgault

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/jetraw-and-omero/69250/25

imagesc-bot avatar Sep 27 '22 08:09 imagesc-bot

Hi @dgault

Thanks for your feedback. I have changed the code so we load the JNI libraries from outside the JAR file. First from default locations and then using System.loadLibrary so it checks in the PATH or LD_LIBRARY_PATH.

Added better messages when the exceptions occur, so the user can troubleshoot better.

Regarding the testing, as your server will not have the libraries an UnsatisfiedLinkError will be thrown and the test should pass fine.

Let me know if you are OK with all the changes.

arvequina avatar Sep 27 '22 16:09 arvequina

@dgault again thanks for you feedback.

Indeed the copyright header should be changed, sorry.

Regarding images, with a simple compressed tiff image should be sufficient so far. If you need larger images or with more dimensions, please let me know and I can compress them for you.

Test image: test_image.p.zip

In order to be able to test it manually, you will need to install Jetraw in your computer and copy the JNI library of the OS you are using in the bin64 folder (windows) or the lib folder for Linux/macOS of the installation folder.

Download Jetraw: https://github.com/Jetraw/Jetraw/releases/tag/22.02.16.1

JNI libraries (jetraw_bioformats_plugin) can be found in the demo .jar file at loci/formats/codec/: formats-bsd-6.10.1.zip

You can also use directly the formats-bsd-6.10.1.jar for testing purposes as the JNI libraries are still included in there. The PR version will no longer have them and the Jetraw installer will include them so this step of copying them will not be necessary.

Let me know if there is anything not clear.

I will send you the CLA form signed later.

Thanks ;-)

arvequina avatar Sep 28 '22 15:09 arvequina

@arvequina : as @joshmoore explained in https://github.com/ome/bioformats/pull/3872#pullrequestreview-1128840996, this pull request still requires a fair amount of work before it can be considered for inclusion in Bio-Formats. I have converted it to a "draft" pull request to reflect the fact that this is not ready for review. You can still push commits and add comments as usual.

If you have any updates on your internal discussions, or no longer have time to work on this, please let us know.

melissalinkert avatar Feb 06 '23 15:02 melissalinkert

@arvequina : are you still planning to update this pull request? As noted above, the main issue that prevents us from including this is the hard-coded library paths.

If we don't hear from you, this pull request will be closed in one week. It can always be re-opened at a later date.

melissalinkert avatar Feb 27 '23 14:02 melissalinkert

Hello @melissalinkert

Yes, I know.

The problem is that removing those hard-coded paths, it will add an extra step to the installation process to our users and we would like to avoid that. We know that the libraries will always be on those paths as our installer puts them there.

Would not be possible to make just an exception for this case, please?

Thanks and we keep in touch, Marc

arvequina avatar Feb 27 '23 14:02 arvequina

Just to add my two cents as a user: I'd love to see a Fiji update site 'Jetraw' that I can enable to make my Jetraw-compressed files readable by Bio-Formats.

@arvequina why do you dislike the approach using native-lib-loader suggested by @joshmoore?


Edit: How about the other direction: saving Jetraw-prepared pixel data into .ome.tif with Jetraw compression, using a Bio-Formats writer with the ability to handle all the metadata? Is that something that would be feasible as well? I guess the license would have to be detected somehow...

imagejan avatar Feb 28 '23 13:02 imagejan

Hi @imagejan

Thanks for your contribution. We do not really want to involve an extra step such as native-lib-loader, it would do the job but we are moving the problem of having the correct dependencies to another place. We would like to have everything in our installation folder or directly in the JAR file so we do not need to worry about a third-party loader.

Regarding the alternative you are proposing the same issue with dependencies but you are adding the capability of compressing directly in Fiji. Is that what you mean?

arvequina avatar Mar 07 '23 15:03 arvequina

Hi @arvequina. Apologies but it seems like this issue has gone stale. How do you want to move forward? I think it's fair to say that the OSS community is a bit concerned about having hard-coded paths expressed here. Any other possibilities we should consider?

joshmoore avatar Nov 24 '23 17:11 joshmoore

Happy New Year, all. Having not heard back, I'm inclined to close this PR. I'm afraid that in order to balance the differing points-of-view, we may need to go back to the drawing board. (e.g., IMO the most complete solution would be to have an extensible Codecs mechanism so others can do as they please without the need to convince the core maintainers. As one of said, though, I'd argue that that is also an opportunity for data to become all but unopenable but that's likely already possible.)

joshmoore avatar Jan 15 '24 15:01 joshmoore

Hello @joshmoore and happy new year as well (a bit late ;-))

After discussing with my colleagues, we agree that this issue can be closed and if it is ok with you we will contact you by email so we can discuss this further privately.

Let us know and we are very thankful for your help and feedback.

arvequina avatar Jan 24 '24 10:01 arvequina