[Webkit-unassigned] [Bug 108690] Add ImageFrameCache to facilitate access to ImageFrames

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Stephen White <senorblanco at chromium.org>  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.  :)

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