Force image-decoding before resolving createImageBitmap promise

Force image-decoding before resolving createImageBitmap promise

In the case of createImageBitmap(HTMLImageElement) without any cropRect
and ImageBitmapOptions parameters, the promise seems to be resolved before
image-decoding is done.

This CL changes such that image-decoding is forced to happen before the
promise is resolved.

BUG=595090
Committed: https://crrev.com/1247e32994c8f828f3a22ce8dde5b4631d142763
Cr-Commit-Position: refs/heads/master@{#382662}

[xidachen, 2016-03-15 20:24:53]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org

[xidachen, 2016-03-15 20:24:53]

PTAL

[Justin Novosad, 2016-03-17 21:27:21]

Could you add a simple benchmark that isolates the run time of calling drawImage
with a newly created ImageBitmap?

https://codereview.chromium.org/1800153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1800153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:141: OwnPtr<uint8_t[]>
imagePixels = copySkImageData(croppedSkImage, info);
Making a full copy, only to discard it after is wasteful. I understand that the
skia API is too opaque to trigger an immediate decode by any other means, but I
think you could read just a 1x1 pixel subregion.

[xidachen, 2016-03-22 16:01:38]

On 2016/03/17 21:27:21, Justin Novosad wrote:
> Could you add a simple benchmark that isolates the run time of calling
drawImage
> with a newly created ImageBitmap?
> 

I do have perf tests to measure 2 things:
1). createImageBitmap(HTMLImageElement) with/out this CL
2). drawImage from the created ImageBitmap with/out this CL.

The two perf are here:
https://codereview.chromium.org/1818803002/: with this CL
https://codereview.chromium.org/1818793002/: without this CL

What I saw is that the createImageBitmap(HTMLImageElement) degrades about 25%
with the change in this CL. But drawImage() doesn't have any improvement. Does
this make sense?

[Justin Novosad, 2016-03-22 18:50:27]

On 2016/03/22 16:01:38, xidachen wrote:
> On 2016/03/17 21:27:21, Justin Novosad wrote:
> > Could you add a simple benchmark that isolates the run time of calling
> drawImage
> > with a newly created ImageBitmap?
> > 
> 
> I do have perf tests to measure 2 things:
> 1). createImageBitmap(HTMLImageElement) with/out this CL
> 2). drawImage from the created ImageBitmap with/out this CL.
> 
> The two perf are here:
> https://codereview.chromium.org/1818803002/: with this CL
> https://codereview.chromium.org/1818793002/: without this CL
> 
> What I saw is that the createImageBitmap(HTMLImageElement) degrades about 25%
> with the change in this CL. But drawImage() doesn't have any improvement. Does
> this make sense?

The benchmark does not capture the improvement because keeps re-using the same
image bitmap, so you only get hit with the deffered decode cost on the first
iteration.

[Justin Novosad, 2016-03-22 18:53:09]

Making a banchmark that correctly isolates the behaviour we're trying to measure
is probably overkill anyways.
lgtm

[xidachen, 2016-03-22 18:55:31]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 18:56:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800153003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800153003/20001

[commit-bot: I haz the power, 2016-03-22 20:12:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-22 20:12:15]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-03-22 20:12:37]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-03-22 20:13:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800153003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800153003/20001

[commit-bot: I haz the power, 2016-03-22 20:22:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-22 20:24:46]

Description was changed from

==========
Force image-decoding before resolving createImageBitmap promise

In the case of createImageBitmap(HTMLImageElement) without any cropRect
and ImageBitmapOptions parameters, the promise seems to be resolved before
image-decoding is done.

This CL changes such that image-decoding is forced to happen before the
promise is resolved.

BUG=595090
==========

to

==========
Force image-decoding before resolving createImageBitmap promise

In the case of createImageBitmap(HTMLImageElement) without any cropRect
and ImageBitmapOptions parameters, the promise seems to be resolved before
image-decoding is done.

This CL changes such that image-decoding is forced to happen before the
promise is resolved.

BUG=595090

