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

Issue 801769 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Missing Images on pages - Skia + Discardable Images texture bindings interaction

Project Member Reported by ericrk@chromium.org, Jan 13 2018

Issue description

Before we unlock a discardable texture in CC, we tell Skia to flush pending IO on the texture. However, this doesn't clear any outstanding texture bindings involving this texture which Skia has created. Unfortunately, the act of unlocking in CC does clear these bindings on the GPU service side, leaving Skia's understanding of GL state incorrect. This leads to errors if we quickly re-lock this texture and use it in Skia, as Skia  will assume the texture is still bound, won't re-bind it, and we'll hit an error when trying to draw.

This leads to missing images on websites. This was discovered via WIP canvas work on the following layout test:
fast/canvas/patternfill-repeat.html

However analysis of the code shows that this can be hit in the current M64 code today.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 14 2018

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

commit 462ca4d55b56fa45f85ec613e73d3e52d306b82d
Author: Eric Karl <ericrk@chromium.org>
Date: Sun Jan 14 01:27:20 2018

Reset texture bindings when unlocking discardable textures used by Skia

Before we unlock a discardable texture in CC, we tell Skia to flush
pending IO on the texture. However, this doesn't clear any outstanding
texture bindings involving this texture which Skia has created.
Unfortunately, the act of unlocking in CC does clear these bindings on
the GPU service side, leaving Skia's understanding of GL state incorrect.
This leads to errors if we quickly re-lock this texture and use it in
Skia, as Skia  will assume the texture is still bound, won't re-bind
it, and we'll hit an error when trying to draw.

This change resets Skia's texture bindings whenever we unlock textures,
preventing this scenario.

R=khushalsagar

Bug:  801769 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I4cdfa58b6696e95fb40fa7d48a8fec3f09e8398e
Reviewed-on: https://chromium-review.googlesource.com/866111
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529179}
[modify] https://crrev.com/462ca4d55b56fa45f85ec613e73d3e52d306b82d/cc/tiles/gpu_image_decode_cache.cc

Comment 2 by ericrk@chromium.org, Jan 15 2018

Labels: ReleaseBlock-Stable Merge-Request-64
This bug is very likely to cause users to have missing images during page display, and is a regression. The fix is very targeted / safe, and I'd like to merge to M64.
Project Member

Comment 3 by sheriffbot@chromium.org, Jan 15 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by ericrk@chromium.org, Jan 16 2018

Labels: -OS-Linux OS-Chrome
Thanks for the details.
Eric - can you please confirm this bug is well tested in canary and extremely safe? This is our last M64 Beta tomorrow, so would like to ensure only critical and absolutely safe merges are in.

Comment 6 by ericrk@chromium.org, Jan 16 2018

Cc: abdulsyed@chromium.org
abdulsyed@, The fix should be extremely safe, and was tested on ToT. I'll spend some time this morning doing additional testing on today's Canary to verify that everything is as expected and will update here. when is the merge cutoff for tomorrow's beta?
Labels: -Merge-Review-64 Merge-Approved-64
Thanks! I'm pre-approving this merge for now (branch:3282). Can you please merge after you're done with additional testing/verifications and if things look good? Please merge before 5PM PDT to make it to tomorrow's release. 

Comment 8 by ericrk@chromium.org, Jan 16 2018

Labels: -Hotlist-Merge-Review -ReleaseBlock-Stable -Merge-Approved-64
Status: Fixed (was: Started)
After additional testing, we don't believe this bug hittable with the code that exists in M64 (repro case was a WIP CL on ToT, which was believed to generalize to current code, but turns out to not do so). Removing merge request.

Sign in to add a comment