[chromium] Implement deferred image decoding for WebKit Chromium

• Component: Images
• Status: RESOLVED FIXED

[Hin-Chung Lam]

Objective of this feature is to record a page rendering into a SkPicture without having to decode all images. Decoding is done in a later and decoded image is consumed on impl thread during rasterization phase.

[Hin-Chung Lam]

Created attachment 162595 [details]
Patch

[Hin-Chung Lam]

Created attachment 162596 [details]
Patch

[Hin-Chung Lam]

I have uploaded a temporary patch to show current progress of deferred image decoding. Patch is incomplete and still pretty rough. I haven't updated it for 2 weeks. I'm focused in fixing 95121 which is a blocking issue for this change as it will simplifies scaled image caching.

[Hin-Chung Lam]

Created attachment 162971 [details]
Patch

[Hin-Chung Lam]

The latest patch is closer to review with all major problems solved. I have also added unit tests and layout tests. This change still depends on 95121 and 96135.

[James Robinson]

Comment on attachment 162971 [details]
Patch

Could you move the settings / initialization code out of CC / WebCompositor?  This is a concern of how an image is painted into the SkCanvas, which isn't really a concern of the compositor - WebCore is responsible for painting stuff.  The system doing the raster doesn't need to know about this directly - does it?

[Hin-Chung Lam]

Created attachment 163224 [details]
Patch

[Hin-Chung Lam]

Updated patch without use of CCSettings. One step closer.

[Dongseong Hwang]