Committed: https://crrev.com/1247e32994c8f828f3a22ce8dde5b4631d142763
Cr-Commit-Position: refs/heads/master@{#382662}
==========

[commit-bot: I haz the power, 2016-03-22 20:24:47]

Patchset 2 (id:??) landed as
https://crrev.com/1247e32994c8f828f3a22ce8dde5b4631d142763
Cr-Commit-Position: refs/heads/master@{#382662}

[Justin Novosad, 2016-03-22 20:35:46]

+scroggo
Should have asked earlier, but do you know of a cleaner way of doing this?
Basically, we want createImageBitmap to force an immediate decode, so that once
we have a resolved ImageBitmap object, we have a guarantee that it is in a
pre-decoded state. Right now, we have an SkImage that may be using deferred
decode, and we could not think of a cleaner way to trigger an immediate decode
than to peek a pixel.

[scroggo_chromium, 2016-03-23 14:06:57]

On 2016/03/22 20:35:46, Justin Novosad wrote:
> +scroggo
> Should have asked earlier, but do you know of a cleaner way of doing this?
> Basically, we want createImageBitmap to force an immediate decode, so that
once
> we have a resolved ImageBitmap object, we have a guarantee that it is in a
> pre-decoded state. Right now, we have an SkImage that may be using deferred
> decode, and we could not think of a cleaner way to trigger an immediate decode
> than to peek a pixel.

The way we typically trigger a decode in SkImage is using preroll() [1]. That
said, it does not guarantee that the SkImage remains decoded - this particular
SkImage has a DecodingImageGenerator, which will decode into either the
SkResourceCache, which uses discardable memory (if the image is a single frame
image that is complete on the first call to decode) or into its own cache (an
ImageFrame on an ImageDecoder in the ImageDecodingStore). Either way, it's
possible that the image will be thrown out of the cache.

vmpstr@ is working on a way to ensure that the image the compositor uses is
locked when it comes time to draw, and as I understand it, it will copy the
image (actually scale it to the desired size) into memory that is guaranteed to
stick around until it is no longer needed [2]. I don't know if that kind of
approach makes sense for this case. 

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
[2]
https://docs.google.com/document/d/13UsG1IVEIqRg5yaQ9ZmF7dXQprI6KCrSy82CK7Xwfkw/

[Justin Novosad, 2016-03-23 14:17:12]

On Wed, Mar 23, 2016 at 10:06 AM, <scroggo@chromium.org> wrote:

> On 2016/03/22 20:35:46, Justin Novosad wrote:
> > +scroggo
> > Should have asked earlier, but do you know of a cleaner way of doing
> this?
> > Basically, we want createImageBitmap to force an immediate decode, so
> that
> once
> > we have a resolved ImageBitmap object, we have a guarantee that it is in
> a
> > pre-decoded state. Right now, we have an SkImage that may be using
> deferred
> > decode, and we could not think of a cleaner way to trigger an immediate
> decode
> > than to peek a pixel.
>
> The way we typically trigger a decode in SkImage is using preroll() [1].
> That
> said, it does not guarantee that the SkImage remains decoded - this
> particular
> SkImage has a DecodingImageGenerator, which will decode into either the
> SkResourceCache, which uses discardable memory (if the image is a single
> frame
> image that is complete on the first call to decode) or into its own cache
> (an
> ImageFrame on an ImageDecoder in the ImageDecodingStore). Either way, it's
> possible that the image will be thrown out of the cache.
>
> vmpstr@ is working on a way to ensure that the image the compositor uses
> is
> locked when it comes time to draw, and as I understand it, it will copy the
> image (actually scale it to the desired size) into memory that is
> guaranteed to
> stick around until it is no longer needed [2]. I don't know if that kind of
> approach makes sense for this case.
>
> [1]
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> [2]
>
>
https://docs.google.com/document/d/13UsG1IVEIqRg5yaQ9ZmF7dXQprI6KCrSy82CK7Xwfkw/
>
> https://codereview.chromium.org/1800153003/
>

Thanks. This is very useful information.  I think preroll() makes a lot of
sense.  The contract that ImageBitmap needs to fulfill is to guarantee that
the image is can be drawn "without undue latency".  The spec is
deliberately vague to give implementations some freedom. Having images in a
pre-decoded state is ideal, but making them subject to eviction in low
memory conditions also makes sense, to avoid OOM crashes.
Xida, mind changing this code to use preroll()?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2016-03-23 14:17:14]

On Wed, Mar 23, 2016 at 10:06 AM, <scroggo@chromium.org> wrote:

> On 2016/03/22 20:35:46, Justin Novosad wrote:
> > +scroggo
> > Should have asked earlier, but do you know of a cleaner way of doing
> this?
> > Basically, we want createImageBitmap to force an immediate decode, so
> that
> once
> > we have a resolved ImageBitmap object, we have a guarantee that it is in
> a
> > pre-decoded state. Right now, we have an SkImage that may be using
> deferred
> > decode, and we could not think of a cleaner way to trigger an immediate
> decode
> > than to peek a pixel.
>
> The way we typically trigger a decode in SkImage is using preroll() [1].
> That
> said, it does not guarantee that the SkImage remains decoded - this
> particular
> SkImage has a DecodingImageGenerator, which will decode into either the
> SkResourceCache, which uses discardable memory (if the image is a single
> frame
> image that is complete on the first call to decode) or into its own cache
> (an
> ImageFrame on an ImageDecoder in the ImageDecodingStore). Either way, it's
> possible that the image will be thrown out of the cache.
>
> vmpstr@ is working on a way to ensure that the image the compositor uses
> is
> locked when it comes time to draw, and as I understand it, it will copy the
> image (actually scale it to the desired size) into memory that is
> guaranteed to
> stick around until it is no longer needed [2]. I don't know if that kind of
> approach makes sense for this case.
>
> [1]
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> [2]
>
>
https://docs.google.com/document/d/13UsG1IVEIqRg5yaQ9ZmF7dXQprI6KCrSy82CK7Xwfkw/
>
> https://codereview.chromium.org/1800153003/
>

Thanks. This is very useful information.  I think preroll() makes a lot of
sense.  The contract that ImageBitmap needs to fulfill is to guarantee that
the image is can be drawn "without undue latency".  The spec is
deliberately vague to give implementations some freedom. Having images in a
pre-decoded state is ideal, but making them subject to eviction in low
memory conditions also makes sense, to avoid OOM crashes.
Xida, mind changing this code to use preroll()?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.