Skip to content

Fix #5302: Proper alpha-blending for jpeg #5324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 28, 2015
Merged

Fix #5302: Proper alpha-blending for jpeg #5324

merged 5 commits into from
Oct 28, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Oct 26, 2015

Saving a JPEG with an alpha channel doesn't look right -- I think pillow just removes the alpha channel altogether and uses the rest of the RGB values as-is.

This blends RGBA images onto a white background before saving.

This is JPEG-specific since it's our only raster output file format that doesn't support alpha blending. If we ever grow another (unlikely) this will need to be duplicated there.

@mdboom mdboom added this to the next bug fix release (2.0.1) milestone Oct 26, 2015
@QuLogic
Copy link
Member

QuLogic commented Oct 26, 2015

Strangely, the image from #5302 is white in the transparent parts, so I'm not sure what Pillow is doing.

Anyway, 👍 from me, but I'm not sure where 2.0.1 fixes are going.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

This is passing and ready to merge.

@WeatherGod
Copy link
Member

Just curious. Do we want to hardcode a white background, or should we be getting the color from an rcParam?

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

I think adding an rcParam would just add complication to this. We already have an rcParam for the figure background (savefig.facecolor). If the user really wants a different background color, they can just set that. This only deals with the case where the background was accidentally set to transparent (through savefig.transparent or the transparent kwarg) without realising that JPEG can't handle that.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

That said, I suppose it could go to savefig.facecolor, which by default is white.

@WeatherGod
Copy link
Member

Right... that was the param I was thinking of.

On Tue, Oct 27, 2015 at 11:05 AM, Michael Droettboom <
notifications@github.com> wrote:

That said, I suppose it could go to savefig.facecolor, which by default
is white.

—
Reply to this email directly or view it on GitHub
#5324 (comment)
.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

This now gets the background color from savefig.facecolor as suggested by @WeatherGod.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

The test failures are puzzling, and I can't reproduce locally. 😢

@WeatherGod
Copy link
Member

Perhaps add a useful message for the assert so that we can see what the value is?

@WeatherGod
Copy link
Member

prints aren't getting caught by nosetests (I think it is some sort of option set by us). Add that stuff to the assert message so that the exception will print out the relevant message.

@cbreeden
Copy link

When I first tried coming up with a patch, I tried using tostring_rgb from the renderer but that doesn't seem to be good enough. My guess is that RGB doesn't really 'flatten' the transparency regions when converting from RGBA, which I thought would've been the ideal solution, which I think would allow for figures to be given figure.facecolor and axes to be given axes.facecolor. But honestly I don't think anyone really uses transparency; except for nbagg that insists on having transparency in the figure.

I opened up a new issue related to this #5335.

@QuLogic
Copy link
Member

QuLogic commented Oct 28, 2015

Printing does work; it's just muddled between the dots: "num colors: 181". It does not print the corner pixel because the colour count assert has already failed.

@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

@QuLogic: Thanks for catching that. My hunch is there is some variability because it's lossy JPEG. I'll just increase the range somewhat.

@mdboom mdboom mentioned this pull request Oct 28, 2015
Closed
@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

@cbreeden: I suppose we could try to fix tostring_rgb as well, but it would need to have a background color as an argument added. I've created #5336 for that.

@mdboom mdboom mentioned this pull request Oct 28, 2015
Closed
tacaswell added a commit that referenced this pull request Oct 28, 2015
@tacaswell
Merge pull request #5324 from mdboom/transparent-jpg
b9957b1 
Fix #5302: Proper alpha-blending for jpeg
@tacaswell tacaswell merged commit b9957b1 into matplotlib:master Oct 28, 2015
@jenshnielsen jenshnielsen mentioned this pull request Nov 6, 2015
Closed
@mdboom mdboom deleted the transparent-jpg branch November 10, 2015 02:46
@tacaswell
Copy link
Member

Do we want to backport this to 2.x?

@efiring
Copy link
Member

efiring commented Feb 18, 2016

Sounds reasonable to me; it can be viewed as a bugfix. But it also makes me wonder whether saving a jpg with transparent=True, which this handles, should raise a warning.

@QuLogic
Copy link
Member

QuLogic commented Jul 18, 2016

I guess this was not backported, then?

QuLogic pushed a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2016
@tacaswell @QuLogic
Merge pull request matplotlib#5324 from mdboom/transparent-jpg
1e437d3 
Fix matplotlib#5302: Proper alpha-blending for jpeg

Conflicts:
	lib/matplotlib/tests/test_image.py
@QuLogic QuLogic mentioned this pull request Oct 16, 2016
Merged
@QuLogic
Copy link
Member

QuLogic commented Oct 16, 2016

Backported to v2.x via 1e437d3.

@QuLogic QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0 (style change major release) Dec 7, 2016
@tacaswell tacaswell mentioned this pull request Oct 26, 2017
Closed
@anntzer anntzer mentioned this pull request Mar 29, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
topic: color/alpha
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@mdboom @QuLogic @WeatherGod @cbreeden @tacaswell @efiring