[Webkit-unassigned] [Bug 90375] Parallel image decoders

[bugzilla-daemon at webkit.org]

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





--- Comment #40 from Hin-Chung Lam <hclam at google.com>  2012-07-26 19:25:06 PST ---
Created an attachment (id=154812)
 --> (https://bugs.webkit.org/attachment.cgi?id=154812&action=review)
Draft patch for refactoring in BitmapImage and Image

I have drafted a patch for the refactoring of BitmapImage and Image to show the direction we want to do, here's some highlights of this patch and particularly the difference, I would like to use this to continue our discussion of refactoring.

Here's the highlights of the design / refactoring:

1. Getters for metadata go directly into the decoder without decoding the entire frame.

This is different than the design you suggested. We really want to restrict the start of decoding to only one case, i.e. when nativeImageForCurrentFrame() is called. This is important for us because we don't want a metadata getter to start decoding and we can then have a clear indication when we can defer decoding.

This is also important to performance because decoder should do the best effort to fetch the metadata without decoding, this can be implemented for frameIsCompleteAtIndex(), frameDurationAtIndex(), frameOrientationAtIndex(). The current implementation of always decoding the entire frame is not optimal.

2. Specifically call out the desire of asynchronous image decoding in BitmapImage.

I added methods to BitmapImage to indicate the desire to use asynchronous decoding, i.e. BitmapImage::setUseAsynchronousDecoding(). I would like to have usage of BitmapImage at each site explicit call out the intention, this is also important because a usage of BitmapImage can disable asynchronous decoding and this case can be handled explicitly.

So an example of usage of this would be:

if (image->isBitmapImage())
  static_cast<BitmapImage*>(image)->setUseAsynchronousDecoding(true);

draw(image, ...);

And if the usage of the image requires synchronous decoding like that of a canvas, then:

if (image->isBitmapImage())
  static_cast<BitmapImage*>(image)->setUseAsynchronousDecoding(false);

draw(image, ...);

I find it important to identify the intention of use at each site. And having this method in BitmapImage can make things more explicit.

3. Limit the asynchronous interface only to ImageSource

I noticed in your patch that ImageSource and ImageDecoder are both asynchronous. I would like to reduce this down to only ImageSource. In fact because ImageSource.cpp is only used by non-cg port, we can just implement a parallel decoding mode there and keep ImageDecoder always synchronous.

4. Use of default state before frame is fully decoded is acceptable

We talked about the worries of returning default states before an image is fully decoded, I would like to go into details about each of these metadata getters and discuss why it is okay to return a default value:

a. frameOrientationAtIndex()

This method is current *always* returning a default value for all formats. Even if this is properly this can be decoded from the EXIF data and so calling directly into the decoder can return this correctly without decoding the full frame, given that this is implemented for WebKit's jpeg decoder.

b. frameIsCompleteAtIndex()

This method is to return if a frame is full decoded. This doesn't contradict with asynchronous decoding at all, when asynchronous decoding is in use and frame is not fully decoded the frame is always incomplete. We can also see if we have enough bytes to determine of a frame is complete.

Also note there is only two main usages of this method:

i. GIF animation

For the case of asynchronous decoding, a frame is not fully decoded is always not complete and this doesn't contradict with the animation strategy.

ii. Extract image data for GraphicsContext3D for a fully loaded image

Use of frameIsCompleteAtIndex() is really not necessary there, they are used incorrectly to see if image can be decoded.

Ultimately this method should be protected and shouldn't be exposed. I think further refactoring of GraphicsContext3DSkia and GraphicsContext3DCario will elimate the use of this method. Once this method is fully internal to BitmapImage there is not problem for asynchronous decoding.

c. frameHasAlphaAtIndex()

Note that current WebKit code already returns hasAlpha = true for an incomplete and naturally a partially decoded image always has alpha. Again this method is only meaningful only if the image is fully decoded, otherwise hasAlpha should always be true anyway.

d. frameDurationAtIndex()

This is only meaningful for GIF, and for GIF this can be determined without decoding the frame. Also note that this is only meaningful if a frame is complete (because we don't care about duration until a frame is completely loaded / decoded). Grabbing the value of this attribute is the responsibility of the GIF decoder and hence should be delegated directly to the decoder.

5. Metadata state mismatch

We discussed about having a mismatch of metadata state, for example:

image->currentFrameHasAlpha() // returns true
... do something else ..
NativeImage* nativeImage = image->nativeImageForCurrentFrame() // nativeImage doesn't have alpha

This actually won't be the case, because of the following:

a. Suppose when we call image->currentFrameHasAlpha() the image has not been decoded, this method will return true and later we call image->nativeImageForCurrentFrame(), this nativeImage will either be null or contain an empty image because decoding is just going to start, ans it *will* have alpha.

b. Suppose when we call image->currentFrameHasAlpha() the image is partially decoded, this method will return true, and again image->nativeImageForCurrentFrame() will not decode anything synchronously and this doesn't change the alpha state.

c. Suppose when we call image->currentFrameHasAlpha() the image is fully decoded is cached, then we return the correct value and image->nativeImageForCurrentFrame() will not lead to a different result.

Please let me know what you think about the approach in the patch posted. I think the next thing I'll do is to review all places where nativeImageForCurrentFrame is called and put whether asynchronous decoding should be used at each spot. My gut felling is that asynchronous decoding is the common case while synchronous decoding is rare.

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