avoid improper handling of alpha in comp-op blend_pix methods #1369

Closed
springmeyer opened this Issue Aug 3, 2012 · 6 comments

Projects

None yet

2 participants

@springmeyer
Member

#1365 highlighted that color-burn and grain-extract were not handling alpha correctly. The nature of color-burn (before the fix in #1365) made it prone to division by zero crashes. Now that that is fixed we need:

  1. A robust method to detect proper alpha handling, and test coverage of all comp-op methods. (A possibility would be to start with https://github.com/mapnik/mapnik/blob/master/tests/python_tests/compositing_test.py and add debugging checks after each pass that checks to ensure rgba values are with valid bounds).

  2. A detailed inspection pass over all comp-op methods in agg_pixfmt_rgba.h, with special attention to custom methods recently added.

It is clear from a quick glance that colorize-alpha does not handle premultiplied rgb correctly.

See also #1313

@springmeyer springmeyer pushed a commit that referenced this issue Aug 3, 2012
Dane Springmeyer first, not yet fully working attemp to validate pixel alpha status - …
…refs #1369
a51678d
@springmeyer springmeyer pushed a commit that referenced this issue Sep 1, 2012
Dane Springmeyer agg compositing: change src_over alpha to avoid pixel artifacts by re…
…ordering computations and add basic tests comparing src_over composting to normal agg alpha blending - closes #1452 - refs #1313, #1454, #1369
5e84ce0
@lightmare
Contributor

Didn't want to start another issue, this one seems ok for some thoughts about pre/de-multiplication.

agg::rgba8::premultiply computes ((color * alpha) >> 8)
instead of more precise ((color * alpha + 128) / 255)
I guess that is to make it faster. Shouldn't agg::rgba8::demultiply then do the inverse, ((color << 8) / alpha)? Currently it does ((color * 255) / alpha), which I think unnecessarily darkens colors.

I wrote a test that generates combinations of color (0..255, with r=g=b) and alpha (1..255), premultiplies, demultiplies, and gathers some statistics. With the current implementation:

colors          65280     : total number of colors checked
changed colors  64770 = N : number of colors where c.r != demul(premul(c)).r
invalid colors  0         : number of colors where c.r > c.a
avg. error      4.00823   : sum(e)/N where e = abs(c.r - demul(premul(c)).r)
avg. error*a    0.984864  : sum(e*a/255)/N  (error weighted by alpha)
avg. error^2    145.593   : sum(e*e)/N
avg. error^2*a  3.5351    : sum(e*e*a/255)/N

Here's the modified demultiply. I separately checked that ((r << 8) / a <= 255) for every (r < a).

    AGG_INLINE const self_type& demultiply()
    {
        if(a == base_mask) return *this;
        if(a == 0)
        {
            r = g = b = 0;
            return *this;
        }
        r = value_type(r < a ? (calc_type(r) << base_shift) / a : base_mask);
        g = value_type(g < a ? (calc_type(g) << base_shift) / a : base_mask);
        b = value_type(b < a ? (calc_type(b) << base_shift) / a : base_mask);
        return *this;
    }

Stats with modified demultiply:

colors          65280
changed colors  64001
invalid colors  0
avg. error      3.60171
avg. error*a    0.768455
avg. error^2    144.352
avg. error^2*a  2.71093

Less changed colors, and lower average error of those that were changed. Don't take it for granted, I'm not particularly good at statistics. I pushed the test in https://github.com/mapycz/mapnik/commits/test_premultiply for review.

@lightmare
Contributor

In deps/agg/src/agg_pixfmt_rgba.cpp, destination alpha in comp_op functions is computed in the same way that proved defective here #1452. No testcase yet, and I'm actually missing alpha handling in these ops completely. I'd think that if for example a 50% transparent pixel is comp-op="saturation" blended over destination, the resulting color would be 50% destination + 50% what would be the result if the source was opaque -- in other words, a solid source should imo saturate more than a semi-transparent source.

@springmeyer
Member

right, I wonder if we should make the 5e84ce0 change to all comp_op's.

@springmeyer springmeyer pushed a commit that referenced this issue Oct 2, 2012
Dane Springmeyer expose more compositing options in python (the non-agg custom ones) t…
…o set up for testing as per #1493 and #1369
7866cc3
@springmeyer
Member

yeah, the entire lack of alpha handling in the custom hue/saturation/color comp_op's seems to be creating some odd black results as can be see in 7866cc3.

@springmeyer
Member

@lightmare - regarding the darkening from demultiply - just honed a test case the suffers from this. I think it deserves its own issue (given your fix) - so let's discuss at #1519

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013
@PetrDlouhy Dane Springmeyer + PetrDlouhy expose more compositing options in python (the non-agg custom ones) t…
…o set up for testing as per #1493 and #1369
7052bca
@springmeyer springmeyer pushed a commit that referenced this issue Sep 20, 2013
Dane Springmeyer reduced testcase for #1995 - refs #1452 and #1369 4713ddf
@herm herm added a commit that referenced this issue Oct 13, 2013
@herm herm Merge commit '4713ddf8d6e7a280600ca331328034ecd637ba68' into hb-merge
* commit '4713ddf8d6e7a280600ca331328034ecd637ba68':
  reduced testcase for #1995 - refs #1452 and #1369

Conflicts:
	tests/visual_tests/test.py
2ad3eb0
@springmeyer
Member

next up to fix: grain-merge: #2067

@springmeyer springmeyer closed this Sep 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment