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

Thu Jul 26 23:11:10 PDT 2012

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

--- Comment #42 from Huang Dongsung <luxtella at company100.net>  2012-07-26 23:11:10 PST ---

Thanks for your efforts. I agreed on most of your approach.
I have some questions and concerns.

(In reply to comment #40)
> Created an attachment (id=154812)
 --> (https://bugs.webkit.org/attachment.cgi?id=154812&action=review) [details]
> 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.
> 

Absolutely, it will be easy to use BitmapImage and implement async decoders if only nativeImageForCurrentFrame() can cause full image decoding.

> 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.
> 

If we have setUseAsynchronousDecoding, we must use it with nativeImageForCurrentFrame() too.
For examples,

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

 image->nativeImageForCurrentFrame();

There are so many sites which call nativeImageForCurrentFrame() and there are only several (maybe 4) sites which need to call static_cast<BitmapImage*>(image)->setUseAsynchronousDecoding(true).
BitmapImage is used for various purposes such as <image>, backingStore, theme images and etc. I think it is hard for all developers to remember calling setUseAsynchronousDecoding(false) before draw(image, ...) and nativeImageForCurrentFrame().
So, I think the default policy should be to decode synchronously. When we want async decoding, we need to give the hint to BitmapImage.
For examples,

 draw(RetainedModeBitmapImage(image), ...);

I think you want to ask why I put RetainedModeBitmapImage in here again.

In the following situation, BitmapImage can not determine whether to decode synchronously or asynchronously.

 #1 RenderImage calls

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

 draw(image, ...);

 #2 CanvasRenderingContext2D calls with the same image.

 draw(image, ...);  <- We should do following jobs: 1. cancel async image decoding, 2. decode the image synchronously, 3. notify CachedImageClient::imageChanged(), 4. draw the image in succession.
                              BUT we can not know this calling is from CanvasRenderingContext2D.

I suggested RetainedModeBitmapImage instead of setter because of it.

> 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.
> 

I think your concern is in that ImageSource delegates ImageDecoder to ParallelImageDecoder. Maybe because ImageSource must directly call ImageDecoder::frameHasAlphaAtIndex and other metadata query functions.
If ParallelImageDecoder creates its own ImageDecoder, it will be ok.

> 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.

It is true when we need to decode an image asynchronously. If we need to decode synchronously and all data were received, image->nativeImageForCurrentFrame() must return a fully decoded nativeImage and the nativeImage can have alpha.
In the synchronous decoding case like CanvasRenderingContext2D, we can not return either null or an empty image because we do not have a chance to draw again after decoding is complete.

Other metadata friends have similar problems. For examples,

 image->frameIsCompleteAtIndex() // returns false
 ... do something else ..
 NativeImage* nativeImage = image->nativeImageForCurrentFrame() // nativeImage can be complete when decoding is synchronous.

It is the most tricky problem, and I have not found a promising solution yet. I want to hear your opinion.

> 
> 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.
> 

Ditto.

> 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.
> 

Yes, exactly.

> 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.

As I mentioned earlier, if our default policy is serial image decoding, it may not be necessary to review all the places where nativeImageForCurrentFrame is called.
I think most cases that WebKit users encounter are covered by asynchronous decoding, but in the WebKit code there are only 4 sites that we can change to decode asynchronously: RenderImage, RenderSVGImage, RenderObject (css border, mask and background image) and RenderLayerBacking (for <image>).
It is because the 4 cases have the ability to receive a decoding complete notification via CachedImageClient::imageChanged() and draw the image again.

I am very glad that there are people who dig this subject like you. I think we almost grasp how to do it.

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