Bug 93590 - Add NonImmediateBitmapImage to mark it is safe to decode asynchronously.
Summary: Add NonImmediateBitmapImage to mark it is safe to decode asynchronously.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 93467 94190
Blocks: 90375 90935
  Show dependency treegraph
 
Reported: 2012-08-09 01:05 PDT by Dongseong Hwang
Modified: 2014-02-05 10:52 PST (History)
6 users (show)

See Also:


Attachments
Patch (26.30 KB, patch)
2012-08-09 01:24 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (25.12 KB, patch)
2012-08-10 04:47 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (25.10 KB, patch)
2012-08-15 23:47 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-08-09 01:05:10 PDT
Because image decoding is used in various contexts in WebKit, it is simply
impossible to make all decoding asynchronous. Instead, we selectively use async
image decoders where possible and use the existing sequential image decoders otherwise.
RetainedModeBitmapImage wraps a BitmapImage and helps decide when it is safe and beneficial
to use async image decoders.
Comment 1 Dongseong Hwang 2012-08-09 01:24:45 PDT
Created attachment 157418 [details]
Patch
Comment 2 Nick Carter 2012-08-09 14:24:23 PDT
While I understand the intended analogy to retained/immediate graphics APIs, the "RetainedMode" designation doesn't seem to describe what actually happens. 

I'd prefer this name indicate more clearly that the decode may happen in the future, and not complete before the draw operation.

I think the dichotomy that exists here is not between "Immediate/Retained", but "Immediate/Eventual".

Some ideas: FutureBitmapImage, DelayedBitmapImage, EventualBitmapImage.
Comment 3 Hin-Chung Lam 2012-08-09 15:56:10 PDT
Comment on attachment 157418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157418&action=review

> Source/WebCore/platform/graphics/RetainedModeBitmapImage.cpp:1
> +/*

I think a better name would be NonImmediateBitmapImage. It is unclear from the name what RetainedModeBitmapImage does.

> Source/WebCore/platform/graphics/RetainedModeBitmapImage.cpp:40
> +    , m_retainedModeCount(0)

This can be a simple boolean instead of counter.

> Source/WebCore/platform/graphics/RetainedModeBitmapImage.cpp:46
> +    RetainedModeSaver retainedModeSaver(this);

There's no need to have this because this method doesn't trigger image decoding.

> Source/WebCore/platform/graphics/RetainedModeBitmapImage.cpp:106
> +    RetainedModeSaver retainedModeSaver(this);

It will be more clear to only have this line in this method and draw().

> Source/WebCore/platform/graphics/RetainedModeBitmapImage.cpp:148
> +RetainedModeBitmapImage* toRetainedModeBitmapImage(Image* image)

Looks like there's no user of this function other than toRetainedModeBitmapImageIfPossible, I'd combine them.
Comment 4 Dongseong Hwang 2012-08-10 04:47:42 PDT
Created attachment 157704 [details]
Patch
Comment 5 Dongseong Hwang 2012-08-10 04:49:08 PDT
(In reply to comment #2)
> Some ideas: FutureBitmapImage, DelayedBitmapImage, EventualBitmapImage.

I agree with you. Thanks for good suggestions.
Comment 6 Dongseong Hwang 2012-08-10 04:51:56 PDT
(In reply to comment #3)
> I think a better name would be NonImmediateBitmapImage. It is unclear from the name what RetainedModeBitmapImage does.

Ok, I changed.

> This can be a simple boolean instead of counter.

Absolutely!

> There's no need to have this because this method doesn't trigger image decoding.

Yes.

> It will be more clear to only have this line in this method and draw().

Now only NonImmediateBitmapImage::draw() and NonImmediateBitmapImage::nativeImageForCurrentFrame() have this line.

> Looks like there's no user of this function other than toRetainedModeBitmapImageIfPossible, I'd combine them.

Done.
Comment 7 Dongseong Hwang 2012-08-15 23:47:49 PDT
Created attachment 158722 [details]
Patch
Comment 8 Dongseong Hwang 2012-08-15 23:48:23 PDT
(In reply to comment #7)
> Created an attachment (id=158722) [details]
> Patch

Rebased to the upstream.
Comment 9 Andreas Kling 2014-02-05 10:52:10 PST
Comment on attachment 158722 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.