Bug 93590

Summary: Add NonImmediateBitmapImage to mark it is safe to decode asynchronously.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: ImagesAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: hclam, jamesr, nick, simon.fraser, skyul, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 93467, 94190    
Bug Blocks: 90375, 90935    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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.