Last Comment Bug 1281593 - Nested elements with mix-blend-mode and background-image have strange artifact when transformed
: Nested elements with mix-blend-mode and background-image have strange artifac...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 46 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla51
Assigned To: David Anderson [:dvander]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 1235995
  Show dependency treegraph
 
Reported: 2016-06-22 12:12 PDT by Stephen Horlander [:shorlander]
Modified: 2017-01-03 11:11 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
wontfix
fixed


Attachments
mix-blend-mode + background-image + transform screenshot (8.29 KB, image/png)
2016-06-22 12:12 PDT, Stephen Horlander [:shorlander]
no flags Details
Force premultiplied source color to black when alpha is 0 (1.90 KB, patch)
2016-07-19 03:02 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
Force premultiplied source color to transparent when alpha is 0. (2.21 KB, patch)
2016-07-21 19:23 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
part 1, d3d11 fix (17.82 KB, patch)
2016-07-27 14:35 PDT, David Anderson [:dvander]
mstange: review+
Details | Diff | Splinter Review
reftest (3.63 KB, patch)
2016-07-27 14:36 PDT, David Anderson [:dvander]
mstange: review+
Details | Diff | Splinter Review
opengl fix (1.27 KB, patch)
2016-07-27 22:12 PDT, David Anderson [:dvander]
mstange: review+
Details | Diff | Splinter Review

Description User image Stephen Horlander [:shorlander] 2016-06-22 12:12:50 PDT
Created attachment 8764369 [details]
mix-blend-mode + background-image + transform screenshot

I ran into a rendering glitch when I tried to nest an element with mix-blend-mode inside an object with mix-blend-mode and then apply a transform.

Attaching a screenshot.

Top — no transform
Bottom — transform: rotate(45deg);

The bottom image has a grey box that should not be there. 

Test case: http://people.mozilla.org/~shorlander/bugs/transform-blend-mode-glitch.html

Only tested this on OS X, not sure what it does on other platforms.
Comment 2 User image Jet Villegas (:jet) 2016-07-19 03:02:00 PDT
Created attachment 8772360 [details] [diff] [review]
Force premultiplied source color to black when alpha is 0
Comment 3 User image Jet Villegas (:jet) 2016-07-21 19:23:24 PDT
Created attachment 8773572 [details] [diff] [review]
Force premultiplied source color to transparent when alpha is 0.

Markus: moving r? to you since you reviewed this shader before. Thx!
Comment 4 User image David Anderson [:dvander] 2016-07-21 23:41:30 PDT
Sorry, was waiting to test this locally - the fix looks good, but it needs a D3D11 shader fix as well. A reftest would almost certainly pass on Mac and fail on Windows.
Comment 5 User image Jet Villegas (:jet) 2016-07-21 23:53:38 PDT
(In reply to David Anderson [:dvander] from comment #4)
> Sorry, was waiting to test this locally - the fix looks good, but it needs a
> D3D11 shader fix as well. A reftest would almost certainly pass on Mac and
> fail on Windows.

No worries. I had to re-work the patch anyway as the comments no longer matched the code. Are you able to port it to D3D? (I'm traveling and don't have a Windows machine.)
Comment 6 User image Markus Stange [:mstange] (away until Feb 22) 2016-07-25 12:07:10 PDT
Comment on attachment 8773572 [details] [diff] [review]
Force premultiplied source color to transparent when alpha is 0.

Review of attachment 8773572 [details] [diff] [review]:
-----------------------------------------------------------------

A description of the bug would have helped me understand this patch much more quickly :) For future reference, the problem is that the "if (color.a == 0.0) { return backdrop; }" optimization is just wrong - if color.a is 0.0, then what we return must also have .a == 0.0, and that's not necessarily true for the backdrop.

Actually, writing this down cleared things up for me. Couldn't we just change the early return to return vec4(0.0, 0.0, 0.0, 0.0) instead?

By the way, two of the comments have whitespace at the end of the line.
Comment 7 User image David Anderson [:dvander] 2016-07-27 14:35:24 PDT
Created attachment 8775333 [details] [diff] [review]
part 1, d3d11 fix

Yeah good point, I think we can just return 0.
Comment 8 User image David Anderson [:dvander] 2016-07-27 14:36:12 PDT
Created attachment 8775334 [details] [diff] [review]
reftest
Comment 9 User image David Anderson [:dvander] 2016-07-27 22:11:34 PDT
Jet, I'll just steal this unless you very much want to own a mix-blend bug :) thanks for diagnosing the problem.
Comment 10 User image David Anderson [:dvander] 2016-07-27 22:12:09 PDT
Created attachment 8775424 [details] [diff] [review]
opengl fix
Comment 11 User image Markus Stange [:mstange] (away until Feb 22) 2016-07-30 08:01:36 PDT
Comment on attachment 8775424 [details] [diff] [review]
opengl fix

Review of attachment 8775424 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +736,5 @@
>    fs << "  if (backdrop.a == 0.0) {" << endl;
>    fs << "    return color;" << endl;
>    fs << "  }" << endl;
>    fs << "  if (color.a == 0.0) {" << endl;
> +  fs << "    return vec4(0.0, 0.0, 0.0, 0.0);;" << endl;

double semicolon
Comment 12 User image Pulsebot 2016-08-02 11:50:22 PDT
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1cec49be7a
Fix D3D11 mix-blending when the source alpha is 0. (bug 1281593 part 1, r=mstange)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8ee572051d
Fix OpenGL mix-blending when the source alpha is 0. (bug 1281593 part 2, r=mstange)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e63d98406fe
Add a reftest for bug 1281593. r=mstange
Comment 14 User image Liz Henry (:lizzard) (needinfo? me) 2016-08-12 10:46:47 PDT
Are you ok with the fix staying in 51, or do you want to try uplift? Unless you think this is a common problem for users or has a big impact I think it's likely ok to stay in 51.
Comment 15 User image David Anderson [:dvander] 2016-08-12 12:33:41 PDT
I think this is okay to not uplift for now, it's pretty obscure.

Note You need to log in before you can comment on or make changes to this bug.