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

fix blue-channel bias on blend() #514

Closed
processing-bugs opened this Issue Feb 10, 2013 · 7 comments

Comments

Projects
None yet
3 participants
@processing-bugs

processing-bugs commented Feb 10, 2013

Original author: daveboll...@gmail.com (December 13, 2010 16:30:19)

What steps will reproduce the problem?

PGraphics blender;
void setup() {
  size(200,200,P2D);
  // to rule out bilinear filtering as culprit
  noSmooth();
  blender = createGraphics(width,height,P2D);
  blender.beginDraw();
  // set up to perform "-2" math via blend()
  // (which will actually perform "-1" math due to alpha rounding)
  // aside:
  //   this bug is NOT intended to revisit the >>8 vs /255 issue
  //   but its effects are present:
  //   background(1) isn't enough, becomes 0 in alpha rounding, no fade
  //   background(2) gets perfectly "stuck" by rounding,
  //      fades to blue, best demo of THIS bug
  //   background(3) overcomes rounding, and does fade to black,
  //      though passes through "blue" on the way (again this bug)
  blender.background(2);
  blender.endDraw(); 
  // start with black screen
  background(0);
}

void draw() {
  // add some non-black content
  fill(255);
  noStroke();
  ellipse(random(width),random(height),10,10);
  // fade to "black"?  nope, fade to "blue"
  blend(blender,0,0,width,height,0,0,width,height,SUBTRACT);
  // suspect blend_sub_pin() wiping out other channels:
  //   green mask used in red channel
  //   blue mask used in green channel
}

What is the expected output? What do you see instead?
expected: fade to black
instead: fade to blue

What version of the product are you using? On what operating system?
1.2.1 WinXPsp3

Original issue: http://code.google.com/p/processing/issues/detail?id=475

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From f...@processing.org on June 20, 2011 23:53:59
Removing blend() (in favor of blendMode()) for 2.0, so closing this one.

Though if there are ideas for a fix, would be great to have so that we're not running into a similar problem with the blendMode() implementation.

/**

  • subtractive blend with clipping
    */
    private static int blend_sub_pin(int a, int b) {
    int f = (b & ALPHA_MASK) >>> 24;

    return (low(((a & ALPHA_MASK) >>> 24) + f, 0xff) << 24 |
    high(((a & RED_MASK) - ((b & RED_MASK) >> 8) * f),
    GREEN_MASK) & RED_MASK |
    high(((a & GREEN_MASK) - ((b & GREEN_MASK) >> 8) * f),
    BLUE_MASK) & GREEN_MASK |
    high((a & BLUE_MASK) - (((b & BLUE_MASK) * f) >> 8), 0));
    }

processing-bugs commented Feb 10, 2013

From f...@processing.org on June 20, 2011 23:53:59
Removing blend() (in favor of blendMode()) for 2.0, so closing this one.

Though if there are ideas for a fix, would be great to have so that we're not running into a similar problem with the blendMode() implementation.

/**

  • subtractive blend with clipping
    */
    private static int blend_sub_pin(int a, int b) {
    int f = (b & ALPHA_MASK) >>> 24;

    return (low(((a & ALPHA_MASK) >>> 24) + f, 0xff) << 24 |
    high(((a & RED_MASK) - ((b & RED_MASK) >> 8) * f),
    GREEN_MASK) & RED_MASK |
    high(((a & GREEN_MASK) - ((b & GREEN_MASK) >> 8) * f),
    BLUE_MASK) & GREEN_MASK |
    high((a & BLUE_MASK) - (((b & BLUE_MASK) * f) >> 8), 0));
    }

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 15:37:59
I'd almost forgot about this. I'll post over a couple comments to try to separate some various thoughts. First, an alternate (simpler, and perhaps clearer?) demo of the problem:

void setup() {
//
// set up the equation: 255 - 2 = ???
//
color A = color(255);
color B = color(2);
//
// demo incorrect result of SUBTRACT
//
printColor( blendColor(A,B,SUBTRACT) ); // 253,253,254
//
// demo correct result of DIFFERENCE
// which is functionally equivalent to subtract
// WHEN all channels of A > all channels of B
//
printColor( blendColor(A,B,DIFFERENCE) ); // 253,253,253
}

void printColor(int c) {
println("<" + red(c) + "," + green(c) + "," + blue(c) + ">");
}

processing-bugs commented Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 15:37:59
I'd almost forgot about this. I'll post over a couple comments to try to separate some various thoughts. First, an alternate (simpler, and perhaps clearer?) demo of the problem:

void setup() {
//
// set up the equation: 255 - 2 = ???
//
color A = color(255);
color B = color(2);
//
// demo incorrect result of SUBTRACT
//
printColor( blendColor(A,B,SUBTRACT) ); // 253,253,254
//
// demo correct result of DIFFERENCE
// which is functionally equivalent to subtract
// WHEN all channels of A > all channels of B
//
printColor( blendColor(A,B,DIFFERENCE) ); // 253,253,253
}

