Bug 108690 – Add ImageFrameCache to facilitate access to ImageFrames

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Min Qin]

Add ImageFrameCache to facilitate access to ImageFrames

[Min Qin]

Created attachment 186135 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 186135 [details]
Patch

Thanks! I like this. LGTM.

Please fix merge conflicts so it greens EWS.

[Hin-Chung Lam]

Comment on attachment 186135 [details]
Patch

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

> Source/WebCore/platform/image-decoders/ImageDecoder.h:253
> +        void setFrameBufferAtIndex(size_t index, ImageFrame& frame)

By the way this should be const ImageFrame& frame.

[Min Qin]

Created attachment 188133 [details]
Patch

[Min Qin]

Comment on attachment 186135 [details]
Patch

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

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:253
>> +        void setFrameBufferAtIndex(size_t index, ImageFrame& frame)
> 
> By the way this should be const ImageFrame& frame.

Done.

[Early Warning System Bot]

Comment on attachment 188133 [details]
Patch

Attachment 188133 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16537248

[Early Warning System Bot]

Comment on attachment 188133 [details]
Patch

Attachment 188133 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16454962

[Min Qin]

Created attachment 188142 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 188142 [details]
Patch

LGTM. Thanks for working on this!

[Min Qin]

Stephen, any thoughts?

[Stephen White]

Comment on attachment 188142 [details]
Patch

I'm not sure I understand why a new class is necessary here, unless you plan to reuse it elsewhere. It seems to add unnecessary indirection to the call sites, especially for the getters. Would it not be simpler to keep m_frameBufferCache in the ImageDecoder base class, and add some helper functions to resize it, and set the memory allocator?

[Hin-Chung Lam]

The idea is to isolate the platform specific bits into this ImageFrameCache class.

Right now it has allocator, it will be extended to have lock and unlock such that we can use discardable memory to back it. The concept is ImageDecoder is generic while ImageFrameCache and ImageFrame contain platform specific details.

[Stephen White]

(In reply to comment #12)
> The idea is to isolate the platform specific bits into this ImageFrameCache class.
> 
> Right now it has allocator, it will be extended to have lock and unlock such that we can use discardable memory to back it. The concept is ImageDecoder is generic while ImageFrameCache and ImageFrame contain platform specific details.

I don't really see much advantage to USE(SKIA) in a new nested class vs USE(SKIA) in the ImageDecoder base class.  Unless you plan to put the platform-specific bits into an ImageDecoderSkia.cpp a la GraphicsContext[Skia].cpp?

Perhaps you could describe this design a bit further, since it doesn't appear in this patch.  I find it difficult to review code I don't see.  :)

[Stephen White]

(In reply to comment #13)
> (In reply to comment #12)
> > The idea is to isolate the platform specific bits into this ImageFrameCache class.
> > 
> > Right now it has allocator, it will be extended to have lock and unlock such that we can use discardable memory to back it. The concept is ImageDecoder is generic while ImageFrameCache and ImageFrame contain platform specific details.
> 
> I don't really see much advantage to USE(SKIA) in a new nested class vs USE(SKIA) in the ImageDecoder base class.  Unless you plan to put the platform-specific bits into an ImageDecoderSkia.cpp a la GraphicsContext[Skia].cpp?

I see now there is already an ImageDecoderSkia.cpp.  This is probably proof that I should not be reviewing this code.  :)

> Perhaps you could describe this design a bit further, since it doesn't appear in this patch.  I find it difficult to review code I don't see.  :)

[Min Qin]

