Canary 65 w/ default flags looks even worse, the Composite Layers step (which looks to actually be the commit lock) takes *800ms* of main thread time.

With "AsyncImageDecoding" enabled it gets rid of the 800ms main thread slices for me, though it still doesn't pan smoothly like Safari or Firefox which get 60fps here.

The AsyncImageDecoding change is also confusing to me since it's not putting that 800ms slice on another thread, it's just gone entirely. The raster also becomes much faster. That makes it seem like Chrome is doing something bad with image decoding even ignoring the async flag which appears to have more side effects than just checkering. I don't see any checkerboarding when I turn it on either.

Why is the compositor holding the main thread for 800ms in the lock?

Deleted: Canary65NoFlags.json.zip 1.1 MB

Canary65NoFlags.json.zip 1.1 MB Download

Deleted: HammerAsyncImage.json.zip 1.1 MB

HammerAsyncImage.json.zip 1.1 MB Download

Deleted: trace_Canary65Trace.json.gz 1.8 MB

trace_Canary65Trace.json.gz 1.8 MB Download

This also looks like a 2x regression in 65. The commit lock was being held for 400ms in  63, and now 800ms in 65.

Oof. On my chromebook, the demos are really painful. It locks up the machine so much that the mouse cursor doesn't move for 5-20 seconds.

The source image is 15110 × 1872 pixels, so it's likely blowing our image cache limits, or possibly falling in a slow path.

Beta is ok in GPU mode on linux, dev is not ok. Bisecting now.

Eric bisected the issue back to a Skia patch, linked below. It looks like
we are (a) falling back to Skia for some reason, and in this case (b) it ends
up going into a code path where the entire source image (15000x1800) in order
to color-correct it.

https://skia.googlesource.com/skia.git/+/8b1360dcfab9ae748361268284068049cd3796a7

junov@, brianosman@ or mtklein@, can you take a look below? It feels like we want a Skia side fix here, but we may want a CC patch to harden things as well.

After digging into this, the problem occurs because of the following sequence of events:

1) If CC decides there are no images in a draw, it will skip creating an image_provider (caching support), see https://cs.chromium.org/chromium/src/cc/tiles/tile_manager.cc?rcl=017f0ceef514623148a9d79cd186250835c3b6cc&l=1233
2) In general, CC relies on the SkCanvas to quick-reject operations which should be clipped, see comment here. https://cs.chromium.org/chromium/src/cc/paint/paint_op_buffer.cc?rcl=89ae89fb0062ebb3d7944bdb8814d87e9169b23c&l=2163
3) When using an SkColorSpaceXformCanvas, we will upload/color convert any images *before* checking for clipping / quick rejection (see PrepareImage call): https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkColorSpaceXformCanvas.cpp?rcl=9daae51c4ad84b1d6364a7f6415621f12b232a41&l=147
4) This means that in the case where we are doing color conversion, we will upload / color convert *any* image passed to DrawImageRect/DrawImage, whether or not the draw op is completely clipped.

It feels like SkColorSpaceXformCanvas should skip upload and color conversion for images in DrawImage type ops which are completely clipped.

Even with this, it feels like there is a dangerous coupling between CC/Skia. CC may want to handle this in a more deterministic way, ensuring we never pass Skia an image we don't want it to decode (even if the we expect the image to be clipped).

This was present in M63, removing RBS, but I still think we should try to get a simple fix into 64 if possible.

>It feels like SkColorSpaceXformCanvas should skip upload and color conversion for images in DrawImage type ops which are completely clipped.

Is this the sort of thing you were thinking?  https://skia-review.googlesource.com/c/skia/+/85044

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/d096e0d126bfe11aa33886124103ad128b807693

commit d096e0d126bfe11aa33886124103ad128b807693
Author: Mike Klein <mtklein@chromium.org>
Date: Thu Dec 14 16:35:01 2017

quick-reject before transforming images

Transforming images is expensive, and pointless if they're clipped out.

Bug:  chromium:794690 

Change-Id: Iffa4f6c60275caf310b8327e083b8857018621c2
Reviewed-on: https://skia-review.googlesource.com/85044
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Brian Osman <brianosman@google.com>

[modify] https://crrev.com/d096e0d126bfe11aa33886124103ad128b807693/src/core/SkColorSpaceXformCanvas.cpp

Thanks for the super quick turnaround! I'll test this out and also look at hardening CC a bit.

