New issue
Advanced search Search tips

Issue 870222 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Flashing when displaying composited image

Project Member Reported by jakearchibald@chromium.org, Aug 2

Issue description

Canary Android.

A few of us are building a little image compression app https://squoosh-beta.firebaseapp.com/, and I'm seeing some weird graphics glitches in Canary Android.

To reproduce:
1. https://squoosh-beta.firebaseapp.com/.
2. Open large image (one attached).
3. Pinch zoom until you can see the whole image.
4. Move the red handle to reveal more/less of the original image.

Trace of issue attached.

Video of issue: https://www.youtube.com/watch?v=aqjq1Tz5084&feature=youtu.be
 
IMG_8024.jpg
1.4 MB View Download
trace_glitch.json.gz
3.3 MB Download
Components: -Blink>Compositing Internals>Compositing>Images
Labels: -Pri-3 Pri-2
Confirmed. It's worse than that. Sometimes the image completely disappears or portions of it are not painted at all.

It's also present in 69.0.3497.9. I was testing on a Pixel 2.
Cc: pbomm...@chromium.org
Labels: ReleaseBlock-Stable M-69 69 Needs-Bisect Target-69
Status: Available (was: Untriaged)
I can repro as well and tried disabling some obvious optimizations, OOP-raster, GPU-raster, partial-raster and skip-clipped-image-ops but I could still repro. This is a regression in M69, I verified that this works correctly on 68.0.3440.70.

Requesting a bisect from the test team.
Cc: aluo@chromium.org
cc'ing Andrew for further triage and bisecting of the issue.
Owner: prashanthpola@chromium.org
please bisect, thanks.
Labels: -Type-Bug -Needs-Bisect -69 hasbisect-per-revision FoundIn-69 Type-Bug-Regression
Owner: junov@chromium.org
Status: Assigned (was: Available)
We are able to repro the issue on Chrome:69.0.3497.24 Device:Pixel 2/8.0.1 with the steps mentioned in C#1

Bisect info:
Good build:69.0.3489.0
Bad build:69.0.3491.0
Regression range:
https://chromium.googlesource.com/chromium/src/+log/69.0.3489.0..69.0.3491.0?pretty=fuller&n=10000

Good commit:574613
Bad commit:574614
Culprit CL: 
https://chromium.googlesource.com/chromium/src/+/f8b8872f2080130c2b5bdf026f32f46aad7598da
Components: -Internals>Compositing>Images Blink>Canvas
Owner: fs...@chromium.org
junov@ isn't working on chromium anymore. Passing to fserb@ for canvas triaging.
Cc: fs...@chromium.org
Owner: ----
Status: Available (was: Assigned)
Cc: benmason@chromium.org
Owner: fs...@chromium.org
Status: Assigned (was: Available)
Cc: jakearchibald@chromium.org
Can still repro on ToT.

If I disable "will-change:transform", then the bug disappears.

I've been having a hard time trying to pinpoint the problem with this.

Is there any chance you can provide a simpler example where this happen, or at least a non-minified javascript?

Components: Blink>Paint
So, we figured out the problem with this CL.
It had the undesired side-effect of slightly changing the logic of which type of backing buffer we use on Canvas.

I'm sending a fix to this, but although this solves the example above there's still the underlying problem.

It seem that if you have a RAM bitmap that is very big (4032 x 3024 in the example above) and that bitmap is put in its own layer, sometimes paint only draws a part of it, on Android.

I'm going to try to force canvas to use bitmap arrays before that review and see if I can bisect again.


Here's a frame viewer trace. There's a huge texturelayer and a huge picturelayer. My guess is that we're running out of memory.
trace_frameviewer.json.gz
26.3 MB Download
I've tried to bisect the original problem (forcing canvas to always use memory bitmaps), but I can't get chrome android to run past ~2000 CLs ago. :/
At that point, the bug was still happening. 
So, what's happening is:

With a GPU backed canvas, we appear to be hitting a fast path where we directly composite the canvas image (no need for intermediate raster).