void printColor(int c) {
println("<" + red(c) + "," + green(c) + "," + blue(c) + ">");
}

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 15:52:23
The difference (sic) between DIFFERENCE and SUBTRACT is the exact formulation of the lerp. Purely mathematically they are equivalent, but with alpha rounding (and >>8 != /255 issue) they are functionally different.

If we normalize RGBA on 0..1 for simplicity, then:

SUBTRACT is worded: result = a - b_f;
DIFFERENCE (when a > b) is worded: c=a-b; result = a + (c-a)_f

Assuming alpha is at perfect unity so can eliminate the "f" term, both equations simplify to "a-b" as expected. But in SUBTRACT the integer alpha rounding occurs only on the b term, in isolation. In DIFFERENCE that rounding occurs on the difference term in a lerp formulation. That difference becomes an issue when "f" is NOT perfect unity.

If if it were up to me to fix it, I'd borrow from the blend_difference code, just simplifying the "// formula:" portion so that it's always (not conditionally) a-b. It wouldn't be quite as inline optimized as current version, but it'll be more correct.

processing-bugs commented Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 15:52:23
The difference (sic) between DIFFERENCE and SUBTRACT is the exact formulation of the lerp. Purely mathematically they are equivalent, but with alpha rounding (and >>8 != /255 issue) they are functionally different.

If we normalize RGBA on 0..1 for simplicity, then:

SUBTRACT is worded: result = a - b_f;
DIFFERENCE (when a > b) is worded: c=a-b; result = a + (c-a)_f

Assuming alpha is at perfect unity so can eliminate the "f" term, both equations simplify to "a-b" as expected. But in SUBTRACT the integer alpha rounding occurs only on the b term, in isolation. In DIFFERENCE that rounding occurs on the difference term in a lerp formulation. That difference becomes an issue when "f" is NOT perfect unity.

If if it were up to me to fix it, I'd borrow from the blend_difference code, just simplifying the "// formula:" portion so that it's always (not conditionally) a-b. It wouldn't be quite as inline optimized as current version, but it'll be more correct.

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 16:19:18
wrt that darned >>8 alpha blending...

So when the alpha value == 255 we pretend that that's full unity (1.0), then divide it by 256 (>>8) in the equation, giving slightly less than full unity (~0.0996), leading to various errors throughout ALL the blending math.

OK, so we all know the right way to fix it, right? Simply replace ">>8" with "/255" wherever that sort of thing occurs. (and similarly there are a few equations containing a ">>7" that should really be "*2/255") But that's a 3-divisions-per-pixel (or more) hit where you may still prefer performace to accuracy.

There IS a way to improve (tho not completely fix) that accuracy with one division per pixel. Instead of fixing it at the end in the blend equation per channel, we tweak it up front in the alpha value extraction once:

as is:
int f = (b & ALPHA_MASK) >>> 24;

fix is to "stretch" f on 0..255 into f on 0..256
so at least the ends of the range are correct when later >>8'd

modified long form for readability:
int f = ((b & ALPHA_MASK) >>> 24) * 256 / 255;

modified short form for performance:
int f = ((b & ALPHA_MASK) >>> 16) / 255;

(or you could potentially just set up that int[256] remap in advance, then:
int f = stretch255to256[(b & ALPHA_MASK) >>> 24];
)

now when the actual RGBA alpha is 255, it will remap to 256, so that when later >>8 math occurs, unity is preserved. there will still be "dead" spots in the math across the whole range, (as the "stretched histogram" of f will have "plateaus" where single input values repeat twice), but the overall error will be less (and in fact, this fix alone would be enough to correct the specific 255-2=254 problem demonstrated above in blend_subtract()), and the unity of 255==256==1.0 is at least sorted out.

anyway, long-winded, hth

processing-bugs commented Feb 10, 2013

From daveboll...@gmail.com on June 21, 2011 16:19:18
wrt that darned >>8 alpha blending...

So when the alpha value == 255 we pretend that that's full unity (1.0), then divide it by 256 (>>8) in the equation, giving slightly less than full unity (~0.0996), leading to various errors throughout ALL the blending math.

OK, so we all know the right way to fix it, right? Simply replace ">>8" with "/255" wherever that sort of thing occurs. (and similarly there are a few equations containing a ">>7" that should really be "*2/255") But that's a 3-divisions-per-pixel (or more) hit where you may still prefer performace to accuracy.

There IS a way to improve (tho not completely fix) that accuracy with one division per pixel. Instead of fixing it at the end in the blend equation per channel, we tweak it up front in the alpha value extraction once:

as is:
int f = (b & ALPHA_MASK) >>> 24;

