Bug 108690 - Add ImageFrameCache to facilitate access to ImageFrames
Summary: Add ImageFrameCache to facilitate access to ImageFrames
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-01 14:04 PST by Min Qin
Modified: 2013-03-01 02:53 PST (History)
12 users (show)

See Also:


Attachments
Patch (30.98 KB, patch)
2013-02-01 14:20 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (31.14 KB, patch)
2013-02-13 11:44 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (35.64 KB, patch)
2013-02-13 12:11 PST, Min Qin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Min Qin 2013-02-01 14:04:38 PST
Add ImageFrameCache to facilitate access to ImageFrames
Comment 1 Min Qin 2013-02-01 14:20:16 PST
Created attachment 186135 [details]
Patch
Comment 2 Hin-Chung Lam 2013-02-12 21:39:02 PST
Comment on attachment 186135 [details]
Patch

Thanks! I like this. LGTM.

Please fix merge conflicts so it greens EWS.
Comment 3 Hin-Chung Lam 2013-02-12 21:40:13 PST
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.
Comment 4 Min Qin 2013-02-13 11:44:00 PST
Created attachment 188133 [details]
Patch
Comment 5 Min Qin 2013-02-13 11:44:43 PST
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.
Comment 6 Early Warning System Bot 2013-02-13 11:55:37 PST
Comment on attachment 188133 [details]
Patch

Attachment 188133 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16537248
Comment 7 Early Warning System Bot 2013-02-13 11:59:31 PST
Comment on attachment 188133 [details]
Patch

Attachment 188133 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16454962
Comment 8 Min Qin 2013-02-13 12:11:33 PST
Created attachment 188142 [details]
Patch
Comment 9 Hin-Chung Lam 2013-02-13 13:34:43 PST
Comment on attachment 188142 [details]
Patch

LGTM. Thanks for working on this!
Comment 10 Min Qin 2013-02-13 15:06:09 PST
Stephen, any thoughts?
Comment 11 Stephen White 2013-02-13 17:26:22 PST
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?
Comment 12 Hin-Chung Lam 2013-02-13 17:31:43 PST
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.
Comment 13 Stephen White 2013-02-13 18:19:06 PST
(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.  :)
Comment 14 Stephen White 2013-02-13 18:21:09 PST
(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.  :)
Comment 15 Min Qin 2013-02-13 18:28:46 PST
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.  :)
Comment 16 Min Qin 2013-02-13 18:39:41 PST
Peter, would you please take a look at this?
Comment 17 Stephen White 2013-02-13 18:40:49 PST
(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.  :)
Comment 18 Min Qin 2013-02-13 18:54:14 PST
(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.
Comment 19 Stephen White 2013-02-13 19:08:54 PST
(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?
Comment 20 Hin-Chung Lam 2013-02-13 19:19:31 PST
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?
Comment 21 Hin-Chung Lam 2013-02-13 19:26:00 PST
(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?
Comment 22 Min Qin 2013-02-13 19:38:03 PST
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?
Comment 23 Hin-Chung Lam 2013-02-13 19:42:54 PST
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.
Comment 24 Stephen White 2013-02-13 19:51:08 PST
(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.
Comment 25 Hin-Chung Lam 2013-02-13 19:54:52 PST
(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..
Comment 26 Min Qin 2013-02-13 20:08:25 PST
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..
Comment 27 Peter Kasting 2013-02-13 20:40:39 PST
I think I agree with comment 24.
Comment 28 Build Bot 2013-02-14 16:47:32 PST
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
Comment 29 Eric Seidel (no email) 2013-03-01 02:53:31 PST
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).