[Webkit-unassigned] [Bug 27561] ImageDecoder enhancements for WINCE port

[bugzilla-daemon at webkit.org]

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





--- Comment #38 from Peter Kasting <pkasting at google.com>  2009-08-14 11:59:16 PDT ---
(In reply to comment #37)
> (In reply to comment #30)
> > I don't understand what createInstance() and deleteInstance() buy you.  Callers
> > could just use "foo = new ImageFrameSink(bool);" or "delete foo;".
> >
> createInstance is useful in this case:
> 
> class ImageFrameSinkXxxxWithMoreMembers : public ImageFrameSink
> {
>  more members here;
> };
> 
> ImageFrameSink* ImageFrameSink::createInstance()
> {
>   return new ImageFrameSinkXxxxWithMoreMembers ;
> }

But does anyone do that?  It doesn't look like it from this patch.  Why build
factory functions unless you need them?

> > I'm skeptical of the utility of m_compositedWithPreviousFrame.  The only use
> > for this would be in decoding GIFs, to avoid creating the complete frame at a
> > particular index.  But this means you can't use the "large GIF, discard
> > unnecessary frames" memory optimization which makes an enormous difference for
> > large animated GIFs, because you need to keep around an arbitrary number of
> > frames.
> 
> m_compositedWithPreviousFrame is not only for "avoiding someting", but also
> important for transparent effects in GIF animation.

I don't understand.  I know how GIF animation and frame disposal methods work. 
The existing ports handle them correctly without using this mechanism.  Why
doesn't the existing mechanism work for you, and how do you deal with not
having the frame-clearing optimization for large GIFs?  I would think on a
memory-constrained platform that'd be even more important than it is on
desktops.

> > Why are the data members protected instead of private?  Per Darin Adler we
> > should make things as private as possible.
> 
> because the subclasses need to access them

What subclasses?

> > Having both setRGB16() and setRGBA() seems like a poor API for callers.  Why
> > doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit?
> 
> they also have different inputs

This response is vague enough that I haven't learned anything.  What is the
difference and why can't it be created at the SharedBuffer or ImageFrameSink
level?

> > Why does getFrame() return 0 when the status is frameEmpty?  An empty frame is
> > still a valid one.
> 
> why? Who likes to waste CPU time on empty frame?

What CPU is being wasted?  We don't blit anything with empty frames.

> > The proper use of setCanFreeBuffer() is not obvious to me.
> 
> This nofities the buffer that decoder has finished writting to the buffer, so
> the buffer can be released when it's under memory pressure.

Isn't this the same signal as setting the status to FrameComplete?

> The one that I'm most concerned is about GIF m_compositedWithPreviousFrame
> thing. That's complicated, and ImageFrameSink code works in a little different
> way. Want to talk to somebody on this.

Maybe you could write up a short doc with descriptions of the existing code and
your code, pointing out the differences and why they are valuable?

I can guess how someone would implement GIF animation with a flag like this but
I can't see how to do it without needing to keep a potentially arbitrary number
of frames around, for example if you scroll an animated GIF out of the viewport
and then back in (causing the animation to need to spin forward).

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