Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

premultiplication handling in image scaling #1489

Closed
springmeyer opened this Issue Sep 15, 2012 · 7 comments

Comments

Projects
None yet
2 participants
Owner

springmeyer commented Sep 15, 2012

Realizing now the potential of confusion around premultiplied alpha and image scaling, so a ticket to track it until its not friday afternoon and I'm thirsty.

Recently I determined that agg image scaling classes did not need to worry about premultiplied pixels: 1b4e7a8. I deemed this so because debug printing revealed that the blender (that makes the difference between agg::pixfmt_rgba32 and agg::pixfmt_rgba32_pre) was not being called. And anyway, why assume the destination was premultiplied when the source was not (yet)?

This was wrong, or at least caused regressions in test output.

We keep a custom copy of agg_span_image_filter in include/mapnik/span_image_filter.hpp (#1227) for reasons I've not fully tracked. Perhaps its code changes in that file are compounding issues - needs a closer look.

Until then I will revert 1b4e7a8 and 61e8a9c

Owner

springmeyer commented Sep 15, 2012

test cases that are failing:

======================================================================
FAIL: python_tests.raster_alpha_test.test_map_alpha_compare
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/dane/projects/mapnik5/tests/python_tests/raster_alpha_test.py", line 25, in test_map_alpha_compare
    eq_(im.tostring(),expected_im.tostring(), 'failed comparing actual (%s) and expected(%s)' % (actual,'tests/python_tests/'+ expected))
  File "/Library/Python/2.7/site-packages/nose/tools.py", line 31, in eq_
    assert a == b, msg or "%r != %r" % (a, b)
AssertionError: failed comparing actual (/tmp/mapnik-raster-alpha.png) and expected(tests/python_tests/images/support/raster-alpha.png)

======================================================================
FAIL: python_tests.raster_alpha_test.test_map_alpha_gradient_compare
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/dane/projects/mapnik5/tests/python_tests/raster_alpha_test.py", line 37, in test_map_alpha_gradient_compare
    eq_(im.tostring(),expected_im.tostring(), 'failed comparing actual (%s) and expected(%s)' % (actual,'tests/python_tests/'+ expected))
  File "/Library/Python/2.7/site-packages/nose/tools.py", line 31, in eq_
    assert a == b, msg or "%r != %r" % (a, b)
AssertionError: failed comparing actual (/tmp/mapnik-raster-alpha-gradient.png) and expected(tests/python_tests/images/support/raster-alpha-gradient.png)

======================================================================
FAIL: python_tests.raster_symbolizer_test.test_raster_with_alpha_blends_correctly_with_background
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/dane/projects/mapnik5/tests/python_tests/raster_symbolizer_test.py", line 142, in test_raster_with_alpha_blends_correctly_with_background
    assert contains_word('\xff\xff\xff\xff', imdata)
AssertionError

----------------------------------------------------------------------

specifically this:

is now, wrongly:

Owner

springmeyer commented Sep 15, 2012

61e8a9c reverted in f3589ab (warper)

@springmeyer springmeyer pushed a commit that referenced this issue Sep 15, 2012

Dane Springmeyer rollback 1b4e7a8 - refs #1489 and #1227 2d5287c
Owner

springmeyer commented Sep 15, 2012

1b4e7a8 rolled back in 2d5287c

Owner

springmeyer commented Sep 15, 2012

all tests now pass again. leaving this open until we can research how this should, does, or does not interplay with the span modifications.

Contributor

lightmare commented Sep 29, 2012

Recently I determined that agg image scaling classes did not need to worry about premultiplied pixels:

Scaling a premultiplied image is different from scaling a non-premultiplied one. A naive 1-dimensional example: let's have 2 pixels (c1,a1), (c2,a2), and interpolate a pixel (c,a) between them (like when we're stretching the x-axis twice).

In a demultiplied image, we can compute the average of color and alpha separately:

c = (c1 + c2) / 2
a = (a1 + a2) / 2
if we then premultiply this image, ca = (c1 + c2) * (a1 + a2) / 4

But if we used the same method in a premultiplied image, the result would be different:

ca = (ca1 + ca2) / 2
a = (a1 + a2) / 2
which leads to c = ca/a = (ca1 + ca2) / (a1 + a2) = (c1*a1 + c2*a2) / (a1 + a2),
the source pixels' alpha affects the result's color, and i think it shouldn't

edit: According to a discussion dane found on agg mailing list, I had it reversed: the first formula would be correct for premultiplied images, and the second for non-premultiplied. Sorry for the confusion.

@springmeyer springmeyer pushed a commit that referenced this issue Oct 3, 2012

Dane Springmeyer agg scaling assumes/requires a premulitiplied renderer (and optionall…
…y a source defined as premultiplied) - to avoid any confusion use purely premultiplied types - refs #1508 and #1489
1b5a1f0
Owner

springmeyer commented Jan 31, 2013

finally just hit a testcase that exposes a problem in our custom version of the span filter. Switching back to agg and the bug goes away so I think we modified the agg version in error when we were not handling premultiplication correct. Scaling algo's in agg DO expect/assume premultiplied pixels!

@springmeyer springmeyer pushed a commit that referenced this issue Jan 31, 2013

Dane Springmeyer add testcase for broken premultiplied alpha in resampling - refs #1489
…and mapbox/tilemill#1888
523f00c
Owner

springmeyer commented May 21, 2013

/cc @rcoup - as far as I'm concerned this is fixed but you indicated that it is not fully working. Please don't forget creating a ticket for this :)

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

@PetrDlouhy Dane Springmeyer + PetrDlouhy rollback 1b4e7a8 - refs #1489 and #1227 15f1cdd

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

@PetrDlouhy Dane Springmeyer + PetrDlouhy agg scaling assumes/requires a premulitiplied renderer (and optionall…
…y a source defined as premultiplied) - to avoid any confusion use purely premultiplied types - refs #1508 and #1489
7a3ad68

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

@PetrDlouhy Dane Springmeyer + PetrDlouhy add testcase for broken premultiplied alpha in resampling - refs #1489
…and mapbox/tilemill#1888
8f0e848

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

@PetrDlouhy Dane Springmeyer + PetrDlouhy fix raster resampling bug - closes #1489 and fixes mapbox/tilemill#1888 ff3affa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment