[Webkit-unassigned] [Bug 93467] Change ImageSource to be asynchronous.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93467





--- Comment #20 from Hin-Chung Lam <hclam at google.com>  2012-08-10 12:38:15 PST ---
(From update of attachment 157706)
View in context: https://bugs.webkit.org/attachment.cgi?id=157706&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:-133
> -    

nit: Don't remove this empty line.

> Source/WebCore/platform/graphics/BitmapImage.cpp:150
> +    m_frames[index].m_isComplete = m_source.frameIsCompleteAtIndex(index, ImageSource::NonRequestDecoding);

I can understand why you add this flag here: you want to prevent this call from doing a full image decoding internally. I suggest we don't worry about this yet. It becomes confusing with details at this level because we want to just complete asynchronous ImageSource for this change.

> Source/WebCore/platform/graphics/ImageSource.cpp:178
>      ImageFrame* buffer = m_decoder->frameBufferAtIndex(index);

I understand this function causes a full decode when didDecodeFrameAtIndex() is called and you want to avoid that with RequestDecodingMode. I think a better way to do this is to delegate the call to ImageDecoder. And then only GIFImageDecoder will do a full frame decode, which is not in the scope of parallel image decoding and keep us out of trouble.

I suggest we don't change this implementation, we should narrow our focus to just making ImageSource asynchronous.

> Source/WebCore/platform/graphics/ImageSource.cpp:203
> +

Same here we should let ImageDecoder answer this call.

> Source/WebCore/platform/graphics/ImageSource.cpp:218
> +    if (mode == RequestDecoding)

Same here we don't need to do this. We should have a separate change to move decoding in this call into ImageDecoder.

> Source/WebCore/platform/graphics/ImageSource.h:151
> +    float frameDurationAtIndex(size_t, RequestDecodingMode = RequestDecoding);

I don't think we need this enum. See below in SynchronousImageSource.

> Source/WebCore/platform/graphics/SynchronousImageSource.cpp:106
> +    return m_imageSource.frameHasAlphaAtIndex(index);

We can write this function as:

m_imageSource.requestFrameAtIndex(index);
return m_imageSource.frameHasAlphaAtIndex(index);

If we're worried about future changes we can write a unit test to make sure behavior is correct. Also give a comment to state calling requestFrameAtIndex() is needed.

> Source/WebCore/platform/graphics/SynchronousImageSource.cpp:111
> +    return m_imageSource.frameIsCompleteAtIndex(index);

Same here:

m_imageSource.requestFrameAtIndex(index);
return m_imageSource.frameIsCompleteAtIndex(index);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.