Khushal is taking a look.

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

commit fe9adb97d4c4ca7ee44130198d39d18af146b299
Author: Khushal <khushalsagar@chromium.org>
Date: Fri Dec 15 00:48:46 2017

cc: Always create PlaybackImageProvider for raster playback.

The PlaybackImageProvider is required for using the compositor's image
decode cache during raster. Currently we conditionally create it if the
images are known to exist on a tile from discardable image analysis.
This results in an expectation for skia to implement some optimizations
assumed during analysis. Instead always create the provider to ensure
more predictable behaviour.

R=ericrk@chromium.org

Bug:  794690 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2e21a88697fd400b43eb54fa6b63e73c591592d5
Reviewed-on: https://chromium-review.googlesource.com/826691
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524264}
[modify] https://crrev.com/fe9adb97d4c4ca7ee44130198d39d18af146b299/cc/tiles/tile_manager.cc

I'm still seeing the bug with the change in #16. Its definitely not a clipping issue. I enabled always quick rejecting of ops in cc, before passing it to skia, and there are still multiple chunks of lengthy image decodes when going through a color transform canvas. What's surprising is that these are for the same image. Here is a trace attached, look at generator_id on DecodingImageGenerator which is unique for each image. There are multiple decodes of the same generator id even during raster for the same tile? 0_0

The change in #19 does fix it, because with cc's cache we avoid all these multiple decodes. Could this be a case where we got the analysis wrong, which is why we didn't detect the image to be on a tile and didn't predecode it? Because I can see a decode task running on a background worker thread and the same image decoded at raster. Here is another trace which shows this.

Deleted: trace_cc_cache_color_correct.json.gz 2.7 MB

trace_cc_cache_color_correct.json.gz 2.7 MB Download

Deleted: trace_color_correct_skia.json.gz 1.0 MB

trace_color_correct_skia.json.gz 1.0 MB Download

The change in #19 fixes the bug and should be safe to merge to 64.

This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review
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

Any test you can add along with the cl in #19? 

Also, have you verified the issue is fixed in canary?

Yes. Its fixed on the latest canary (65.0.3300.0) and  https://chromium-review.googlesource.com/c/chromium/src/+/839540 is adding a test for the fix.

Approving merge to M64 Chrome OS.

Reminder to please merge CL to M64 branch 3282.

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

commit e37303da3760eb6787bccadeb844a64dc0fd4081
Author: Khushal <khushalsagar@chromium.org>
Date: Wed Jan 03 19:03:55 2018

cc: Always create PlaybackImageProvider for raster playback.

The PlaybackImageProvider is required for using the compositor's image
decode cache during raster. Currently we conditionally create it if the
images are known to exist on a tile from discardable image analysis.
This results in an expectation for skia to implement some optimizations
assumed during analysis. Instead always create the provider to ensure
more predictable behaviour.

R=​ericrk@chromium.org

Bug:  794690 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2e21a88697fd400b43eb54fa6b63e73c591592d5
Reviewed-on: https://chromium-review.googlesource.com/826691
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#524264}(cherry picked from commit fe9adb97d4c4ca7ee44130198d39d18af146b299)
Reviewed-on: https://chromium-review.googlesource.com/849232
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#401}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/e37303da3760eb6787bccadeb844a64dc0fd4081/cc/tiles/tile_manager.cc

I've forked this into issue 798796 for multiple decodes in skia and 798798 for potentially incorrect image position analysis in cc.

Unable to reproduce this issue on MacBookAir 10.12.6, Windows-10 and Ubuntu 14.04 using chrome latest M64 #64.0.3282.85 by following steps mentioned in the original comment. Able to drag the pan smoothly left and right without any lag.

Note: Unable to reproduce this issue on reported version of chrome #63.0.3239.84 as well. Attaching screen cast for reference. Could any one let us know how to reproduce this issue to verify it from Chrome-TE end. 

Thanks!

Deleted: 794690.mp4 402 KB

	794690.mp4 402 KB View Download

You need 2 things to be able to repro this. GPU raster and color conversion.

1) For GPU raster, you can look up GPU rasterization in chrome://flags and force enable it. Please verify that you see "Rasterization: Hardware accelerated" in chrome://gpu.

2) For color conversion, go to chrome://flags and look for "Force color profile". Select any profile other than default.

Seems like "try it" section has been removed from http://hammerjs.github.io/. So unable to verify the above targeted fix.