(In reply to comment #7)
> Created an attachment (id=163224) [details]
> Patch

Hi, Alpha. It's a great work.

Deffered image decoding seems not to need a callback like ImageSourceObserver. I think this approach is good. There may be two key points.
1. ImageDecoderChromium::frameBufferAtIndex does not decode and return a ImageFrame including LazyDecodingPixelRef.
2. When LazyDecodingPixelRef::onLockPixels is called (i.e. replay SkPicture), we decode an image lazily.

This approach is so reasonable to me.

However, I have some questions.

1. Why do we need ImageDecodingStore? I think it is ok for ImageDecoderChromium::frameBufferAtIndex just to create SkBitmap including LazyDecodingPixelRef.
2. This prototype is not thread safe. I think Chrome need to play SkPicture off the main thread. Do you have plan to make it thread-safe?

[Hin-Chung Lam]

Created attachment 165161 [details]
Patch

[Hin-Chung Lam]

Created attachment 165162 [details]
Patch

[Hin-Chung Lam]

James: This patch is good for review. It depends on 95121 before this can land, 95121 depends on skia change but otherwise is good and stable. Since this is a big patch I would like to start the process for this while waiting for skia fix to land.

[Hin-Chung Lam]

Thanks for looking. 

This patch is the first of a series of changes to enable deferred image decoding. This patch intends to land the basic structure and do just deferring while lazily decoding on the main thread. Answers to your questions are below.

> 
> However, I have some questions.
> 
> 1. Why do we need ImageDecodingStore? I think it is ok for ImageDecoderChromium::frameBufferAtIndex just to create SkBitmap including LazyDecodingPixelRef.

We plan to centralize the job dispatching and caching of bitmap images in ImageDecodingStore. I think it can do a better job than MemoryCache because it knows what images and which portion is needs once it is operated on the impl thread together with the compositor. Later on there will be a lot of features built into the ImageDecodingStore.

> 2. This prototype is not thread safe. I think Chrome need to play SkPicture off the main thread. Do you have plan to make it thread-safe?

Yes. Chrome now only playback SkPicture on the main thread so this is safe.

The plan is to:
1. Create an instance of ImageDecodingStore on the impl thread.
2. Synchronize ImageDecodingStore on the main thread and impl thread. More like a push model from main thread to impl thread.
3. Push encoded data from main thread to impl thread. This is the first step though we want to share SharedBuffer on both threads but that will need some extra effort.

[Hin-Chung Lam]

Created attachment 165165 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> The plan is to:
> 1. Create an instance of ImageDecodingStore on the impl thread.
> 2. Synchronize ImageDecodingStore on the main thread and impl thread. More like a push model from main thread to impl thread.
> 3. Push encoded data from main thread to impl thread. This is the first step though we want to share SharedBuffer on both threads but that will need some extra effort.

Thanks for answers.

All sounds good!.

[James Robinson]

Comment on attachment 165165 [details]
Patch

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

Sorry it's taken so long to get to this.  I've left a bunch of comments, mostly about style and sticking to WebKit conventions.  There are also some layering violations to address.

The one big design question I have is the design of ImageDecoderChromium.  It closely mirrors ImageDecoder's interface and embeds an ImageDecoder but otherwise has no relationship to ImageDecoder in the type system.  All of the public functions on ImageDecoderChromium mimic public virtual functions on ImageDecoder AFAICT.  What is the plan here?  Do you intend to make ImageDecoderChromium a subclass of ImageDecoder?  If so, why doesn't this patch do that?  If not, why does it look so closely related?

> Source/WebKit/chromium/ChangeLog:39
> [phone]  Alpha Lam  <[e-mail]>

you have a double changelog here

> Source/WebCore/page/Settings.h:435
> +#if PLATFORM(CHROMIUM)

if you have a setting that's only used in chromium, put it on WebKit::WebSettings (which is chromium-specific) not WebCore::Settings (which is not)

> Source/WebCore/platform/graphics/ImageSource.h:56
> +#if USE(SKIA) && PLATFORM(CHROMIUM)

USE(SKIA) && PLATFORM(CHROMIUM) is redundant. chromium always sets USE(SKIA)

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:30
> +#include "Settings.h"

this is a layering violation.  Settings is a WebCore/page concept and this is in WebCore/platform - the rule for /platform is it can only depend on WTF and other things in WebCore/platform but shouldn't know about anything above that

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:53
> +    ImageDecoder* actualDecoder = ImageDecoder::create(data, alphaOption, gammaAndColorOption);
> +    if (!actualDecoder)
> +        return 0;
> +    return new ImageDecoderChromium(actualDecoder);

use OwnPtr<> / PassOwnPtr<> for the ImageDecoder here, not raw. When using WebCore objects it's extremely rare (and extremely suspicious) to be dealing with raw pointers unless there is no ownership transfer taking place

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:64
> +    if (m_actualDecoder.get())

why are we null checking m_actualDecoder in this function? it seems that it can't be null, looking at the ::create()

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:110
> +    } else {
> +        ASSERT(!m_data.get());

i don't see how we could get into this branch

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:119
> +    if (m_actualDecoder.get())

no .get() needed here or on any of the other m_actualDecoder null checks

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.h:44
> +    // Used in test only. Ownership of decoder is transferred.
> +    static ImageDecoderChromium* create(ImageDecoder*);

If ownership is transfered encode that in the type i.e. use a PassOwnPtr<ImageDecoder> instead of a raw one

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:63
> +    WTF::deleteAllValues(m_frameGenerators);
> +    WTF::deleteAllValues(m_frameCache);

instead of these, why not make m_frameGenerators and m_frameCache have OwnPtr<>s as values?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:66
> +// static

we don't typically use this type of comment in WebKit code

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:92
> +SkBitmap ImageDecodingStore::createLazyDecodedSkBitmap(WTF::PassOwnPtr<ImageDecoder> decoder)

no WTF:: needed. <wtf/PassOwnPtr.h> has a using statement to put PassOwnPtr<> in the global namespace - that's the common pattern for all WTF types

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:107
> +    bitmap.pixelRef()->setURI(labelLazyDecoded);

this seems odd - in what sense is "lazy" a URI?  why do pixelrefs have URIs?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:123
> +    // Note: Because row bytes is fixed to SkBitmap this means we have to scale the
> +    // entire image.

I can't quite follow what this comment is saying

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:199
> +    if (this == instanceOnMainThread() && WTF::isMainThread())

no WTF::

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:200
> +        return true;

why not just "return this == instanceOnMainThread && isMainThread();" ?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:204
> +void ImageDecodingStore::createFrameGenerator(WTF::PassOwnPtr<ImageDecoder> decoder)

no WTF:: - I won't comment on the rest but please find+remove them all

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:208
> +    ImageFrameGenerator* generator = new ImageFrameGenerator(decoder);
> +    ++m_nextId;
> +    m_frameGenerators.add(m_nextId, generator);

what's the ownership of the ImageFrameGenerator?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:237
> +            delete m_frameCache[i];

if m_frameCache had OwnPtr values you wouldn't need this explicit delete. smart pointers are great, use 'em!

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:53
> +    if (m_decoder.get())

just if (m_decoder) - WTF smart pointers all have operator bool()s implementations that do the right thing

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.h:41
> +    ImageFrameGenerator(PassOwnPtr<ImageDecoder>);

explicit

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:32
> +SK_DECLARE_GLOBAL_MUTEX(gLazyDecodingImageRefMutex);

this looks like it creates a static initializer and destructor. double-yuck!  why do we have to use a skia macro here?

we don't name globals with "gFooBar" in webkit - if it's a static member variable do s_fooBar, otherwise just fooBar

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.h:39
> +class LazyDecodingPixelRef : public SkPixelRef {

SkPixelRef means it's implicit SkRefCnt'd.  Could we try to keep it in SkAutoTUnref's when it's not owned by another object?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:150
> +#if PLATFORM(CHROMIUM)
> +        void setSkBitmap(const SkBitmap& bitmap)

it feels a bit weird to make ImageDecoder even more aware of skia, but i guess it already knows about SkBitmap in a few ways

[Hin-Chung Lam]

(In reply to comment #16)
> (From update of attachment 165165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165165&action=review
> 
> Sorry it's taken so long to get to this.  I've left a bunch of comments, mostly about style and sticking to WebKit conventions.  There are also some layering violations to address.
> 
> The one big design question I have is the design of ImageDecoderChromium.  It closely mirrors ImageDecoder's interface and embeds an ImageDecoder but otherwise has no relationship to ImageDecoder in the type system.  All of the public functions on ImageDecoderChromium mimic public virtual functions on ImageDecoder AFAICT.  What is the plan here?  Do you intend to make ImageDecoderChromium a subclass of ImageDecoder?  If so, why doesn't this patch do that?  If not, why does it look so closely related?

Thanks for looking!

I'll cleanup the style and layering violations later and focus on ironing the bigger design questions around ImageDecoderChromium.

Here's some background information.

A platform image decoder is accessed through a NativeImageSourcePtr type (defined as a typedef), CG and other platforms have different implementations.

ImageDecoder interface is used by all non-CG ports and currently:
typedef ImageDecoder* NativeImageSourcePtr; // for non-CG
typedef CGImageSourceRef NativeImageSourcePtr; // for CG

ImageSource is used by BitmapImage to utilize an image decoder. ImageSource access this NativeImageSourcePtr not through a type system but method names, which are implemented by CGImageSourceRef and ImageDecoder.

The design here for Chromium is to have ImageDecoderChromium NOT inherits from ImageDecoder. The objective of having this extra type is to:

1. Be a bridge to delicate calls to the actual image decoder, i.e. subclasses of ImageDecoder
2. Be a bridge to delicate calls to the deferred image decoding system, i.e. ImageDecodingStore
3. Provide a factory method for ImageSource to construct a NativeImageSourcePtr

I specifically do not want ImageDecoderChromium to inherit from ImageDecoder because there's too many other things (member variables and methods) defined in ImageDecoder not related to the above objectives. I also want to avoid ImageDecoderChromium being a subclass of ImageDecoder and keeps a reference to another subclass of ImageDecoder (the actual image decoder say for JPEG, GIF, etc). So I concluded that I want to have bridge layer be its own type.

The name of ImageDecoderChromium seems to be confusing and I can rename it to ImageDecodingBridge so it is not confused with ImageDecoder.

I also do not want to pack the functionalities of ImageDecoderChromium into ImageSource, This avoids having a duplicate implementation of ImageSource and again have a clear bridge layer.

[Hin-Chung Lam]

James: Any comments on the suggestions I posted?

[Hin-Chung Lam]

James: Would like to get more feedback before fixing the patch with style. More specifically about the design of ImageDecoderChromium. Do you think the rationale on comments #17 makes sense? Should I rename the class to ImageDecodingBridge to avoid confusion? Please advice.

[James Robinson]

That plan sounds OK I suppose.  I can look in detail when there's a patch to look at.

[Hin-Chung Lam]

Created attachment 167232 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 165165 [details]
Patch

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

Address comments from James. Patch won't compile because it needs to roll DEPS on chromium that I'll do separately. Otherwise it builds locally and ran all webkit_unit_tests and layout tests without failures.

>> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:30
>> +#include "Settings.h"
> 
> this is a layering violation.  Settings is a WebCore/page concept and this is in WebCore/platform - the rule for /platform is it can only depend on WTF and other things in WebCore/platform but shouldn't know about anything above that

Removed.

>> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:53
>> +    return new ImageDecoderChromium(actualDecoder);
> 
> use OwnPtr<> / PassOwnPtr<> for the ImageDecoder here, not raw. When using WebCore objects it's extremely rare (and extremely suspicious) to be dealing with raw pointers unless there is no ownership transfer taking place

There's a bug filed about making ImageDecoder::create() return PassOwnPtr<> but that not in the scope of this patch. See: https://bugs.webkit.org/show_bug.cgi?id=97039
 
ImageDecoder now returns a raw pointer already so this factory method does the same to match. It needs a separate refactoring to clean that up.

>> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:64
>> +    if (m_actualDecoder.get())
> 
> why are we null checking m_actualDecoder in this function? it seems that it can't be null, looking at the ::create()

See line 82. When an image is deferred the ownership of ImageDecoder is transfered to ImageDecodingStore and is not referenced by this object any more.

>> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:110
>> +        ASSERT(!m_data.get());
> 
> i don't see how we could get into this branch

See line 82. When an image is deferred the ownership of ImageDecoder is transfered to ImageDecodingStore and is not referenced by this object any more. In that case we set data to ImageDecodingStore.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:107
>> +    bitmap.pixelRef()->setURI(labelLazyDecoded);
> 
> this seems odd - in what sense is "lazy" a URI?  why do pixelrefs have URIs?

This is used as an identifier that the SkPixelRef referenced by SkBitmap is of  type LazyDecodingPixelRef. URI is optional and is generally not used, we use it here as type identifier. Added comments to explain this.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:123
>> +    // entire image.
> 
> I can't quite follow what this comment is saying

Removed this comments to avoid confusion. This means that SkBitmap has the row bytes of the entire scaled image and thus when we prepare a SkPixelRef we can't just scale a portion but have to provide the full scaled image.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:208
>> +    m_frameGenerators.add(m_nextId, generator);
> 
> what's the ownership of the ImageFrameGenerator?

Changed it to OwnPtr in m_frameGenerators. They are owned by this object.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:237
>> +            delete m_frameCache[i];
> 
> if m_frameCache had OwnPtr values you wouldn't need this explicit delete. smart pointers are great, use 'em!

Done! :)

>> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:32
>> +SK_DECLARE_GLOBAL_MUTEX(gLazyDecodingImageRefMutex);
> 
> this looks like it creates a static initializer and destructor. double-yuck!  why do we have to use a skia macro here?
> 
> we don't name globals with "gFooBar" in webkit - if it's a static member variable do s_fooBar, otherwise just fooBar

We don't really need this actually, removed.

>> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.h:39
>> +class LazyDecodingPixelRef : public SkPixelRef {
> 
> SkPixelRef means it's implicit SkRefCnt'd.  Could we try to keep it in SkAutoTUnref's when it's not owned by another object?

Good catch! This class is never referenced by objects other than SkBitmap but I did find I forgot to call setPixelRef(new LazyDecodingPixelRef(...))->unref().

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:150
>> +        void setSkBitmap(const SkBitmap& bitmap)
> 
> it feels a bit weird to make ImageDecoder even more aware of skia, but i guess it already knows about SkBitmap in a few ways

You're right ImageDecoder does know about SkBitmap, in fact look at ImageDecoderSkia.cpp for Skia port this class is written around SkBitmap.

[Hin-Chung Lam]

Created attachment 167233 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 167233 [details]
Patch

Attachment 167233 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14181212

[Peter Beverloo (cr-android ews)]

Comment on attachment 167233 [details]
Patch

Attachment 167233 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14152776

[WebKit Review Bot]

Comment on attachment 167233 [details]
Patch

Attachment 167233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14175361

[Build Bot]

Comment on attachment 167233 [details]
Patch

Attachment 167233 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14152796

[Hin-Chung Lam]

Created attachment 167262 [details]
Patch

[Build Bot]

Comment on attachment 167262 [details]
Patch

Attachment 167262 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14171601

[Peter Beverloo (cr-android ews)]

Comment on attachment 167262 [details]
Patch

Attachment 167262 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14176406

[WebKit Review Bot]

Comment on attachment 167262 [details]
Patch

Attachment 167262 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14152852

New failing tests:
platform/chromium/virtual/deferred/fast/images/paletted-png-with-color-profile.html

[Build Bot]

Comment on attachment 167262 [details]
Patch

Attachment 167262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14180362

[Hin-Chung Lam]

Created attachment 167362 [details]
Patch

[Hin-Chung Lam]

Fixed compile warnings and marked a test with missing results. Bots should be green now.

[Build Bot]

Comment on attachment 167362 [details]
Patch

Attachment 167362 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14171837

[Hin-Chung Lam]

Created attachment 167364 [details]
Patch

[Hin-Chung Lam]

Fixed Mac CG port.

[Hin-Chung Lam]

Created attachment 167377 [details]
Patch

[Hin-Chung Lam]

Merged again... touching too many files.. :-/

[Hin-Chung Lam]

Woohoo all green now!

[Hin-Chung Lam]

Any updates on this patch? It is difficult to move forward without this patch reviewed and landed.

[Stephen White]

Comment on attachment 167377 [details]
Patch

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

Preliminary comments; more to come.

> Source/WebCore/ChangeLog:14
> +        When used in conjunction with per-tile painting this feature defer

Grammar nits:

defer -> defers

> Source/WebCore/ChangeLog:15
> +        image decoding until SkCanvas referencing the image is rasterized.

until SkCanvas -> until the SkCanvas

> Source/WebCore/ChangeLog:23
> +        drawing BitmapImage into a GraphicsContext it makes a request to

drawing BitmapImage -> drawing a BitmapImage

> Source/WebCore/ChangeLog:24
> +        create NativeImageSkia. We substitute the content in NativeImageSkia

create NativeImageSkia -> create a NativeImageSkia.

> Source/WebCore/ChangeLog:27
> +        ImageDecodingBridge

<bikeshed>Would it make sense to call this DeferredImageDecoder instead?  (I realize it may not always defer).</bikeshed>

> Source/WebCore/ChangeLog:29
> +        This is the platform implementation of a image decoder for Chromium.

a image decoder -> an image decoder

> Source/WebCore/ChangeLog:31
> +        ImageDecoder or create a lazily decoded SkBitmap and delegates calls

or create a lazily decoded -> or creates a lazily-decoded

> Source/WebCore/ChangeLog:37
> +        data and corresponding ImageDecoder for doing actual decoding. It is

and corresponding -> and the corresponding

> Source/WebCore/platform/graphics/ImageSource.h:51
> +#elif !USE(SKIA) || !PLATFORM(CHROMIUM)

These #ifdefs don't match the ones used below.  IIRC, Blackberry uses #USE(SKIA), but not PLATFORM(CHROMIUM).  Will the below code still work?  (Sadly, there is no EWS bot for Blackberry AFAIK).

> Source/WebCore/platform/graphics/ImageSource.h:58
> +typedef ImageDecodingBridge NativeImageSourceFactory;

<bikeshed> It seems that NativeImageSourceFactory is really more an image decoder (or wrapper that defers decoding).  Perhaps it should be called NativeImageDecoder instead?</bikeshed>

> Source/WebCore/platform/graphics/chromium/ImageDecodingBridge.cpp:92
> +    return &m_lazyDecodedFrame;

This seems dangerous.  Are you always certain that the returned frame is destroyed or copied before the ImageDecodingBridge?  OTOH, it looks like the regular decoders do this too, since they serve ImageFrame*'s out of cache.

[Hin-Chung Lam]

Comment on attachment 167377 [details]
Patch

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

>> Source/WebCore/ChangeLog:27
>> +        ImageDecodingBridge
> 
> <bikeshed>Would it make sense to call this DeferredImageDecoder instead?  (I realize it may not always defer).</bikeshed>

I went back and forth with this and settled with this at the end. If needed we can rename this in a later patch.

>> Source/WebCore/platform/graphics/ImageSource.h:51
>> +#elif !USE(SKIA) || !PLATFORM(CHROMIUM)
> 
> These #ifdefs don't match the ones used below.  IIRC, Blackberry uses #USE(SKIA), but not PLATFORM(CHROMIUM).  Will the below code still work?  (Sadly, there is no EWS bot for Blackberry AFAIK).

I changed this to !PLATFORM(CHROMIUM) this should work for all non-chromium and non-cg platforms.

>> Source/WebCore/platform/graphics/ImageSource.h:58
>> +typedef ImageDecodingBridge NativeImageSourceFactory;
> 
> <bikeshed> It seems that NativeImageSourceFactory is really more an image decoder (or wrapper that defers decoding).  Perhaps it should be called NativeImageDecoder instead?</bikeshed>

Note that "NativeImageSourceFactory" is just a factory it doesn't need to be a decoder. See ImageSource.cpp:85. Also the native image decoder is called "NativeImageSourcePtr" so I think NativeImageSource + Factory is a better name.

We can rename all these to be better, e.g. NativeImageDecoderPtr and NativeImageDecoderFactory but that can be done afterwards.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingBridge.cpp:92
>> +    return &m_lazyDecodedFrame;
> 
> This seems dangerous.  Are you always certain that the returned frame is destroyed or copied before the ImageDecodingBridge?  OTOH, it looks like the regular decoders do this too, since they serve ImageFrame*'s out of cache.

ImageFrame is never cached by caller. All usage of ImageFrame looks like this:
ImageFrame *frame = decoder->frameBufferAtIndex(0);
frame->asNewNativeImage();

So I think this is safe.

[Hin-Chung Lam]

Created attachment 168622 [details]
Patch

[Adam Barth]

Comment on attachment 168622 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ImageDecodingBridge.h:44
> +    // Use only in test.
> +    static PassOwnPtr<ImageDecodingBridge> create(PassOwnPtr<ImageDecoder>);

Please rename this function to something that indicates that it should only be used during testing.  For example, "createForTesting"

[Adam Barth]

Comment on attachment 168622 [details]
Patch

I'm sorry if I missed this in the discussion, but can you include information in the ChangeLog about which benchmarks get faster any by how much?  Do any benchmarks get slower?  (I'm assuming the goal here is to improve performance---if there are other goals, it would be helpful to explain them in the ChangeLog as well.)

I'm marking this patch R- for now because this patch doesn't explain the relative benefits and costs of this patch.  I haven't reviewed the actual code change in detail.

[Hin-Chung Lam]

Created attachment 168629 [details]
Patch

[Hin-Chung Lam]

Thanks for looking.

I have updated the ChangeLog to include the rationale of this change.

This change does not improve (or degrade) performance but an architectural change to enable impl-side painting, which will improve performance by doing rasterization on multiple threads.

[Adam Barth]

Thanks.  It sounds like we might want to review this change in two parts:

1) jamesr or enne should provide feedback as to whether we want to make this architectural change and as to whether this particular design integrates well with other impl-side processing.

2) If the reviewer for (1) doesn't feel comfortable reviewing the image decoding details, we can ask an image decoding expert, such as Noel Gordon or pkasting, to review that aspect of the change.

[Adam Barth]

Stephen, I certainly don't mean to exclude you.  It sounds like you're interested in providing feedback as well.  :)

