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

[bugzilla-daemon at webkit.org]

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





--- Comment #30 from Peter Kasting <pkasting at google.com>  2009-08-14 10:51:53 PDT ---
(In reply to comment #23)
> Created an attachment (id=34772)
 --> (https://bugs.webkit.org/attachment.cgi?id=34772) [details]
> 1) ImageFrameSink

Here's a spattering of comments all over this patch, based on a quick glance.

I assume that your motive for resurrecting the "decoded height" variable is so
that when you paint a SharedBitmap you can do an opaque blit of the valid
height (and avoid the undecoded portion entirely) instead of doing a
transparent blit of the entire frame size?  If so, I'm not necessarily opposed
to resurrecting the height member, although no other ports do this and I wonder
if it actually saves much time (it's only relevant while an image is
half-decoded and being painted, and at that point the user isn't getting
high-speed animation or similar anyway).  If it doesn't actually buy meaningful
performance, then returning true for frameHasAlphaAtIndex() (either using the
method it has now, which I think is fine, or pushing this into all the
decoders, which I think is unnecessary code duplication for no benefit) seems
like a simpler and better way.

I don't understand what createInstance() and deleteInstance() buy you.  Callers
could just use "foo = new ImageFrameSink(bool);" or "delete foo;".

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_bmp to m_bitmap,
can to canFreeBuffer.

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.

Why are the data members protected instead of private?  Per Darin Adler we
should make things as private as possible.

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

Not a big fan of things like "foo >> 1 << 1".  "foo & ~0x1" would be clearer. 
Same with "foo >> 3 << 7"; "(foo << 4) & 0xFFFFFF80" seems clearer.

Why does getFrame() return 0 when the status is frameEmpty?  An empty frame is
still a valid one.

The proper use of setCanFreeBuffer() is not obvious to me.

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

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