New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 618122 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Animometer - CSS bouncing blend circles renders incorrectly with GPU raster

Project Member Reported by vmi...@chromium.org, Jun 7 2016

Issue description

Version: r395672
OS: OS X

What steps will reproduce the problem?
(1) https://trac.webkit.org/export/HEAD/trunk/PerformanceTests/Animometer/developer.html
(2) Run "CSS bouncing blend circles" in the HTML Suite.

I'm running on a MBP 15" Mid 2015 with AMD Radeon R9 M370X.  It also reproduces with the Intel GPU.
 
Screen Shot 2016-06-07 at 3.16.59 PM.png
431 KB View Download
Labels: Hotlist-Animometer OS-Android OS-Mac
I can repro on linux as well.
Owner: bsalo...@google.com
Status: Assigned (was: Available)
I grabbed a skp and it does not reproduce in standalone skia.
bouncing_blend_circles.skp
22.7 KB Download
There are few things going on here:

1) Chrome isn't exposing support for GL_[KHR|NV]_blend_equation_advanced even though the driver on my system supports it. This means that for complicated blends Skia will fallback to making a copy of the destination to feed into the fragment shader.

2) Skia marks the temporary surface created for the copy of the destination as having an different y-orientation than the original destination FBO.

3) When the deferred copy is actually executed Skia finds it can't come up with a way of making a copy of the destination using any of its techniques: (The "src" of the copy is the original destination of the draw and the "dst" of the copy is the temporary dst copy).
  a) We can't do a draw because Skia knows the src's FBO ID but doesn't have a texture ID for it
  b) We can't do a glCopyTexSubImage2D because the src and dst are BGRA and this isn't spec'ed for
     GL_EXT_texture_format_BGRA8888.
  c) We can't do a glBlitFramebuffer because GL_CHROMIUM_framebuffer_multisample doesn't allow mirroring
     blits and we chose a different y orientation for the dst in 2).

I have a change that fixes this by making the temporary in 2) use the same orientation as the original dst. This allows us to do a glBlitFramebuffer to make the copy.

However, it seems like we should also address 1) so that we don't need to make a copy for shader based blending as well. Alternatively, if GL_NV_texture_barrier were exposed Skia would use that to perform shader-based blending without a copy.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 8 2016

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/5c7d62d431a0ad7988f05fbdb206237729c0c308

commit 5c7d62d431a0ad7988f05fbdb206237729c0c308
Author: bsalomon <bsalomon@google.com>
Date: Wed Jun 08 17:02:42 2016

When setting up a copySurface dst texture make the orientation match the src when glBlitFramebuffer requires it

BUG= chromium:618122 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2049753002

Review-Url: https://codereview.chromium.org/2049753002

[modify] https://crrev.com/5c7d62d431a0ad7988f05fbdb206237729c0c308/src/gpu/gl/GrGLGpu.cpp

Thanks for figuring this out! I'll file bugs about exposing these extensions.

I think we should merge the fix into M52, as this bug will cause visual artifacts on sites that use "mix-blend-mode".
Labels: Merge-Request-52

Comment 8 by tin...@google.com, Jun 8 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 8 2016

Labels: merge-merged-m52
The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/5bfd891d97fc4ac0b54aef0062474018d0e73389

commit 5bfd891d97fc4ac0b54aef0062474018d0e73389
Author: bsalomon <bsalomon@google.com>
Date: Wed Jun 08 19:32:02 2016

When setting up a copySurface dst texture make the orientation match the src when glBlitFramebuffer requires it

TBR=egdaniel@google.com
BUG= chromium:618122 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2049753002

Review-Url: https://codereview.chromium.org/2049753002

Cherry-pick from master to chrome/m52
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2049493003

[modify] https://crrev.com/5bfd891d97fc4ac0b54aef0062474018d0e73389/src/gpu/gl/GrGLGpu.cpp

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d214d26887a2d522c80ef35b54d09cc242aac8e2

commit d214d26887a2d522c80ef35b54d09cc242aac8e2
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Wed Jun 08 20:34:57 2016

Roll src/third_party/skia/ 7942f22c6..2a437e645 (4 commits).

https://chromium.googlesource.com/skia.git/+log/7942f22c607c..2a437e64593a

$ git log 7942f22c6..2a437e645 --date=short --no-merges --format='%ad %ae %s'
2016-06-08 liyuqian Provide filter when there are many options
2016-06-08 csmartdalton Replace targetHasUnifiedMultisampling in GrPB constructor
2016-06-08 bsalomon When setting up a copySurface dst texture make the orientation match the src when glBlitFramebuffer requires it
2016-06-08 msarett Modify QCMS test to match use by Chrome

BUG= 618122 

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=mtklein@google.com

Review-Url: https://codereview.chromium.org/2044293003
Cr-Commit-Position: refs/heads/master@{#398662}

[modify] https://crrev.com/d214d26887a2d522c80ef35b54d09cc242aac8e2/DEPS

Confirmed with Dev, the change was in Skia. It was cherry picked it to the M52 branch of Skia. So, the next build of M52 Chrome will pick it up.

Thank you Brian for merging it to the M52 branch quickly.
Project Member

Comment 13 by sheriffbot@chromium.org, Jun 12 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Approved -Merge-Approved-52

Sign in to add a comment