To support animated gif and the capability to discard partially decoded images for deferred image decoding, we want calls to lock all the frames before we start decoding, and unlock them after decoder completes.
So we want a chromium specific lock()/unlock() calls for ImageFrame vector. Having a separate class makes us easier to do this platform specific change. Maybe I should add empty lock() and unlock() calls to USE(SKIA) to make this change more clear?

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > The idea is to isolate the platform specific bits into this ImageFrameCache class.
> > > 
> > > Right now it has allocator, it will be extended to have lock and unlock such that we can use discardable memory to back it. The concept is ImageDecoder is generic while ImageFrameCache and ImageFrame contain platform specific details.
> > 
> > I don't really see much advantage to USE(SKIA) in a new nested class vs USE(SKIA) in the ImageDecoder base class.  Unless you plan to put the platform-specific bits into an ImageDecoderSkia.cpp a la GraphicsContext[Skia].cpp?
> 
> I see now there is already an ImageDecoderSkia.cpp.  This is probably proof that I should not be reviewing this code.  :)
> 
> > Perhaps you could describe this design a bit further, since it doesn't appear in this patch.  I find it difficult to review code I don't see.  :)

[Min Qin]

Peter, would you please take a look at this?

[Stephen White]

(In reply to comment #15)
> To support animated gif and the capability to discard partially decoded images for deferred image decoding, we want calls to lock all the frames before we start decoding, and unlock them after decoder completes.
> So we want a chromium specific lock()/unlock() calls for ImageFrame vector. Having a separate class makes us easier to do this platform specific change. Maybe I should add empty lock() and unlock() calls to USE(SKIA) to make this change more clear?

No, just describing it is enough, thanks.  I'm still not seeing the advantage of the new class, though, since whether the USE(SKIA) lives on the ImageDecoder, or on the ImageFrameCache, it still lives in ImageDecoder.h.  Unless, as I said, you plan to move it out of this file, in which case I suggest you do it now so the advantage is clear.

My objection is the extra level of indirection introduced to common operations.

E.g., 

 if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete)
         return m_frameBufferCache[index].hasAlpha();

becomes

    if (m_imageFrameCache.frameBufferAtIndex(index).status() == ImageFrame::FrameComplete)
         return m_imageFrameCache.frameBufferAtIndex(index).hasAlpha();
    return true;

What's the benefit?

> 
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > The idea is to isolate the platform specific bits into this ImageFrameCache class.
> > > > 
> > > > Right now it has allocator, it will be extended to have lock and unlock such that we can use discardable memory to back it. The concept is ImageDecoder is generic while ImageFrameCache and ImageFrame contain platform specific details.
> > > 
> > > I don't really see much advantage to USE(SKIA) in a new nested class vs USE(SKIA) in the ImageDecoder base class.  Unless you plan to put the platform-specific bits into an ImageDecoderSkia.cpp a la GraphicsContext[Skia].cpp?
> > 
> > I see now there is already an ImageDecoderSkia.cpp.  This is probably proof that I should not be reviewing this code.  :)
> > 
> > > Perhaps you could describe this design a bit further, since it doesn't appear in this patch.  I find it difficult to review code I don't see.  :)

[Min Qin]

