[Webkit-unassigned] [Bug 26379] Reconsider image decoding architecture/APIs

[bugzilla-daemon at webkit.org]

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





------- Comment #31 from pkasting at google.com  2009-06-14 22:48 PDT -------
(In reply to comment #30)
> This seems reasonable to me.  After looking things over, the stuff in
> ImageDecoder (RGBA32Buffer) that is duplicated in wx and Cairo are themselves
> duplicates of concepts in the ImageBuffer class.

That is good to hear.  I can start trying to understand ImageBuffer tomorrow,
hopefully soon I will grasp it enough to make a concrete proposal.  Sounds
promising though.

> To build on some of Holger's suggestions, could the RGBA32Buffer be handled as
> a cross-platform base class (which the ImageBuffer would inherit from), and
> push the platform-specific logic into the individual ImageBuffer classes?  The
> decoder class could then deal in ImageBuffers:
> 
> RGBA32Buffer::clear <==> ImageBuffer::clearImage
> RGBA32Buffer::width/height <==> ImageBuffer::size
> 
> ImageBuffer would inherit some pixel-level access methods, which might or might
> not be useful/good.

I am too ignorant to say at the moment.  My one concern with having too much of
an inheritance hierarchy is that I'd like to make sure there is not much
overhead involved in things that happen in the inner loop of the decoders,
which is primarily "set this pixel".  In fact I could have avoided most of the
current #ifdefs in ImageDecoder.h if I just made getAddr() not be inline, but I
was worried about not wanting to hurt performance.

I should probably run some tests on some Chromium builds (as we have good
perf-testing infrastructure) to see whether this kind of thing actually hurts. 
Pointless optimization just hurts readability.

In the meantime, don't be shy about posting patches if you want; code is always
more concrete than a description.

> > [...] but I certainly like Holger's idea of collapsing all the separate implementaions into one for
> > starters.
> 
> Agreed.

I will post an updated version of that patch tomorrow that will apply to the
tree as-is (and, I suspect, remove all the per-platform files here). 
Eventually I'd like to take a look at the function this class is performing to
ensure it makes sense.  Of course, eventually I'd like to write a detailed
design doc for the entire image subsystem :)


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