[James Robinson]

(In reply to comment #50)
> Thanks.  It sounds like we might want to review this change in two parts:
> 
> 1) jamesr or enne should provide feedback as to whether we want to make this architectural change and as to whether this particular design integrates well with other impl-side processing.

Yes we do and it does.

> 
> 2) If the reviewer for (1) doesn't feel comfortable reviewing the image decoding details, we can ask an image decoding expert, such as Noel Gordon or pkasting, to review that aspect of the change.

That would be excellent - I don't know all that much about the specifics of the image decoder guts.

[Hin-Chung Lam]

> > 2) If the reviewer for (1) doesn't feel comfortable reviewing the image decoding details, we can ask an image decoding expert, such as Noel Gordon or pkasting, to review that aspect of the change.
> 
> That would be excellent - I don't know all that much about the specifics of the image decoder guts.

Let me ping pkasting to look at the image decoder part and Stephen to look at the Skia (SkPixelRef) part.

[Peter Kasting]

(In reply to comment #53)
> Let me ping pkasting to look at the image decoder part and Stephen to look at the Skia (SkPixelRef) part.

At a glance it looks like most code is new rather than a modification of existing code.  So I'm not sure what all you want me to look at.  Tell me specifically and I'll take a look.

Note that I am not a WebKit reviewer so my signoff doesn't mean anything official.

[Hin-Chung Lam]

pkasting: Please look at these two files:

1. ImageDecodingBridge.cpp

This file implements a bridge to generate defer-decoded images from ImageDecodingStore.

2. ImageDecodingStore.cpp

This file is where the image decoding happens using a WebKit ImageDecoder.

They are in fact new implementation that is based on existing image decoding architecture in WebKit. I think your experience with this area is very useful for the review of this patch.


senorblanco: Please look at these two files:

1. LazyDecodingPixelRef.cpp

Implementation of SkPixelRef which calls ImageDecodingStore to lazily to decode an image.

2. ImageDecodingStore.cpp

Please look at how lazy decoding is done.

[Mike Reed]

Comment on attachment 168629 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        Both recording and rasterization happen on the main thread.

How does this line jive with "This feature enables ... on multiple threads". ?

> Source/WebCore/ChangeLog:45
> +        This feature is disabled by default.

Is it important that this detail have an on/off switch? Is it just this way for while, til we're sure it works, or are there times when we really don't want this?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:105
> +    // Use URI to identify this is a lazily decoded SkPixelRef of type LazyDecodingPixelRef.

The intent (but not enforced) is for the URI to try to identify the specific image (e.g. the web-address for this image). This use is fine, but if we have the url somewhere, that would be more meaningful.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:107
> +    bitmap.setImmutable(); // Prevents SkPicture from recording pixels.

setImmutable(); // Inform the bitmap that we will never change the pixels. This is a performance hint subsystems that may try to cache this bitmap (e.g. pictures, pipes, gpu, pdf, etc.)

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:108
> +    return bitmap;

Can we also mark the bitmap as opaque (if it is)? setIsOpaque(). This is a big perf win, but only if the bitmap is flagged. See ImageFrameSkia.cpp for sites that make this call.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:125
> +    return resizedBitmap;

Similar comments here about setImmutable and setIsOpaque (see createLazyDecodedSkBitmap).

[Stephen White]

Comment on attachment 167377 [details]
Patch

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

>>> Source/WebCore/ChangeLog:27
>>> +        ImageDecodingBridge
>> 
>> <bikeshed>Would it make sense to call this DeferredImageDecoder instead?  (I realize it may not always defer).</bikeshed>
> 
> I went back and forth with this and settled with this at the end. If needed we can rename this in a later patch.

No, I'd prefer we get the name correct now.  You describe it as "the platform implementation of a image decoder for Chromium."  That suggests to me it would be better described as a DeferredImageDecoder, no?  I'm open to other suggestions (DeferredImageDecodingBridge?), I just find the "Bridge" name alone to be close to meaningless.

>>> Source/WebCore/platform/graphics/ImageSource.h:58
>>> +typedef ImageDecodingBridge NativeImageSourceFactory;
>> 
>> <bikeshed> It seems that NativeImageSourceFactory is really more an image decoder (or wrapper that defers decoding).  Perhaps it should be called NativeImageDecoder instead?</bikeshed>
> 
> Note that "NativeImageSourceFactory" is just a factory it doesn't need to be a decoder. See ImageSource.cpp:85. Also the native image decoder is called "NativeImageSourcePtr" so I think NativeImageSource + Factory is a better name.
> 
> We can rename all these to be better, e.g. NativeImageDecoderPtr and NativeImageDecoderFactory but that can be done afterwards.

As above, I'd prefer you get the names right before landing.  It'll save you doing SCM/gyp file renames later too.

I still think what both classes have in common is decoding.  Yes, all decoders are factories, but we don't have any factories yet which aren't decoders, so my preference would be to stick to existing concepts.

[Hin-Chung Lam]

Upload a new patch to address your comments in a bit.

> How does this line jive with "This feature enables ... on multiple threads". ?
> 
> > Source/WebCore/ChangeLog:45
> > +        This feature is disabled by default.
> 
> Is it important that this detail have an on/off switch? Is it just this way for while, til we're sure it works, or are there times when we really don't want this?

This is the same for other WebKit features. On top of that this feature is incomplete so turned off by default now and only run with layout tests.

> 
> > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:105
> > +    // Use URI to identify this is a lazily decoded SkPixelRef of type LazyDecodingPixelRef.
> 
> The intent (but not enforced) is for the URI to try to identify the specific image (e.g. the web-address for this image). This use is fine, but if we have the url somewhere, that would be more meaningful.

It requires significant plumbing to get the actual URI here so I put a FIXME there. Right now this is just to identify the type is LazyDecodingPixelRef.

I'm open to suggestions for better ways to identify types of SkPixelRef.

> 
> > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:107
> > +    bitmap.setImmutable(); // Prevents SkPicture from recording pixels.
> 
> setImmutable(); // Inform the bitmap that we will never change the pixels. This is a performance hint subsystems that may try to cache this bitmap (e.g. pictures, pipes, gpu, pdf, etc.)
> 
> > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:108
> > +    return bitmap;
> 
> Can we also mark the bitmap as opaque (if it is)? setIsOpaque(). This is a big perf win, but only if the bitmap is flagged. See ImageFrameSkia.cpp for sites that make this call.
> 
> > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:125
> > +    return resizedBitmap;
> 
> Similar comments here about setImmutable and setIsOpaque (see createLazyDecodedSkBitmap).

It doesn't know until the image is actually decoded.

There are cases that we can guess but I'll leave this optimization for later. Left a FIXME in the code.

[Stephen White]

Comment on attachment 168629 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        This feature enables playback and decoding strategy that runs on
> +        multiple threads.

Out of curiosity, does this mean that we might be decoding the same image on different playback threads?  Does the current design make that safe (and performant)?

> Source/WebCore/ChangeLog:33
> +        An image decoding operation is recorded as a SkPixelRef object
> +        implemented by LazyDecodingPixelRef. This object references raw encoded
> +        data, regions to be decoded and scaling information.

Another design question (apologies if this is addressed by the code; I haven't grokked it all yet):  how does this handle the same image drawn at multiple scale factors?  Will that cause multiple re-decodes in some circumstances?  Something like the Flying Images demo might be a good performance benchmark for this use case.

> Source/WebCore/ChangeLog:74
> +        This object manages all encoded images. It keeps track of encoded
> +        data and the corresponding ImageDecoder for doing actual decoding. It
> +        is also responsible for generating lazily decoded SkBitmaps. This
> +        SkBitmap contains a LazyDecodingPixelRef object which references to an
> +        image entry in ImageDecodingStore.

Ugh.  Is it really necessary to add yet another layer of cacheing?  Is there a way this could be combined with (or subsume) the single layer of "cache" instead NativeImageSkia?

(My longer-term hope was that we could make good quality resizing fast enough that we don't need to do any cacheing at all, although it's not fair to make you wait for that.  It also might be incompatible with the goal of reducing memory usage in the resize-once-to-small-size use case.)

In the interim, I wonder if we could decouple the resize cacheing from the decode cacheing, so that we could use your new cache in place of the NativeImageSkia "cache", irrespective of whether deferred decoding was on or off.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:67
> +ImageDecodingStore* ImageDecodingStore::instanceOnMainThread()

Should this function also be asserting that it's called on the main thread?  It's public, and I see that NativeImageSkia and ImageDecodingBridge both call it without asserts.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:72
> +void ImageDecodingStore::initializeOnMainThread()

Same here.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:103
> +    bitmap.setPixelRef(new LazyDecodingPixelRef(m_nextId, fullSize, fullRect))->unref();

Note that there's already a generation ID in SkPixelRef (getGenerationID()) which seems to do exactly what m_nextId does here.  You might consider using that instead of rolling your own.

[Mike Reed]

duh -- good point about isopaqueness : we don't know (at least for some image types) until we actually decode.

[Mike Reed]

un-duh -- we should definitely mark as TODO to consider decoding just enough to know if the image has per-pixel alpha (e.g. jpeg never does, etc.). If we wait until drawing, then it may be too late to influence which blitter we choose.

[Nick Carter]

If we know the width and height (a prereq for having pixel data), then we know the image format, and in many cases we can predict that the bitmap will be opaque. But it's important to remember that a half-received and half-decoded JPEG image may have a transparent bottom half, even though JPEG doesn't support alpha. I'm a fan of eventually moving to an approach where the decoder tracks the "valid region" of an image, in which case we treat the image as opaque even sooner (it also might speed up pixel buffer initialization).

[Hin-Chung Lam]

> Out of curiosity, does this mean that we might be decoding the same image on different playback threads?  Does the current design make that safe (and performant)?

We are probably not doing this. Decode task is per image and we haven't implemented the multithreading part yet.

> Another design question (apologies if this is addressed by the code; I haven't grokked it all yet):  how does this handle the same image drawn at multiple scale factors?  Will that cause multiple re-decodes in some circumstances?  Something like the Flying Images demo might be a good performance benchmark for this use case.

This code handles same-image-multiple-scale multiple scale by caching the decoded image in original size and scale on demand.

We need some data to back the best caching scheme / strategy and as you can see this patch doesn't have a complete caching scheme. I intentionally leave it blank for later patches.
 
> Ugh.  Is it really necessary to add yet another layer of cacheing?  Is there a way this could be combined with (or subsume) the single layer of "cache" instead NativeImageSkia?
> 
> (My longer-term hope was that we could make good quality resizing fast enough that we don't need to do any cacheing at all, although it's not fair to make you wait for that.  It also might be incompatible with the goal of reducing memory usage in the resize-once-to-small-size use case.)
> 
> In the interim, I wonder if we could decouple the resize cacheing from the decode cacheing, so that we could use your new cache in place of the NativeImageSkia "cache", irrespective of whether deferred decoding was on or off.

When this is turned on NativeImageSkia doesn't do caching and caching is handled centrally by ImageDecodingStore.

I noticed that NativeImageSkia does caching itself, my plan is to obsolete that when this feature is ready. This feature doesn't need to depend on impl thread and operate just as well on the main thread. Which means we can "defer" decoding on the main thread and use the new cache in place of caching in NativeImageSkia.

One goal for this feature is to centralize all image cache (in ImageDecodingStore), give it all the interesting information (i.e. image, scaling info, regions) such that we can come up with the best caching / decoding scheme. 

> 
> Should this function also be asserting that it's called on the main thread?  It's public, and I see that NativeImageSkia and ImageDecodingBridge both call it without asserts.
> Same here.

Done.
 
> Note that there's already a generation ID in SkPixelRef (getGenerationID()) which seems to do exactly what m_nextId does here.  You might consider using that instead of rolling your own.

ID in ImageDecodingStore identifies an image while SkPixelRef identifies (image, scale, region).

[Hin-Chung Lam]

Created attachment 168759 [details]
Patch

[Hin-Chung Lam]

(In reply to comment #61)
> un-duh -- we should definitely mark as TODO to consider decoding just enough to know if the image has per-pixel alpha (e.g. jpeg never does, etc.). If we wait until drawing, then it may be too late to influence which blitter we choose.

I've added FIXME for this. Yes when an image is deferred we already what image format it is and whether it is fully loaded and can make a good guess.

[Mike Reed]

Skia isn't ready to allow a given pixelref (marked immutable) to change its mind about opaqueness. However, I don't think that will be a problem for us...

If we know the width/height, then we can know the opaqueness.
We also know if we have all of the data or not.

Given these two, it seems we can set the opaque bit as follows:

bitmap.setPixelRef(data);
if (format_is_opaque && we_have_all_the_data)
    bitmap.setIsOpaque(true);

If we don't have all the data, we won't mark it opaque. When we finally do get all the data, we *have* to make a new pixelRef, at which time we can mark its opaqueness.

[Hin-Chung Lam]

> Given these two, it seems we can set the opaque bit as follows:
> 
> bitmap.setPixelRef(data);
> if (format_is_opaque && we_have_all_the_data)
>     bitmap.setIsOpaque(true);

Agreed. I left a FIXME in the code that describes just this.

> When we finally do get all the data, we *have* to make a new pixelRef, at which time we can mark its opaqueness.

When we have all the data and image is recorded to SkPixelRef the image is not yet fully decoded. So for PNG, GIF, WebP+alpha we need to wait until the image is decoded (rasterized) and only during next record that we can make a new PixelRef and mark it opaqueness safely. This is also noted in the FIXME.

[Hin-Chung Lam]

(In reply to comment #63)
> > Out of curiosity, does this mean that we might be decoding the same image on different playback threads?  Does the current design make that safe (and performant)?
> 
> We need some data to back the best caching scheme / strategy and as you can see this patch doesn't have a complete caching scheme. I intentionally leave it blank for later patches.
> 

By blank I mean a simple implementation: cache the full size, scale on demand, no eviction. This is left for later patches to implement a better scheme.

[Hin-Chung Lam]

Created attachment 168773 [details]
Patch

[Hin-Chung Lam]

Patch updated with Mike and Stephen's comments addressed. Renamed ImageDecodingBridge to DeferredImageDecoder and NativeImageSouceFactory to NativeImageDecoder.

[Peter Kasting]

Comment on attachment 168773 [details]
Patch

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

I don't really feel like I understand what's going on here, partly since I haven't taken the time to really look at and think about the entire change as opposed to just a couple of files.  If you need a serious, in-depth review, I'll need to think longer and harder about everything the patch does.  (And probably learn more about how Skia works too.)

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:49
> +    if (!actualDecoder)

Nit: Or just

    return actualDecoder ? new DeferredImageDecoder(actualDecoder) : 0;

(Similar comment applies numerous other places in this file)

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:69
> +    // because a multiframe is usually animated GIF. Animation is handled by

Sometimes it's an ICO, which can probably be deferred successfully.  Perhaps we should try to distinguish these, or be more aggressive here, or something?

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:70
> +    // BitmapImage which uses some metadata functions that does synchronous

Nit: does -> do

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:86
> +        // the value doesn't matter.

Nit: This comment is confusing.  "predict this correct" -> "correct" should be "correctly", and what is "this" referring to?  Then the last sentence says this doesn't matter anyway??

Maybe you meant something like:

  // Don't mark the frame as completely decoded until the underlying decoder has really decoded it.  Until then, our data and metadata may be incorrect, so callers can't rely on them.

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:98
> +        // Keep a reference to data until WebLazyDecodingImage is created, at

What class is this?  I don't see this in this patch...

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:107
> +        // thread here.

I have absolutely no idea what this comment means.

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:115
> +    return true;

Shouldn't this be false?

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:131
> +    // uniform size.

What does "and have uniform size" mean?  A single frame's size is inherently "uniform".  But the image size and the frame size could in theory differ (I think... e.g. for an ICO with one image, whose size is marked as different from the overall image size?  Test this).

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h:38
> +class DeferredImageDecoder {

It seems like this is implementing most of the public API on ImageDecoder.  This suggests that perhaps we ought to have some (partly?-)abstract interface that looks about like this class, which this and ImageDecoder both implement.  That makes callers able to use either via polymorphism and ensures the actual functions exposed on each stay in sync.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:54
> +    s_instanceOnMainThread = imageDecodingStore;

This leaks if someone calls setInstanceOnMainThread() with non-NULL more than once.  Either delete unconditionally or assert that |imageDecodingStore| is only non-NULL when |s_instanceOnMainThread| is NULL.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:89
> +        && !memcmp(bitmap.pixelRef()->getURI(), labelLazyDecoded, sizeof(labelLazyDecoded));

This seems like a hack.  Should we mark such bitmaps some other way, e.g. by keeping a set of all the live lazy bitmap*s, or by adding a field on some relevant object to directly reflect if decoding is lazy?

Or perhaps we can get by without this by reworking how resizing works?  I dunno.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:136
> +    // Inform the bitmap that we will never change the pixels. This is a performance hint

Nit: Avoid copy/paste, just say:

    // See comments in createLazyDecodedSkBitmap().
    resizedBitmap.setImmutable();
    return resizedBitmap;

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:154
> +    ASSERT(calledOnValidThread());

If your header comments say you can only lock one thing at a time, you should ASSERT that that's true here, e.g. by checking that m_lockedSkBitmap is NULL.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:187
> +        m_lockedSkBitmap.lockPixels();

Nit: Factor this call out to below the conditional

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:205
> +    return this == instanceOnMainThread() && isMainThread();

Nit: Checking that we're the instanceOnMainThread() seems unnecessary given the API in the header.  Why don't callers here just ASSERT(isMainThread)?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:61
> +    // Lock and unlock supports only one image at a time so call them i pair.

Nit: Maybe this should be:

  // You may only lock one set of pixels at a time; overlapping lock calls are not allowed.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:75
> +    Vector<OwnPtr<ImageFrameCache> > m_frameCache;

Given how lookup and delete work, is a vector really the right structure here?  Maybe a map from (id, size) -> ImageFrameCache would be better?

[Hin-Chung Lam]

Created attachment 168882 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 168773 [details]
Patch

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

Thanks for reviewing. I think your expertise with image decoders is certainly very helpful. I mostly wanted to have your input on the use of ImageDecoder and the interface with ImageSource. Stephen will review the Skia portion and. The latest patch I uploaded has addressed your comments.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:49
>> +    if (!actualDecoder)
> 
> Nit: Or just
> 
>     return actualDecoder ? new DeferredImageDecoder(actualDecoder) : 0;
> 
> (Similar comment applies numerous other places in this file)

Done.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:69
>> +    // because a multiframe is usually animated GIF. Animation is handled by
> 
> Sometimes it's an ICO, which can probably be deferred successfully.  Perhaps we should try to distinguish these, or be more aggressive here, or something?

We never animate a ICO though, DeferredImageDecoder is only accessed through BitmapImage which only animates GIF.

The problem is that frameIsCompleteAtIndex() does synchronous decoding which is only called on GIF, ICO is not a problem here.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:98
>> +        // Keep a reference to data until WebLazyDecodingImage is created, at
> 
> What class is this?  I don't see this in this patch...

Nice catch corrected the comments.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:107
>> +        // thread here.
> 
> I have absolutely no idea what this comment means.

Remove the comments, that's just a reminder for later patches.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:115
>> +    return true;
> 
> Shouldn't this be false?

Explained in the comments.  m_actualDecoder is NULL only if image decoding is deferred and that means image header decoded successfully and size is available.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:131
>> +    // uniform size.
> 
> What does "and have uniform size" mean?  A single frame's size is inherently "uniform".  But the image size and the frame size could in theory differ (I think... e.g. for an ICO with one image, whose size is marked as different from the overall image size?  Test this).

I removed the entire comments. This comments is to explain why there is a single value for frame size while parameter for the function has "size_t index". The reason is we don't defer image decoding for ICO and multiframe GIF so there is only one size and |index| is always 0.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h:38
>> +class DeferredImageDecoder {
> 
> It seems like this is implementing most of the public API on ImageDecoder.  This suggests that perhaps we ought to have some (partly?-)abstract interface that looks about like this class, which this and ImageDecoder both implement.  That makes callers able to use either via polymorphism and ensures the actual functions exposed on each stay in sync.

The function of this class is to only defer decoding to the actual decoder implementing ImageDecoder. That's why it's interface looks so much like ImageDecoder, but it is not an ImageDecoder. I intentionally choose not to extend from ImageDecoder because I want to avoid a subclass of ImageDecoder (say DeferredImageDecoder extends ImageDecoder) to reference other subclasses of ImageDecoder (e.g. JpegImageDecoder extends ImageDecoder). I chose to start a clean implementation and interface for doing this job of deferring.

Compiler makes sure the interface matches what is required by ImageSource (Only ImageSource calls this class).

Another way to look at it is:
a. ImageDecoder = Platform image decoders used by all ports except CG
b. DeferredImageDecoder = Platform image decoder used only by Chromium that is composed by (a), but does not extend from (a).

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:89
>> +        && !memcmp(bitmap.pixelRef()->getURI(), labelLazyDecoded, sizeof(labelLazyDecoded));
> 
> This seems like a hack.  Should we mark such bitmaps some other way, e.g. by keeping a set of all the live lazy bitmap*s, or by adding a field on some relevant object to directly reflect if decoding is lazy?
> 
> Or perhaps we can get by without this by reworking how resizing works?  I dunno.

Thanks for the suggestion I avoided using the URI and instead keep a set of SkBitmap generation IDs to check if a SkBitmap is in the set.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:205
>> +    return this == instanceOnMainThread() && isMainThread();
> 
> Nit: Checking that we're the instanceOnMainThread() seems unnecessary given the API in the header.  Why don't callers here just ASSERT(isMainThread)?

There will be another instance on impl thread so I have this method to verify call to this object is on the right thread.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:75
>> +    Vector<OwnPtr<ImageFrameCache> > m_frameCache;
> 
> Given how lookup and delete work, is a vector really the right structure here?  Maybe a map from (id, size) -> ImageFrameCache would be better?

I try to keep the caching scheme and lookup simple, I'm going to extend this in later patches to include region and scale so use a simple data structure at the moment.

[Peter Kasting]

(In reply to comment #73)
> >> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:69
> >> +    // because a multiframe is usually animated GIF. Animation is handled by
> > 
> > Sometimes it's an ICO, which can probably be deferred successfully.  Perhaps we should try to distinguish these, or be more aggressive here, or something?
> 
> We never animate a ICO though, DeferredImageDecoder is only accessed through BitmapImage which only animates GIF.
> 
> The problem is that frameIsCompleteAtIndex() does synchronous decoding which is only called on GIF, ICO is not a problem here.

We might be talking past each other.  It sounds like you are telling me that ICOs could in theory be deferred successfully.  I thought so too; what I was saying is that the current patch avoids deferring their decoding, which is overly conservative.  I was hoping we could distinguish between animated GIFs and ICOs.

Blue-skying a bit... it seems like with some effort we ought to be able to eventually defer animated GIFs too.  frameIsCompleteAtIndex() doesn't have to synchronously decode, maybe it could simply trigger async decoding and we could have some sort of callback that pushes the animation forward once that happens.

> >> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h:38
> >> +class DeferredImageDecoder {
> > 
> > It seems like this is implementing most of the public API on ImageDecoder.  This suggests that perhaps we ought to have some (partly?-)abstract interface that looks about like this class, which this and ImageDecoder both implement.  That makes callers able to use either via polymorphism and ensures the actual functions exposed on each stay in sync.
> 
> The function of this class is to only defer decoding to the actual decoder implementing ImageDecoder. That's why it's interface looks so much like ImageDecoder, but it is not an ImageDecoder. I intentionally choose not to extend from ImageDecoder because I want to avoid a subclass of ImageDecoder (say DeferredImageDecoder extends ImageDecoder) to reference other subclasses of ImageDecoder (e.g. JpegImageDecoder extends ImageDecoder). I chose to start a clean implementation and interface for doing this job of deferring.

Yeah, I didn't want to extend from ImageDecoder either, at least not the concrete ImageDecoder that exists.  But I wasn't sure it was totally worthless to create a simple base class that both ImageDecoder and DeferredImageDecoder could subclass.  I guess it depends on to what degree you want these objects to be substitutable at callsites.  If the only people who'd ever use DeferredImageDecoder are always going to want to explicitly refer to DeferredImageDecoder, then it doesn't matter.  But the base class would help you if you have some caller that you want to not know about these concepts and just make deferred decoding "hot-pluggable".

> >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:205
> >> +    return this == instanceOnMainThread() && isMainThread();
> > 
> > Nit: Checking that we're the instanceOnMainThread() seems unnecessary given the API in the header.  Why don't callers here just ASSERT(isMainThread)?
> 
> There will be another instance on impl thread so I have this method to verify call to this object is on the right thread.

Might want some header comments about where and how this class can be instantiated.  It at least looked like we only allowed access via a singleton on the main thread.

> >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:75
> >> +    Vector<OwnPtr<ImageFrameCache> > m_frameCache;
> > 
> > Given how lookup and delete work, is a vector really the right structure here?  Maybe a map from (id, size) -> ImageFrameCache would be better?
> 
> I try to keep the caching scheme and lookup simple, I'm going to extend this in later patches to include region and scale so use a simple data structure at the moment.

Might want some comments about your plans.

[Hin-Chung Lam]

> We might be talking past each other.  It sounds like you are telling me that ICOs could in theory be deferred successfully.  I thought so too; what I was saying is that the current patch avoids deferring their decoding, which is overly conservative.  I was hoping we could distinguish between animated GIFs and ICOs.
> 
> Blue-skying a bit... it seems like with some effort we ought to be able to eventually defer animated GIFs too.  frameIsCompleteAtIndex() doesn't have to synchronously decode, maybe it could simply trigger async decoding and we could have some sort of callback that pushes the animation forward once that happens.

Correct I think both ICOs and GIFs (animated or not) can be deferred. I think the tightest condition we can do now is when the image is non-animated, which includes non-animated ICOs and GIFs. 

Only GIFs do animation and I have a pending change to defer it too, by changing GIF image decoders. But that's a separate review.

> Yeah, I didn't want to extend from ImageDecoder either, at least not the concrete ImageDecoder that exists.  But I wasn't sure it was totally worthless to create a simple base class that both ImageDecoder and DeferredImageDecoder could subclass.  I guess it depends on to what degree you want these objects to be substitutable at callsites.  If the only people who'd ever use DeferredImageDecoder are always going to want to explicitly refer to DeferredImageDecoder, then it doesn't matter.  But the base class would help you if you have some caller that you want to not know about these concepts and just make deferred decoding "hot-pluggable".

Good point. DeferredImageDecoder is only going to be called by ImageSource so I don't think an extra interface shared between DeferredImageDecoder and ImageDecoder is necessary at this point.

> Might want some header comments about where and how this class can be instantiated.  It at least looked like we only allowed access via a singleton on the main thread.

Added comments to describe this and intention.

> Might want some comments about your plans.

What and how to cache is undecided (We need some data to back the decision). However this architecture allows a centralized cache for images in original size and scaled size. The key for cache lookup can be a mixed of {image id, scaled size, scaled subset} so I decided to use a simple data structure at the moment (i.e. vector).

[Hin-Chung Lam]

Created attachment 168994 [details]
Patch

[Stephen White]

Comment on attachment 168994 [details]
Patch

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

> Source/WebCore/ChangeLog:79
> +

I see no documentation for the ImageFrameGenerator class here.  (That's one downside to documenting the other classes, which is much appreciated BTW!)).

[Hin-Chung Lam]

Created attachment 169006 [details]
Patch

[Hin-Chung Lam]

Created attachment 169008 [details]
Patch

[Hin-Chung Lam]

> I see no documentation for the ImageFrameGenerator class here.  (That's one downside to documenting the other classes, which is much appreciated BTW!)).

I've updated ChangeLog and explains what ImageFrameGenerator and ImageFrameCache do. (gosh I hate that TestExpectations does not merge... )

[Stephen White]

Comment on attachment 169006 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:143
> +    ASSERT(m_lockedSkBitmap.isNull());

As discussed, this assert (and the singleton itself) will make multithreaded playback of SkPictures containing LazyDecodingPixelRefs impossible, since only a single bitmap can be locked into m_lockedSkBitmap.  I realize that isn't a problem today, I just wanted to note it for posterity.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:198
> +    m_frameGenerators.add(m_nextId, ImageFrameGenerator::create(decoder));

It doesn't look like ImageFrameGenerators are ever destroyed, since they're never removed from this hash map.  Is that correct?  If so, I'm a little concerned about this, since it looks like they'll grow without bound.  I'm also not fond of the fact that they're connected to the pixelrefs only by ID.

Could we instead have the LazyDecodedPixelRef have a RefPtr<ImageFrameGenerator>?  That would limit the growth of the ImageFrameGenerators, and also remove the necessity for a unique ID.  (alpha tells me that the ID is also to future-proof against impl-side decoding, but that's not yet an issue in this patch -- we will discuss this more offline).

[Hin-Chung Lam]

Created attachment 169363 [details]
Patch

[Hin-Chung Lam]

As discussed I have made ImageFrameGenerator refcounted and referenced by LazyDecodingPixelRef.

However in this patch I have to keep an ID around. Because of the following reasons:
1. ImageFrameGenerator is uniquely identified by an ID to represent an image file. This ID is used to index decoded image cache.

2. I want ImageFrameGenerator and ImageFrameCache to be loosely coupled. This means ImageFrameGenerator does not hold a reference to ImageFrameCache but just an ID. ImageDecodingStore can decide whether to evict the cache or not without telling ImageFrameGenerator.

In order to prevent the number ImageFrameCache goes unbounded, in the destructor of ImageFrameGenerator we'll remove all associated ImageFrameCache from ImageDecodingStore.

Please let me know what you think.

[Stephen White]

Comment on attachment 169363 [details]
Patch

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

I still find this design more confusing than it needs to be, although perhaps I'm just not getting it.  It seems you've just pushed the imageID problem to another class.  Instead of a unique ID, perhaps you could consider using a hash of the original SkBitmap (or perhaps the NativeImageSkia) and the requested size?  This might help you avoid the O(N) lookups in the frame cache as well, and allow you to get rid of the static s_nextImageId, which I think is going to be problematic for impl-side playback.

All that said, it is behind a flag, and I don't want to slow you down, so I'm setting this r+ for now.  You can consider my bikeshedding suggestions of names for a future patch.

> Source/WebCore/ChangeLog:76
> +        ImageFrameCache
> +
> +        A container for a scaled image fragment. In addition to bitmap pixels
> +        it contains information about the ID of the image, scale and clipping.

This is not really a cache, but more of a cache entry, no?  It could be named ImageFrameCacheEntry, but I find that a bit unwieldy.  ImageFragment, or ScaledImageFragment, maybe?

> Source/WebCore/ChangeLog:81
> +        ImageFrameGenerator
> +
> +        This object is responsible for generating decoded pixels. It is also
> +        a container for encoded image data and corresponding image decoder.

Would it be strange to call this class ImageFrameDecoder?  Generator is kind of a generic name.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:79
> +    HashMap<int, OwnPtr<ImageFrameGenerator> > m_frameGenerators;

Nit:  This appears to be unused.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:46
> +    m_size = SkISize::Make(m_decoder->size().width(), m_decoder->size().height());

Is this always the original size of the bitmap?  If so, perhaps this could be named m_fullSize instead to make it clear.  Just a suggestion, though.

[Hin-Chung Lam]

Created attachment 169459 [details]
Patch for landing

[Hin-Chung Lam]

> I still find this design more confusing than it needs to be, although perhaps I'm just not getting it.  It seems you've just pushed the imageID problem to another class.  Instead of a unique ID, perhaps you could consider using a hash of the original SkBitmap (or perhaps the NativeImageSkia) and the requested size?  This might help you avoid the O(N) lookups in the frame cache as well, and allow you to get rid of the static s_nextImageId, which I think is going to be problematic for impl-side playback.

This is a good point. The requirement is that there is a unique identifier for ImageFrameGenerator and a quick way to locate the cached fragment in ImageDecodingStore.

These two are separate problems.

I think the URI for the resource is a good way to replace s_imageId for this job and we're going to do this in a separate patch.

We should have a more efficient way to locate the cached fragment and let's discuss that afterwards.

> This is not really a cache, but more of a cache entry, no?  It could be named ImageFrameCacheEntry, but I find that a bit unwieldy.  ImageFragment, or ScaledImageFragment, maybe?
>

Yes renamed this to ScaledImageFragment.
 
> Would it be strange to call this class ImageFrameDecoder?  Generator is kind of a generic name.

I find there's too many XXXDecoder so punt the renaming for later.

> Nit:  This appears to be unused.

Removed.

> Is this always the original size of the bitmap?  If so, perhaps this could be named m_fullSize instead to make it clear.  Just a suggestion, though.

Renamed.

[WebKit Review Bot]

Comment on attachment 169459 [details]
Patch for landing

Rejecting attachment 169459 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t_tests/port/chromium.py
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 4227.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej
patching file LayoutTests/platform/chromium/virtual/deferred/fast/images/README.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14465104

[Hin-Chung Lam]

Created attachment 169461 [details]
Patch for landing

[Hin-Chung Lam]

Comment on attachment 169461 [details]
Patch for landing

Had problems landing it with merging TestExpectations.. marking this review? again.

[WebKit Review Bot]

Attachment 169461 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:4236:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hin-Chung Lam]

Created attachment 169465 [details]
Patch

[Stephen White]

Comment on attachment 169465 [details]
Patch

Looks good (bots willing).  r=me

[WebKit Review Bot]

Comment on attachment 169465 [details]
Patch

Clearing flags on attachment: 169465

Committed r131810: <http://trac.webkit.org/changeset/131810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 99762

[Dana Jansens]

Sorry for the revert. Locally ran debug webkit_unit_tests and I got this:

[----------] 2 tests from DeferredImageDecoderTest
[ RUN      ] DeferredImageDecoderTest.drawIntoSkPicture
./out/Debug/webkit_unit_tests: symbol lookup error: /usr/local/google/home/danakj/s/c/src/out/Debug/lib/libwebkit.so: undefined symbol: _ZN8SkString3setEPKc

Here's the crash log for the unit tests:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/280/steps/webkit_unit_tests/logs/stdio

It also was crashing in layout tests for the linux debug bot:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/builds/280/steps/webkit_tests/logs/stdio

[Dana Jansens]

Windows build errors here:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder%20%28dbg%29/builds/30061/steps/compile/logs/stdio#error1

[James Robinson]

SkString isn't marked with SK_API, so I suspect it isn't exported in the component build.

[Hin-Chung Lam]

Yeah it does look like a component build problem. I'll try it locally.

[Hin-Chung Lam]

James: do you how to solve the problem of int32_t with Skia? I think I'm missing an include file.

[Hin-Chung Lam]

I think I need to include SkTypes.h before SkSize.h, oh well..

[Hin-Chung Lam]

Change is Skia to add SK_API to SkString is landed. Will roll tomorrow.

[noel gordon]

(In reply to comment #84)
> 
> I still find this design more confusing than it needs to be ...

Concur.

[noel gordon]

(In reply to comment #67)
> > Given these two, it seems we can set the opaque bit as follows:
> > 
> > bitmap.setPixelRef(data);
> > if (format_is_opaque && we_have_all_the_data)
> >     bitmap.setIsOpaque(true);
> 
> Agreed. I left a FIXME in the code that describes just this.
> 
> > When we finally do get all the data, we *have* to make a new pixelRef, at which time we can mark its opaqueness.
> 
> When we have all the data and image is recorded to SkPixelRef the image is not yet fully decoded. So for PNG, GIF, WebP+alpha we need to wait until the image is decoded (rasterized) and only during next record that we can make a new PixelRef and mark it opaqueness safely. This is also noted in the FIXME.

"opaqueness" has been mentioned a few times on this thread, and I'm not sure what that means when decoding is deferred.

The paint algorithm must paint from back to front per CSS3 Appendix E, Section E.2 Painting order.  When we paint an image, the background of an image element is painted before its foreground.  The background paint phase asks renderImage if the image frameHasAlphaAtIndex().  Since frameHasAlphaAtIndex is now deferred (decoding is prevented) the answer is true, so renderImage paints the image background.  When the paint phase reaches foreground, decoding is allowed.  Provided we decode all the image, the opaqueness is now know, we can set bitmap.setIsOpaque ok, but frameHasAlphaAtIndex can change state.

Decoding deferral can result is over-painting (a needless background paint) and that can produce image rendering artifacts in some cases. A test case might help ...

[noel gordon]

Created attachment 169621 [details]
test-background.html

1. Load the test case in Chrome canary.
2. Two ~black circles should be shown (reload the page if the are not shown).
3. Resize the page to force a repaint.

Result: The black circles disappear.

Expected result: There should be no black circles.

[Hin-Chung Lam]

Created attachment 169664 [details]
Patch

[Stephen White]

Comment on attachment 169664 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Attachment 169664 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hin-Chung Lam]

Don't worry about the style check, SkTypes.h has to be included before SkSize.h

[Hin-Chung Lam]

Committed r131928: <http://trac.webkit.org/changeset/131928>

[Adam Barth]

This patch might have caused http://code.google.com/p/chromium/issues/detail?id=158182

[Hin-Chung Lam]

Looking now.