-
Notifications
You must be signed in to change notification settings - Fork 82
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
Replaced ImageMagick by Netpbm for load/save of Netpbm files. #280
Conversation
Just a thought. Might it be worth putting this into ImageIO, beside PNGFiles and the other image loaders that will be added? That way FileIO will be told to use ImageIO for Netpbm files, and ImageIO will direct to Netpbm.jl ? There shouldn't be any noticeable overhead. It would just make it easier for users to install image readers in one go. There's even talk of adding |
That's a sensible suggestion. I'll have a closer look at ImageIO and see what I can do. |
I have submitted a PR (JuliaIO/ImageIO.jl#13) to ImageIO that handles Netpbm formats (via Netpbm.jl). If that is merged, I can modify this PR so that FileIO registers ImageIO instead of Netpbm. If somebody else does the modification, that's not a problem for me. |
I've updated the registry codeso that ImageIO is used to load and save Netpbm formats. |
Codecov Report
@@ Coverage Diff @@
## master #280 +/- ##
=======================================
Coverage 73.12% 73.12%
=======================================
Files 8 8
Lines 439 439
=======================================
Hits 321 321
Misses 118 118
Continue to review full report at Codecov.
|
src/registry.jl
Outdated
add_format(format"PBMText", b"P1", ".pbm", [:ImageMagick, LOAD]) | ||
add_format(format"PGMText", b"P2", ".pgm", [:ImageMagick, LOAD]) | ||
add_format(format"PPMText", b"P3", ".ppm", [:ImageMagick, LOAD]) | ||
add_format(format"PBMBinary", b"P4", ".pbm", [:ImageIO]) |
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.
Let's still keep ImageMagick, otherwise, it's a breaking change. Similar to other entries.
add_format(format"PBMBinary", b"P4", ".pbm", [:ImageIO]) | |
add_format(format"PBMBinary", b"P4", ".pbm", [:ImageIO], [:ImageMagick]) |
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.
The current version on master has the following entries:
P4 [:ImageMagick]
P5 [:Netpbm]
P6 [:Netpbm]
P1 [:ImageMagick, LOAD]
P2 [:ImageMagick, LOAD]
P3 [:ImageMagick, LOAD]
I can change it to
P4 [:ImageIO], [:Netpbm], [:ImageMagick]
P5 [:ImageIO], [:Netpbm]
P6 [:ImageIO], [:Netpbm]
P1 [:ImageIO], [:Netpbm], [:ImageMagick, LOAD]
P2 [:ImageIO], [:Netpbm], [:ImageMagick, LOAD]
P3 [:ImageIO], [:Netpbm], [:ImageMagick, LOAD]
Then the current options are available as fallback, and Netpbm can be used for all formats if ImageIO is not installed.
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.
I've added these changes.
Thanks @ianshmean and @johnnychen94 for your advice on getting this right.
Keeping Netpbm and ImageMagick as fallback options
I think we're good to merge now, but can you confirm with approval please @johnnychen94 |
Netpbm.jl has been extended to load and save all netpbm formats. The changes in this PR register Netpbm.jl to perform all load/save operations for these formats.
The default (i.e. without explicit format notation) save operations are the binary formats.