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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 13 19:06:39 PST 2013


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





--- Comment #19 from Stephen White <senorblanco at chromium.org>  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?

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


More information about the webkit-unassigned mailing list