With a SW backed canvas, we seem to be generating a normal PictureLayer. PictureLayers require tiling and rasterization. Unfortunately, we use will-change:transform as a hint to always rasterize at "native" size and scale from there. Because of the size of the image / canvas, and the pixel density of the device, the native size is:

(3000 * 2.5) * (4000 * 2.5) * 4 = 300MB.

This exceeds our tile memory limit, meaning that we cannot rasterize the PictureLayer at native scale and give up half way through.

If we remove will-change:transform (even if we keep the canvas as its own layer with translateZ), the issue goes away, as we always re-rasterize at display resolution.

I remember a bug from a while ago where we debated the correct behavior here and decided that will-change:transform should be a signal that we want to rasterize once (at native size) and scale. 

I wonder if we need additional logic to avoid rasterizing at native scale if that scale is too large (exceeds memory budget).

I think pdr@ and vmpstr@ worked on the original logic - do you have any thoughts here?
Cc: vmp...@chromium.org pdr@chromium.org
Owner: ericrk@chromium.org
Adding more people here.
Does anyone think we could have a solution to be merged to 69?
Cc: enne@chromium.org
I'm not sure this should be release-block stable because it has only been reported on an in-development app, is only caused by extremely large sizes, and there is a workaround.

Enne, at lunch we talked about this briefly. Could we use the same logic we do for directly-composited images? IIRC there's some difference between directly-composited images and the codepath being hit here, or maybe a difference between regular composited content and the codepath being hit here.
pdr: I don't think we can do the same thing for PictureLayer that we do for PictureImageLayer.  For web content, it's important to raster at exact scales for cripsness reasons, rather than have the tiered levels of zoom.

I think this is just part of the heuristic of "will-change: transform" vs other ways of promoting composited layers.  This all comes back to https://chromium-review.googlesource.com/c/chromium/src/+/546580 and the extremely long discussion in  issue 596382 .  This is why we have the logic for will-change: transform as compared to "normal" PictureLayers.

The only thing I can think of is that maybe we could add some hacky additional heuristic on top that says if a will-change layer ever doesn't have memory for its tiles, then to ignore the will-change heuristic that bounds the scale we use for it.  This doesn't seem very pretty either.

Picking the correct scale to balance out performance/memory/quality is extremely complicated, and every time we've changed it, we've had to iron out a lot of wrinkles over time.  I think the future solution here is for slimming paint to move layerization to the compositor thread so that we can un-promote these composited layers and make better performance/memory tradeoffs.
Owner: fs...@chromium.org
I think we should merge the Canvas change fserb@ mentions in #11 - that should put us back to our M68 state. Given that this isn't a regression if we merge the change mentioned in #11, I think we can remove RBS and look at improving this out of band.

Back to fserb@ to see if we can merge that CL.

In terms of improving this, I agree with enne@ - this is something we should think about, but I don't know that there's an immediate easy fix.
It's a very simple fix but it's extremely brittle.
If this demo had a couple more canvases it would break again.
But sure, we can merge.
I agree this probably shouldn't be a release blocker though. WDYT?
Labels: -ReleaseBlock-Stable
This does not seem like a release blocker.
Components: -Blink>Canvas
Owner: ----
Status: Available (was: Assigned)
I'd rather not merge this, given that is not a proper fix. Is everyone ok with that?
We fixed this by removing will-change, so it isn't blocking us https://github.com/GoogleChromeLabs/squoosh/pull/232/files.
Owner: fs...@chromium.org
Status: Assigned (was: Available)
fserb@, could you give an update on the current status? The comment stream leaves me confused.
Owner: ----
Status: Available (was: Assigned)
We decided to not merge the temporary fix, since it was not a proper fix and we agreed it was not a particularly bad regression. Therefore we removed the release blocker.

We found the underlying problem, as pointed out by comment #14 and discussed some solutions (#16, #17). 

This is still a P2 paint bug.
Owner: chrishtr@chromium.org
Status: Assigned (was: Available)
I think we should probably ignore the will-change scale if it is so large that it exceeds memory limits. Will follow up on this to discuss further with the GPU team.

Sign in to add a comment