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

add QOI.jl to read and save the qoi image format #354

Merged
merged 2 commits into from
Jan 1, 2022
Merged

add QOI.jl to read and save the qoi image format #354

merged 2 commits into from
Jan 1, 2022

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Dec 27, 2021

Requires QOI.jl to be registered and KristofferC/QOI.jl#2 to be merged

@codecov
Copy link

codecov bot commented Dec 27, 2021

Codecov Report

Merging #354 (b7b09e3) into master (13a22ac) will decrease coverage by 0.84%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   88.34%   87.50%   -0.85%     
==========================================
  Files          10       10              
  Lines         695      696       +1     
==========================================
- Hits          614      609       -5     
- Misses         81       87       +6     
Impacted Files Coverage Δ
src/registry.jl 93.22% <ø> (ø)
src/deprecated.jl 37.50% <0.00%> (-10.42%) ⬇️
src/mimesave.jl 75.00% <0.00%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a22ac...b7b09e3. Read the comment docs.

@johnnychen94
Copy link
Member

Love this! I was planning to give it a try during this winter vacation; maybe we can also include it in ImageIO and ship it to all users of Images?

@KristofferC
Copy link
Member Author

Sounds good to me!

@KristofferC
Copy link
Member Author

@johnnychen94 This should be good to go.

@KristofferC KristofferC marked this pull request as ready for review December 30, 2021 18:37
@KristofferC
Copy link
Member Author

Or @timholy.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Because this format doesn't have magic numbers (does it?), I think we need to have some tests here to ensure we don't break it in the future. It can be as simple as

FileIO.jl/test/query.jl

Lines 463 to 472 in 13a22ac

@testset "OpenEXR detection" begin
q = query(joinpath(file_dir, "rand.exr"))
@test typeof(q) <: File{format"EXR"}
@test magic(format"EXR") == UInt8[0x76, 0x2F, 0x31, 0x01]
open(q) do io
@test position(io) == 0
skipmagic(io)
@test position(io) == 4
end
end


Curious to ask, how stable do you think the current QOI specification (not QOI.jl implementation) is? This format is quite promising but it seems to me that it's still experimental.

src/registry.jl Outdated
@@ -489,3 +489,5 @@ add_format(format"FCS", "FCS", [".fcs"], [:FCSFiles => UUID("d76558cf-badf-52d4-
add_format(format"HTML", (), [".html", ".htm"], [MimeWriter, SAVE])

add_format(format"MIDI", "MThd", [".mid", ".midi", ".MID"], [:MIDI => UUID("f57c4921-e30c-5f49-b073-3f2f2ada663e")])

add_format(format"QOI", "qoif", ".qoi", [:QOI => UUID("4b34888f-f399-49d4-9bb3-47ed5cae4e65")])
Copy link
Member

Choose a reason for hiding this comment

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

We're trying to make ImageIO a replacement for ImageMagick and make ImageIO the only interface for normal users, would you mind registering this package to ImageIO instead?

An OpenEXR example can be found in JuliaIO/ImageIO.jl#30 and #328.

Copy link
Member Author

Choose a reason for hiding this comment

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

JuliaIO/ImageIO.jl#42, so does this mean it should not be registered in FileIO? I don't immidiately see the benefit of ImageIO over just using FileIO. Should I still have https://github.com/KristofferC/QOI.jl/blob/577645ab3e5930231f4043543d1c622824ceca48/src/QOI.jl#L364-L365 in the package? Should this PR be merged in addition to the ImageIO one, or only ImageIO?

Copy link
Member

@johnnychen94 johnnychen94 Jan 1, 2022

Choose a reason for hiding this comment

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

JuliaIO/ImageIO.jl#42, so does this mean it should not be registered in FileIO? I don't immediately see the benefit of ImageIO over just using FileIO.

That's a good question. I'm not sure if my answer satisfies you: the primary goal is to provide an image-related IO toolbox as an alternative to ImageMagick so that when new users come up, we can save the efforts and tell them to ]add ImageIO instead of ]add PNGFiles QOI ....
The latter can sometimes be inconvenient if we have new formats supported. For instance, when QOI.jl is introduced in ImageIO, all ImageIO users automatically get QOI supports.

Should I still have https://github.com/KristofferC/QOI.jl/blob/577645ab3e5930231f4043543d1c622824ceca48/src/QOI.jl#L364-L365 in the package? Should this PR be merged in addition to the ImageIO one, or only ImageIO?

It depends on how you plan to use or advertise QOI.jl:

  • If you like the unified ImageIO way then there's no need to register QOI.jl here.
  • If you want people to have load("img.qoi") to work with only ]add FileIO QOI, then you can keep registering QOI.jl here.

@KristofferC
Copy link
Member Author

KristofferC commented Dec 31, 2021

It does have magic numbers (which this PR) adds. The format is done and new changes are not accepted. So the qoi format will stay as it is.

@johnnychen94 johnnychen94 mentioned this pull request Jan 1, 2022
2 tasks
@timholy
Copy link
Member

timholy commented Jan 1, 2022

If I'm thinking about this correctly, the issue of "merge here or via ImageIO" isn't something that we have to decide in this PR. If we want to make ImageIO the all-in-one solution, can't we just make QOI a dependency of ImageIO? So that anytime users install ImageIO they get QOI for free?

If that's right, registering it independently here gives people more freedom if they want a more stripped-down experience. I guess the one issue would be the advice that FileIO dispenses: maybe FileIO could recommend ImageIO first and QOI second?

@timholy
Copy link
Member

timholy commented Jan 1, 2022

@KristofferC, thanks for implementing this!

Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
@KristofferC
Copy link
Member Author

If we want to make ImageIO the all-in-one solution, can't we just make QOI a dependency of ImageIO? So that anytime users install ImageIO they get QOI for free?

I think this is done in JuliaIO/ImageIO.jl#42.

@johnnychen94, I also think it makes sense to register here so that the normal FileIO interface works as well.

@timholy timholy merged commit adc4b4e into master Jan 1, 2022
@timholy timholy deleted the kc/qoi branch January 1, 2022 11:35
@timholy
Copy link
Member

timholy commented Jan 1, 2022

OK to tag 1.12?

@KristofferC
Copy link
Member Author

Good from my p.o.v (the FileIO tests in QOI.jl pass with the master branch). #356 suggests the sixel format should be merged as well before 1.12.

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.

3 participants