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

Argb support fix #9

Merged
merged 3 commits into from
Mar 17, 2020
Merged

Argb support fix #9

merged 3 commits into from
Mar 17, 2020

Conversation

Drvi
Copy link
Collaborator

@Drvi Drvi commented Mar 16, 2020

Fixes #8

In the case of BGR, ARGB, BGRA and ABGR represented with floating points we were handling the struct pixel layout twice -- once with libng by calling png_set_brg and png_set_swap_alpha and by converting the data layout to RGBA in _prepare_buffer -- these two operations sort of cancelled each other out. The fix implemented here is not to do the conversions.

cc: @johnnychen94

@Drvi Drvi mentioned this pull request Mar 16, 2020
@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #9 into master will increase coverage by 1.56%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #9      +/-   ##
=========================================
+ Coverage   64.84%   66.4%   +1.56%     
=========================================
  Files           4       4              
  Lines         256     256              
=========================================
+ Hits          166     170       +4     
+ Misses         90      86       -4
Impacted Files Coverage Δ
src/io.jl 91.41% <ø> (+2.45%) ⬆️

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 69d5cbc...465cd86. Read the comment docs.

@IanButterworth
Copy link
Member

I tried to figure out the resolver error but I'm failing.
Any ideas @johnnychen94?

@johnnychen94
Copy link
Member

Thanks for the lighting fix!

A new ColorVectorSpace release with compat fix is needed, see also JuliaGraphics/ColorVectorSpace.jl#122 and JuliaImages/ImageCore.jl#124

It succeeded in nightly because of a Pkg issue, possibly related to JuliaLang/Pkg.jl#1720

This is only a test stage issue, and if it's tested locally with a customed version of ColorVectorSpace, I think it's okay to not wait for a new version of ColorVectorSpace

@kimikage
Copy link

kimikage commented Mar 17, 2020

BTW, PNGFiles.jl uses a primitive Array as the buffer, so I doubt that we really need to use colorview or depend on ImageCore.jl.

Of course, it is intuitive that PNGFiles.jl depends on ImageCore.jl. However, we faced the dependency issue, so I think the low-level processing looks reasonable.

Edit:
Of course, my proposal above is not a solution of this issue.

@IanButterworth
Copy link
Member

I removed the Images dep during test by absorbing absmaxfinite in #10 and rebased this

@IanButterworth IanButterworth merged commit f22be62 into master Mar 17, 2020
@IanButterworth IanButterworth deleted the ARGB_support_fix branch March 17, 2020 18:36
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.

ARGB support
4 participants