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

[bugzilla-daemon at webkit.org]

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





--- Comment #37 from Yong Li <yong.li at torchmobile.com>  2009-08-14 11:43:27 PDT ---
(In reply to comment #30)
> (In reply to comment #23)
> > Created an attachment (id=34772)
 --> (https://bugs.webkit.org/attachment.cgi?id=34772) [details] [details]
> > 1) ImageFrameSink
> 
> 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 ;
}

> 
> Some of the variable names seem problematic to me; e.g. I would change
> m_expectedHeight to m_height, m_height to m_decodedHeight

m_height as RGBA32Buffer defined. Yeah, now it's ok to change the name as
m_height has been removed from RGBA32Buffer.

> 
> 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.

> 
> 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

> 
> 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?
>  If the answer is "we're worried about putting an if() in there for performance
> reasons", I'd like to see data on the effect of such a thing before prematurely
> optimizing.  Note that there's already an if() in setRGBA().

they also have different inputs

> 
> Not a big fan of things like "foo >> 1 << 1".  "foo & ~0x1" would be clearer. 

>> 1 << 1 seems cleaner to. but I'm not sure which one is faster.

> Same with "foo >> 3 << 7"; "(foo << 4) & 0xFFFFFF80" seems clearer.

Same thing here, I don't think the latter is clean.

> 
> 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?

> 
> 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.

> 
> Overall there seems to be a lot of functions to explicitly allocate and free
> things inside the ImageFrameSink.  This doesn't feel like a particularly
> friendly API.  Note that the existing RGBA32Buffer has only a few functions,
> mostly to support deep versus shallow copies of the underlying data.
> 
> This file is also entirely missing a large number of functions from the current
> RGBA32Buffer, such as clear(), copyRowNTimes(), asNewNativeImage(), etc., which
> are used in various different cases.  I assume this is because you wrote this
> code quite a while ago.  If we wanted to use ImageFrameSink, I think a lot of
> these would need to be supported, so you wouldn't be massively rewriting the
> decoders or other cross-platform Image code (especially if such rewrites
> removed optimizations, like the GIF frame-clearing optimization).

I still think it's not difficult to replace RGBA32Buffer with ImageFrameSink.
Yeah, it needs some work definitely. 

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.

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