Skip to content
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

Revert RunTimeException when JPEGTurbo cannot be loaded #4115

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

dgault
Copy link
Member

@dgault dgault commented Nov 15, 2023

Reverts the changes from #4090
Fixes #4109

To test you should attempt to instance ImageReader reader = new ImageReader(); while using Apple Silicon. This should throw a RunTimeException even if the desired reader does not require JPEGTurbo.

I opted to revert the change rather than catch the exception in ImageReader as this would negate the original reason for adding the exception.

This reverts commit 9595b6c, reversing
changes made to f9f4617.
@dgault dgault added this to the 7.1.0 milestone Nov 15, 2023
@dgault dgault changed the title Revert "Merge pull request #4090 from CGDogan/patch-5" Revert RunTimeException when JPEGTurbo cannot be loaded Nov 15, 2023
@melissalinkert
Copy link
Member

I'm completely fine with this approach for 7.1.0, but for a future release would it be worth something like:

  • set libraryLoaded as in Throw error when cannot load library #4090, to accurately represent the library loading state
  • add a getter for libraryLoaded, so that individual readers can query it
  • update initFile in NDPIReader (and other readers that use JPEGTurboService) to call the new getter and throw an exception if the library was not loaded

That would be in the spirit of #4090 (fail before openBytes if data can't be read), but limited to the case where unreadable files are actually being initialized.

@dgault
Copy link
Member Author

dgault commented Nov 16, 2023

Yeah, that seems like a good approach that would meet the demands of the initial PR but without failing unnecessarily. I will look at getting it implemented as part of this PR.

@sbesson sbesson requested a review from joshmoore November 20, 2023 14:26
@joshmoore
Copy link
Member

joshmoore commented Nov 20, 2023

Testing on my arm64 machine:

  • Without this PR I see the TurboJPEG runtime exception. 👍
  • With this PR, I do not. 👍

Testing required running:

bioformats/artifacts $zip -d bioformats_package.jar META-INF/lib/osx_64/libturbojpeg.dylib

after each build, even though I had deleted turbojpeg:

(mvn) /opt/own-bf/bio-formats-build/bioformats/tools $git status
On branch JPegTurbo-RevertRTE
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   ../ant/toplevel.xml
	deleted:    ../components/forks/turbojpeg/LICENSE.txt
	deleted:    ../components/forks/turbojpeg/build.properties
	deleted:    ../components/forks/turbojpeg/build.xml
	deleted:    ../components/forks/turbojpeg/lib/linux_32/libturbojpeg.so
	deleted:    ../components/forks/turbojpeg/lib/linux_64/libturbojpeg.so
	deleted:    ../components/forks/turbojpeg/lib/osx_32/libturbojpeg.dylib
	deleted:    ../components/forks/turbojpeg/lib/osx_64/libturbojpeg.dylib
	deleted:    ../components/forks/turbojpeg/lib/osx_ppc/libturbojpeg.dylib
	deleted:    ../components/forks/turbojpeg/lib/windows_32/turbojpeg.dll
	deleted:    ../components/forks/turbojpeg/lib/windows_64/turbojpeg.dll
	deleted:    ../components/forks/turbojpeg/pom.xml
	deleted:    ../components/forks/turbojpeg/src/TJExample.java
	deleted:    ../components/forks/turbojpeg/src/TJUnitTest.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJ.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJCompressor.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJCustomFilter.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJDecompressor.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJLoader.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJScalingFactor.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJTransform.java
	deleted:    ../components/forks/turbojpeg/src/org/libjpegturbo/turbojpeg/TJTransformer.java
	modified:   ../pom.xml

🤷🏽

Copy link
Member

@melissalinkert melissalinkert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look reasonable to me, with the caveat that I am not set up to actually test functionality.

@melissalinkert melissalinkert merged commit ec22ea2 into ome:develop Nov 27, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#4090 breaks support for Apple Silicon for formats that don't need TurboJPEG
3 participants