Fixed background colour of PNGs saved with a non-zero opacity. #1868

Merged
merged 2 commits into from May 1, 2013

Conversation

Projects
None yet
4 participants
Member

pelson commented Mar 29, 2013

The following code (a derivative of http://stackoverflow.com/questions/15691297/how-to-directly-set-alpha-channel-value-for-matplotlib-figure-background-colour) was producing a png with an unexpected background colour. This PR fixes the problem so that the background colour of a PNG is as expected (and consistent with SVG).

import matplotlib.patches as mpatches
from matplotlib.image import imread
from matplotlib.backends.backend_agg import FigureCanvasAgg as FigureCanvas
from matplotlib.figure import Figure

fig = Figure()
canvas = FigureCanvas(fig)
fig.set_facecolor('black')
fig.patch.set_alpha(0.3)

fig.patches.append(mpatches.CirclePolygon([200, 200],
                                          radius=80,
                                          facecolor='red'))

print 'Target: ', fig.patch.get_facecolor()

fig.savefig('test_fig.png',
            facecolor=fig.get_facecolor(),
            edgecolor='none')
print 'Result: ', tuple(imread('test_fig.png')[0, 0])

Yields:

Target:  (0.0, 0.0, 0.0, 0.3)
Result:  (0.69803923, 0.69803923, 0.69803923, 0.3019608)

After this change, the result is:

Target:  (0.0, 0.0, 0.0, 0.3)
Result:  (0.0, 0.0, 0.0, 0.3019608)

I'm not certain why the value is not exact, but I'm comfortable with the approximation.

There remains a problem with PDF creation (& other backends not tested). I do not propose to fix the PDF issue in this PR.

@mdboom mdboom commented on the diff Mar 29, 2013

lib/matplotlib/backends/backend_pdf.py
@@ -436,7 +436,7 @@ def __init__(self, filename):
revision = ''
self.infoDict = {
- 'Creator': 'matplotlib %s, http://matplotlib.sf.net' % __version__,
+ 'Creator': 'matplotlib %s, http://matplotlib.org' % __version__,
@mdboom

mdboom Mar 29, 2013

Owner

Good catch!

@mdboom mdboom commented on the diff Mar 29, 2013

src/_backend_agg.cpp
@@ -411,7 +411,7 @@
renderingBuffer.attach(pixBuffer, width, height, stride);
pixFmt.attach(renderingBuffer);
rendererBase.attach(pixFmt);
- rendererBase.clear(agg::rgba(1, 1, 1, 0));
+ rendererBase.clear(agg::rgba(0, 0, 0, 0));
@mdboom

mdboom Mar 29, 2013

Owner

Just curious -- why does this matter?

@pelson

pelson Apr 1, 2013

Member

TBH I'm not sure. I'll experiment with reverting this (and my other colour change) now that I've merged @Westacular 's changes into this PR.

@Westacular

Westacular Apr 1, 2013

Contributor

Yeah, it shouldn't make a difference anymore, now that the zero alpha value is being handled correctly.

@mdboom

mdboom Apr 11, 2013

Owner

BTW: I don't care that it's been changed -- all zeros is in some way nicer -- I just wanted to make sure I was understanding what was going on.

@mdboom mdboom commented on an outdated diff Mar 29, 2013

lib/matplotlib/tests/test_figure.py
@@ -74,6 +74,22 @@ def test_suptitle():
fig.suptitle('title', color='g', rotation='30')
+@image_comparison(baseline_images=['alpha_background'],
+ extensions=['png', 'svg'], # PDF is wrong
@mdboom

mdboom Mar 29, 2013

Owner

Should we try to fix PDF?

Contributor

Westacular commented Mar 31, 2013

Hi, I'm the one who posted that question on Stack Overflow.

I've tested this code, and it doesn't fix the underlying problem -- the "cleared" (alpha=0) contents of the renderer are still being blended into the desired background color; the problem is merely masked with the included test because the test background and the cleared renderer are both black.

I did some Googling and some digging into AGG (notably, finding this) and I've found the culprit: It's the use of agg::pixfmt_rgba32 instead of agg::pixfmt_rgba32_plain as the pixel format for the renderer. The blending function for the non-plain version takes a shortcut in the calculation that breaks down when the existing value in the buffer has alpha=0; the _plain variant handles this correctly. I've done a quick test of a build that uses _plain instead, and the output it generates is correct.

Should I submit a pull request with that fix, and some improvements to the test case?

Member

pelson commented Mar 31, 2013

Sure. You can submit them as PRs against my PR, or its own PR to mpl.

Cheers @Westacular

@pelson pelson and 1 other commented on an outdated diff Apr 1, 2013

lib/matplotlib/tests/test_figure.py
@@ -74,6 +74,23 @@ def test_suptitle():
fig.suptitle('title', color='g', rotation='30')
+@image_comparison(baseline_images=['alpha_background'],
+ extensions=['png', 'svg'], # PDF is wrong
+ savefig_kwarg={'facecolor': (0, 1, 0.4), 'edgecolor': 'none'})
+def test_alpha():
+ # We want an image which has a background color of black, with an
@pelson

pelson Apr 1, 2013

Member

Comment now incorrect.

@Westacular

Westacular Apr 1, 2013

Contributor

Oh! Oops. Can you fix that?

My reasoning for the colour change was to choose something that's likely to show a problem were the blending issue (or something like it) to reoccur, regardless of if it's being blended with black or white. So, zero for one colour channel, one for another, and something else for the third.

I changed the alpha to 0.4 simply because I was hand-calculating what the correct blended colour should be in the red circle, and (in theory) 0.4 leads to less round-off error when the values and calculations are being handled as uint8s.

@pelson

pelson Apr 1, 2013

Member

Yep, I'll change it. I agree with your color test - it covers all the bases.

Owner

mdboom commented Apr 11, 2013

Thanks for the work on this. We've found a lot of subtle things out about Agg lately, haven't we?

@pelson: I wonder whether if maybe we should rebase this on v1.2.x, since it's really sort of bugfix. I guess I'm waffling -- because obviously it will change behavior who are expecting transparent to come out as white in the PNG. There have been a few other notable bugfixes on v1.2.x since 1.2.1, and I'm wondering if it might be nice to have a 1.2.2 sooner rather than later.

Member

pelson commented Apr 11, 2013

We've found a lot of subtle things out about Agg lately, haven't we?

Yes. As we speak I'm going through the Agg backend to have another look at compound rendering, so I might turn over some more too...

There have been a few other notable bugfixes on v1.2.x since 1.2.1, and I'm wondering if it might be nice to have a 1.2.2 sooner rather than later.

I certainly have a couple of bugs that need my attention before a v1.2.2, but I also note that v1.3 at https://github.com/matplotlib/matplotlib/blob/master/doc/users/whats_new.rst#new-in-matplotlib-1-3 lists more new features than any other release. There is definitely a debate to be had on the dev mailing list about when to start aiming for a v1.3 (I hate sounding like a broken record 😄 - I'm sorry about that!).

Member

pelson commented Apr 12, 2013

I've updated this (with a trival change). I think this is now good to go.

Owner

mdboom commented Apr 12, 2013

Can you rebase this on master -- it currently won't merge cleanly. That will give us another go around with Travis, too.

Owner

efiring commented Apr 28, 2013

The only conflict is a trivial one in doc/users/whats_new.rst.

Tests look OK, with 1 error and 2 failures related to fonts; I don't think there is any relation to this PR.

Member

pelson commented Apr 29, 2013

I've squashed and rebased. Just waiting for travis-ci to do its thing & then I think it's good to go.

Owner

mdboom commented Apr 29, 2013

My only hesitation is that the PDF backend isn't matching Agg now. Have you had a chance to look into that, or should I?

Contributor

Westacular commented Apr 29, 2013

I'm not sure what you're referring to. What is the issue/mismatch with PDF?

Owner

mdboom commented Apr 29, 2013

The alpha_background test is turned off for PDF and there is a comment "PDF is wrong". I was just thinking maybe we should make it correct if possible before merging this.

Contributor

Westacular commented Apr 30, 2013

As far as I can tell, the PDF being produced is correct. The ghostscript conversion of the PDF, however, insists on blending the PDF data with a white page background colour (eliminating all transparency in the process), and I've been unable to figure out a way around that. (The options that do support transparency seem to consider it an all-or-nothing matter.) I've also been unable to find a way specify page colour within the PDF, and I suspect there isn't one.

Converting the PDF to PNG using Inkscape gives a PNG with the correct colours and alpha values.

Owner

mdboom commented Apr 30, 2013

Ah, I see. Well, maybe we should just update the comment then, to
something like: "Ghostscript does not preserve the background color, so it
is not useful to test this with PDF".

On Apr 29, 2013 9:06 PM, "Wes Campaigne" notifications@github.com wrote:

As far as I can tell, the PDF being produced is correct. The ghostscript
conversion of the PDF, however, insists on blending the PDF data with a
white page background colour (eliminating all transparency in the process),
and I've been unable to figure out a way around that. (The options that do
support transparency seem to consider it an all-or-nothing matter.) I've
also been unable to find a way specify page colour within the PDF, and I
suspect there isn't one.

Converting the PDF to PNG using Inkscape gives a PNG with the correct
colours and alpha values.


Reply to this email directly or view it on GitHubhttps://github.com/matplotlib/matplotlib/pull/1868#issuecomment-17204384
.

Member

pelson commented May 1, 2013

I've just added that comment @mdboom in an appended commit. I always find it odd that I can modify the history of another author with git...

@Westacular @pelson Westacular Switch agg backend to use pixfmt_rgba32_plain instead of pixfmt_rgba3…
…2, which fixes a problem with alpha-blending partially transparent backgrounds.
c985196
Member

pelson commented May 1, 2013

Woops, pushed the wrong branch. Fixed now.

efiring merged commit 89479d3 into matplotlib:master May 1, 2013

1 check passed

default The Travis build passed
Details
Member

pelson commented May 1, 2013

Thanks for merging @efiring. Great work @Westacular - this is really nitty-gritty matplotlib. Thanks for your contribution!

pelson referenced this pull request Oct 9, 2013

Merged

Rastized background color #2479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment