Issue metadata
Sign in to add a comment
|
Flashing when displaying composited image |
||||||||||||||||||||
Issue descriptionCanary 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
,
Aug 2
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.
,
Aug 2
cc'ing Andrew for further triage and bisecting of the issue.
,
Aug 2
please bisect, thanks.
,
Aug 2
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
,
Aug 2
junov@ isn't working on chromium anymore. Passing to fserb@ for canvas triaging.
,
Aug 13
,
Aug 15
,
Aug 16
,
Aug 17
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?
,
Aug 17
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.
,
Aug 17
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.
,
Aug 17
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.
,
Aug 17
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?
,
Aug 20
Adding more people here. Does anyone think we could have a solution to be merged to 69?
,
Aug 20
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.
,
Aug 20
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.
,
Aug 20
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.
,
Aug 20
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.
,
Aug 20
I agree this probably shouldn't be a release blocker though. WDYT?
,
Aug 20
This does not seem like a release blocker.
,
Aug 21
I'd rather not merge this, given that is not a proper fix. Is everyone ok with that?
,
Oct 29
We fixed this by removing will-change, so it isn't blocking us https://github.com/GoogleChromeLabs/squoosh/pull/232/files.
,
Oct 29
fserb@, could you give an update on the current status? The comment stream leaves me confused.
,
Oct 29
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.
,
Nov 24
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 |
|||||||||||||||||||||
Comment 1 by schenney@chromium.org
, Aug 2Labels: -Pri-3 Pri-2