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

Fri Aug 14 12:57:09 PDT 2009

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

--- Comment #40 from Peter Kasting <pkasting at google.com>  2009-08-14 12:57:08 PDT ---
(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (In reply to comment #30)
> > 
> > But does anyone do that?  It doesn't look like it from this patch.  Why build
> > factory functions unless you need them?
> 
> See first patch. We have ImageFrameSinkWince defined which holds a SharedBitmap
> as the buffer provider.

I looked at most of the patches and missed any ImageFrameSinkWince class.  It
looks in this patch like you have an ImageFrameSinkWince.cpp, but it's just
filling out the functions of ImageFrameSink and doesn't do any subclassing.

Maybe I'm missing something.

> > 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.
> 
> I've fixed many bugs on it, and our browser runs OK. Let me review this and get
> back to you. I did that so long time ago :)

Try cases like http://www.aintitcool.com/files/HoD2.gif and
http://img2.abload.de/img/almost_failedovs.gif and make sure that:
* Images animate in the same time as Chrome 2/Safari 4 (not slower)
* You don't blow out memory or CPU usage

> > > > 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?
> 
> I'm lost now. I thought you mean why setRGB16 and setRGBA cannot share a same
> function. If that was the question, I would say separate functions are
> definitely better.

It sounds like we're not understanding each other.

My object is to reduce the complexity and special-casing of shared code,
especially the decoders themselves.  Let's say platform X supports native image
storage in various formats (565, 888, 8888, etc.).  The pattern that makes
sense to me is:

Image Decoder reads the pixel values out of the source image and calls setRGBA
with them
ImageFrameSink/RGBA32Buffer/whatever passes these values along to the backend's
underlying NativeImage functions
These functions decide how to downsample to an appropriate format

With your existing code, the downsampling would need to be done in the decoder,
which would need to check whether the image frame claims to be 16-bit.  All
that seems to do is cause us to plumb more state up through shared code and
push the complexity on everyone.

> > > > 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.
> 
> So why cannot it return 0?

Practically, it can; there are only two reasons I wouldn't do it:
* The API is lying
* A backend can't read any data off a NULL pointer, such as the size of the
frame, if it wants it

If it makes no difference, I don't see why we should lie to callers.  It's not
a big deal, it just seems like a waste to explicitly check for an empty frame
so we can lie.

> > > > 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?
> 
> FrameComplete is just for a single frame. But GIF decoder may still want to
> read a finished frame when it processes the next one. 

So it's _not_ about the decoder being finished writing to the buffer.  It's
some signal that the decoder is finished reading the buffer too.  In that case
it seems like a bug that you don't finishBitmap() when we go to FrameComplete.

In this case I suggest copying clear() from the existing code, which is
designed for precisely this functionality.  Note that there are fewer member
variables involved (no "should" and "can" distinction).

> > > 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?
> 
> ok. but for why they are valuable, I have mentioned in bug 26467, see comment
> #5

It's not at all obvious in the code that these are useful for dynamic image
scaling.  Nor do I see what they buy you for scaling versus the decoding
technique in the existing code.  I think I'd benefit from a writeup :)

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