fix is to "stretch" f on 0..255 into f on 0..256
so at least the ends of the range are correct when later >>8'd

modified long form for readability:
int f = ((b & ALPHA_MASK) >>> 24) * 256 / 255;

modified short form for performance:
int f = ((b & ALPHA_MASK) >>> 16) / 255;

(or you could potentially just set up that int[256] remap in advance, then:
int f = stretch255to256[(b & ALPHA_MASK) >>> 24];
)

now when the actual RGBA alpha is 255, it will remap to 256, so that when later >>8 math occurs, unity is preserved. there will still be "dead" spots in the math across the whole range, (as the "stretched histogram" of f will have "plateaus" where single input values repeat twice), but the overall error will be less (and in fact, this fix alone would be enough to correct the specific 255-2=254 problem demonstrated above in blend_subtract()), and the unity of 255==256==1.0 is at least sorted out.

anyway, long-winded, hth

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From daveboll...@gmail.com on June 23, 2011 14:26:52
apologies for writing what now amounts to a treatise on alpha math, perhaps it'll be of some use some day, but another thought occurs to me wrt a kludge solution...

again, your options, as i see them:

  1. leave as is: f on 0..255, blend uses >>8
  2. kludge fix: f on 0..256, blend uses >> 8
  3. full fix: f on 0..255, blend uses /255

so, IF you decided to go with option 2...
note that it doesn't actually fix the problem, just alters the error distribution to preserve the unity value.

the kludge forms given in the message above have a flaw, but one that can actually be exploited as an even simpler solution. as given above, rounding is ignored, and technically something like this would be more correct (written long form for readability):

int f = (b & ALPHA_MASK) >>> 24; // existing equation
f = (f * 256 + 128) / 255; // stretch with proper rounding

but that second line could be rewritten as a simple conditional, giving up a *+/ in the process:

int f = (b & ALPHA_MASK) >>> 24; // existing equation
if (f > 127) f++; // functionally equivalent to stretch with proper rounding

so NO divisions per pixel, just one extra conditional.

personally, i'd rather see option 3 implemented (let me know, i might volunteer), as i think the performance tradeoff versus the vast array of problems it generates is well worth it. but option 2 would fix many of the more obvious problems, with almost no performance penalty (and just a simple one line code insertion at each occurent to implement).

cheers, hth

processing-bugs commented Feb 10, 2013

From daveboll...@gmail.com on June 23, 2011 14:26:52
apologies for writing what now amounts to a treatise on alpha math, perhaps it'll be of some use some day, but another thought occurs to me wrt a kludge solution...

again, your options, as i see them:

  1. leave as is: f on 0..255, blend uses >>8
  2. kludge fix: f on 0..256, blend uses >> 8
  3. full fix: f on 0..255, blend uses /255

so, IF you decided to go with option 2...
note that it doesn't actually fix the problem, just alters the error distribution to preserve the unity value.

the kludge forms given in the message above have a flaw, but one that can actually be exploited as an even simpler solution. as given above, rounding is ignored, and technically something like this would be more correct (written long form for readability):

int f = (b & ALPHA_MASK) >>> 24; // existing equation
f = (f * 256 + 128) / 255; // stretch with proper rounding

but that second line could be rewritten as a simple conditional, giving up a *+/ in the process:

int f = (b & ALPHA_MASK) >>> 24; // existing equation
if (f > 127) f++; // functionally equivalent to stretch with proper rounding

so NO divisions per pixel, just one extra conditional.

personally, i'd rather see option 3 implemented (let me know, i might volunteer), as i think the performance tradeoff versus the vast array of problems it generates is well worth it. but option 2 would fix many of the more obvious problems, with almost no performance penalty (and just a simple one line code insertion at each occurent to implement).

cheers, hth

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From f...@processing.org on July 02, 2011 21:13:26
Wow, thanks for all this... Our plan is to move things over to an actual blendMode() function rather than the straight image blending for 2.0, so I'll try to get this fixed as we implement it.

processing-bugs commented Feb 10, 2013

From f...@processing.org on July 02, 2011 21:13:26
Wow, thanks for all this... Our plan is to move things over to an actual blendMode() function rather than the straight image blending for 2.0, so I'll try to get this fixed as we implement it.

@benfry benfry removed the bug label Nov 15, 2014

@benfry benfry added the help wanted label Jun 16, 2015

@JakubValtar JakubValtar self-assigned this Aug 11, 2015

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Aug 11, 2015

Contributor

Working on this right now.

Contributor

JakubValtar commented Aug 11, 2015

Working on this right now.

@JakubValtar JakubValtar removed the revised label Aug 11, 2015

@benfry benfry added this to the 3.0 final milestone Aug 11, 2015

@benfry benfry added the high label Aug 11, 2015

@benfry benfry closed this in #3592 Aug 14, 2015

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