(In reply to comment #17)
> (In reply to comment #15)
> > To support animated gif and the capability to discard partially decoded images for deferred image decoding, we want calls to lock all the frames before we start decoding, and unlock them after decoder completes.
> > So we want a chromium specific lock()/unlock() calls for ImageFrame vector. Having a separate class makes us easier to do this platform specific change. Maybe I should add empty lock() and unlock() calls to USE(SKIA) to make this change more clear?
> 
> No, just describing it is enough, thanks.  I'm still not seeing the advantage of the new class, though, since whether the USE(SKIA) lives on the ImageDecoder, or on the ImageFrameCache, it still lives in ImageDecoder.h.  Unless, as I said, you plan to move it out of this file, in which case I suggest you do it now so the advantage is clear.
> 
> My objection is the extra level of indirection introduced to common operations.
> 
> E.g., 
> 
>  if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete)
>          return m_frameBufferCache[index].hasAlpha();
> 
> becomes
> 
>     if (m_imageFrameCache.frameBufferAtIndex(index).status() == ImageFrame::FrameComplete)
>          return m_imageFrameCache.frameBufferAtIndex(index).hasAlpha();
>     return true;
> 
> What's the benefit?
> 

Hmm...the ImageDecoder.h is currently flooded with USE(SKIA). With function definitions in ImageDecoder.h, and actual implementations in ImageDecoder.cpp.

[Stephen White]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > To support animated gif and the capability to discard partially decoded images for deferred image decoding, we want calls to lock all the frames before we start decoding, and unlock them after decoder completes.
> > > So we want a chromium specific lock()/unlock() calls for ImageFrame vector. Having a separate class makes us easier to do this platform specific change. Maybe I should add empty lock() and unlock() calls to USE(SKIA) to make this change more clear?
> > 
> > No, just describing it is enough, thanks.  I'm still not seeing the advantage of the new class, though, since whether the USE(SKIA) lives on the ImageDecoder, or on the ImageFrameCache, it still lives in ImageDecoder.h.  Unless, as I said, you plan to move it out of this file, in which case I suggest you do it now so the advantage is clear.
> > 
> > My objection is the extra level of indirection introduced to common operations.
> > 
> > E.g., 
> > 
> >  if (m_frameBufferCache[index].status() == ImageFrame::FrameComplete)
> >          return m_frameBufferCache[index].hasAlpha();
> > 
> > becomes
> > 
> >     if (m_imageFrameCache.frameBufferAtIndex(index).status() == ImageFrame::FrameComplete)
> >          return m_imageFrameCache.frameBufferAtIndex(index).hasAlpha();
> >     return true;
> > 
> > What's the benefit?
> > 
> 
> Hmm...the ImageDecoder.h is currently flooded with USE(SKIA). With function definitions in ImageDecoder.h, and actual implementations in ImageDecoder.cpp.

Oh duh, I see now that the new class definition is actually in ImageDecoderSkia.cpp already.

Still, my point stands:  all the helper functions could live on ImageDecoder, without the need for a new class, and without introducing another level of indirection to the common operations.  e.g., ImageDecoder::setMemoryAllocator() could iterate over its contained ImageFrames (and I think it could be moved to ImageDecoderSkia.cpp, and made non-virtual, but that's not your fault).

Similarly, your new lock() and unlock() could live in ImageDecoder, protected by the USE(SKIA), and implemented in ImageDecoderSkia.cpp.  No?

[Hin-Chung Lam]

The goal really is to move ImageFrameCache out of ImageDecoder.h. Yes ImageDecoder.h is flooded with #ifdefs but it shouldn't be. You can remove all the platform specifics into ImageFrameCache.h and ImageFrame.h. Will that show an advantage to you?

[Hin-Chung Lam]

(In reply to comment #20)
> The goal really is to move ImageFrameCache out of ImageDecoder.h. Yes ImageDecoder.h is flooded with #ifdefs but it shouldn't be. You can remove all the platform specifics into ImageFrameCache.h and ImageFrame.h. Will that show an advantage to you?

The reason I suggested Min doing this was because:

1. I think ImageDecoder.h contains too much platform details. In particular ImageFrame that has SkBitmap everywhere. It doesn't need to know these at all.

2. We plan to add more lock/unlock semantics to ImageFrame and ImageFrameCache. I felt uncomfortable adding even more Chromium specific features to ImageDecoder.h.

My conclusion was to isolate this logic layer of ImageFrameCache and ImageFrame from ImageDecoder and we will have more flexibility to the behavior of these two objects, without affecting ImageDecoder.

If we move both ImageFrame and ImageFrameCache into separate .h and .cpp, would it make things better?

[Min Qin]

Maybe have the following inheritance?
ImageFrame->ImageFrameSkia
ImageFrameCache->ImageFrameCacheSkia
And ImageDecoder has a ImageFrameCache member variable?

So that ImageFrame, ImageFrameCache, and ImageDecoder will have the minimum amount of USE(SKIA) in it?

(In reply to comment #21)
> (In reply to comment #20)
> > The goal really is to move ImageFrameCache out of ImageDecoder.h. Yes ImageDecoder.h is flooded with #ifdefs but it shouldn't be. You can remove all the platform specifics into ImageFrameCache.h and ImageFrame.h. Will that show an advantage to you?
> 
> The reason I suggested Min doing this was because:
> 
> 1. I think ImageDecoder.h contains too much platform details. In particular ImageFrame that has SkBitmap everywhere. It doesn't need to know these at all.
> 
> 2. We plan to add more lock/unlock semantics to ImageFrame and ImageFrameCache. I felt uncomfortable adding even more Chromium specific features to ImageDecoder.h.
> 
> My conclusion was to isolate this logic layer of ImageFrameCache and ImageFrame from ImageDecoder and we will have more flexibility to the behavior of these two objects, without affecting ImageDecoder.
> 
> If we move both ImageFrame and ImageFrameCache into separate .h and .cpp, would it make things better?

[Hin-Chung Lam]

I think our options are:

1. Add more #if USE(SKIA) in ImageDecoder.h and implement in ImageDecoderSkia.cpp Which I do no prefer.

2. Isolate ImageFrame and ImageFrameCache from ImageDecoder.h. Have #ifdefs in ImageFrame.h ImageFrameCache.h. Chromium specific features will be implemented in ImageFrame and ImageFrameCache and not ImageDecoder.

I don't think inheriting ImageFrame is the right approach here. It is supposed to be the platform layer.

[Stephen White]

(In reply to comment #23)
> I think our options are:
> 
> 1. Add more #if USE(SKIA) in ImageDecoder.h and implement in ImageDecoderSkia.cpp Which I do no prefer.

I don't think it would be that bad.  You'd be adding two more function declarations to an existing #if USE(SKIA) block, and two more implementations to ImageDecoderSkia.cpp.

> 2. Isolate ImageFrame and ImageFrameCache from ImageDecoder.h. Have #ifdefs in ImageFrame.h ImageFrameCache.h. Chromium specific features will be implemented in ImageFrame and ImageFrameCache and not ImageDecoder.

I don't really see the difference.  You still need the #ifdefs, you've just put them in a different (but still platform-common) file.

> 
> I don't think inheriting ImageFrame is the right approach here. It is supposed to be the platform layer.

Inheritance is not the pattern used by the WebKit platform layer, in general.

[Hin-Chung Lam]

(In reply to comment #24)
> (In reply to comment #23)
> > I think our options are:
> > 
> > 1. Add more #if USE(SKIA) in ImageDecoder.h and implement in ImageDecoderSkia.cpp Which I do no prefer.
> 
> I don't think it would be that bad.  You'd be adding two more function declarations to an existing #if USE(SKIA) block, and two more implementations to ImageDecoderSkia.cpp.

Okay I have no objection to this if you like this better.

Which means:

1. ImageDecoder should have a resizeFrameCache method. That we can implement a platform specific version to assign allocator in ImageDecoderSkia.cpp.

2. Have lock / unlock in ImageDecoder under #if USE(SKIA).

Min do you think this is reasonable? Sorry for making you do all this work..

[Min Qin]

I am fine with that. We already got several blocks of USE(SKIA) in imageDecoder.h, so probably just put lock()/unlock() in the same block as others. 
resizeFrameCache() can be used by all platforms. We just need to put the default implementation in !USE(SKIA) block in ImageDecoder.cpp and our own in ImageDecoderSkia.cpp.

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > I think our options are:
> > > 
> > > 1. Add more #if USE(SKIA) in ImageDecoder.h and implement in ImageDecoderSkia.cpp Which I do no prefer.
> > 
> > I don't think it would be that bad.  You'd be adding two more function declarations to an existing #if USE(SKIA) block, and two more implementations to ImageDecoderSkia.cpp.
> 
> Okay I have no objection to this if you like this better.
> 
> Which means:
> 
> 1. ImageDecoder should have a resizeFrameCache method. That we can implement a platform specific version to assign allocator in ImageDecoderSkia.cpp.
> 
> 2. Have lock / unlock in ImageDecoder under #if USE(SKIA).
> 
> Min do you think this is reasonable? Sorry for making you do all this work..

[Peter Kasting]

I think I agree with comment 24.

[Build Bot]

Comment on attachment 188142 [details]
Patch

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

New failing tests:
platform/mac/editing/input/text-input-controller.html
platform/mac/fast/forms/attributed-strings.html

[Eric Seidel (no email)]

Comment on attachment 188142 [details]
Patch

Cleared review? from attachment 188142 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).