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

Issue 603353 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 597791



Sign in to add a comment

WebP Image Encoding is delaying compositor frames on the engine.

Project Member Reported by khushals...@chromium.org, Apr 14 2016

Issue description

This trace was collected on the engine for google.com.

It can be seen that the serialization of the commit messages is taking ~400 ms. This is due to the WebP encoding for each image referenced in the SkPicture.

The long term solution would be with asynchronous image loading so the image encodes don't block the commit message. In the short term we should use the SHA1 of the raw data and avoid re-encoding the images on the engine in subsequent messages.
 
chrometrace.log
60.4 KB View Download

Comment 1 by klo...@chromium.org, Apr 14 2016

Thanks.Assuming this is also the reason why it took 900ms for cnn.com first paint to show. Can we do anything in the short time? 
Yup. If a page has lot of images, this overhead gets added to all of them. Since the sending of images has to be synchronous, not sure we can do anything right now...

Comment 3 by klo...@chromium.org, Apr 14 2016

Assume this is already on your powerful machine.

Can we look at the encoding code to see anything we can do there?

Also do we have a bug for async encoding?
Thanks for the tip. Looks like there is a configuration that the WebPEncoder is using to tweak the time taken by the encoding algorithm. Dropping it down to the lowest value has reduced the total deserialization time to ~70 ms. Attaching a trace.
I'll just upload that change.
chrometrace.log
257 KB View Download
Cc: -khushals...@chromium.org nyquist@chromium.org
Owner: khushals...@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 14 2016

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

commit f2aab581ec1f14450be730478ee715cc084544af
Author: khushalsagar <khushalsagar@chromium.org>
Date: Thu Apr 14 20:59:30 2016

blimp: Reduce WebP encoding time on the engine.

The WebP Encoding during commit serialization blocks the render thread
for close to 300 ms and significantly adds on to the time taken to push
a frame to the client.
Use the config that ensures the lowest encoding time. This reduced the
encode time for an image on google.com from ~215 ms to ~53ms.

BUG= 603353 

Review URL: https://codereview.chromium.org/1889763002

Cr-Commit-Position: refs/heads/master@{#387421}

[modify] https://crrev.com/f2aab581ec1f14450be730478ee715cc084544af/blimp/engine/renderer/engine_image_serialization_processor.cc

A separate bug has been opened to move the encode work off the render thread when asynchronous image transfer is supported. (crbug/603643)
Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 18 2016

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

commit a2796c212636511499f47597e76865d7577cd1c8
Author: nyquist <nyquist@chromium.org>
Date: Mon Apr 18 20:58:17 2016

Initial version of Blimp BlobCache.

Currently, every time there is a commit from engine to client, all
pictures are sent again, leading to poor performance and high
bandwidth usage.

This CL adds a cache on the client side that never clears out any
received resources. This means that the engine can depend on that
the client has all resources it has previously sent, and uses this fact
to only send down a unique identifier (SHA1 hash of the image) and the
the width and height in place of the image itself. This is wrapped in
the new proto BlobCacheImageMetadata. If this would be a new image for
the client, the proto also includes the payload of the image. This
payload will be removed when the BlobChannel is fully integrated.

The identifier is calculated based on the source image, to ensure that
no encode is required on the engine when the same image is seen again.

To ensure that the cache is used for all images, this changes the
behavior of the serializer to also encode images that were previously
encoded as WebP, to ensure that they become cacheable.

BUG= 597807 ,  600719 ,  603353 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1867653002

Cr-Commit-Position: refs/heads/master@{#388013}

[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/client/feature/compositor/decoding_image_generator.cc
[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/BUILD.gn
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/blob_cache.h
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/id_util.cc
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/id_util.h
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/in_memory_blob_cache.cc
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/in_memory_blob_cache.h
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/blob_cache/in_memory_blob_cache_unittest.cc
[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/compositor/webp_decoder.cc
[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/proto/BUILD.gn
[add] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/common/proto/blob_cache.proto
[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/engine/BUILD.gn
[modify] https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8/blimp/engine/renderer/engine_image_serialization_processor.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 25 2016

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

commit 5344a6eebfbd65e6a86f2274c69e74ac6ccfe138
Author: nyquist <nyquist@chromium.org>
Date: Mon Apr 25 23:29:24 2016

[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

Blimp currently uses SHA-1 to uniquely identify resources. There was
no specific reason for this other than the goal of having a unique
ID, so this changes it to instead use SHA-256 from //crypto, since it
is in general better. This does however change the length of the
hash from 20 bytes to 32 bytes.

This concern was raised as a drive-by comment on
https://codereview.chromium.org/1867653002/

This CL also adds functionality to verify that a specific BlobId has
the correct length, and removes all references to SHA1 outside of
the //blimp/common/blob_cache/id_util.[cc|h] files.

In addition, this CL adds a test to ensure that there is certainty
that this unique ID only ever changes intentionally.

BUG= 597807 ,  600719 ,  603353 

Review URL: https://codereview.chromium.org/1912153004

Cr-Commit-Position: refs/heads/master@{#389608}

[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/BUILD.gn
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/DEPS
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/blob_cache/id_util.cc
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/blob_cache/id_util.h
[add] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/blob_cache/id_util_unittest.cc
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/blob_cache/in_memory_blob_cache.cc
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/common/compositor/webp_decoder.cc
[modify] https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138/blimp/engine/renderer/engine_image_serialization_processor.cc

Labels: Archive-Blimp

Sign in to add a comment