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

Mon Jun 15 12:19:59 PDT 2009

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

------- Comment #43 from pkasting at google.com  2009-06-15 12:19 PDT -------
(In reply to comment #41)
> Rather, it is the case that the image-decoders as now configured
> are no longer cross-platform and regardless whether this is a layering
> violation or not, I think it prudent to consider whether this is wise in the
> long term.

Thank you for the calm tone in this reply :)

> 2) ... I've seen pkasting mention that the current Vector<unsigned> is
> suboptimal for Cairo and WX, but I haven't seen the details of what this will
> move to and whether or not there is any possibility Cairo/WX/FooPort can share
> the future backend data store.  It'd be nice to have more info please.

>From a high level, the idea for Cairo is for setSize() to do a
cairo_image_create_surface(), have the operations that write pixels operate on
that surface, and have asNewNativeImage() simply return a copy of the surface
(which is cheap as Cairo surfaces refcount like Skia bitmaps do, from my
limited reading of the Cairo APIs).

For wx the idea is similar except to use a wxBitmap instead of a
cairo_surface_t.  The benefits are more concrete for wx as it is definitely not
only copying the data when reading it out, but also flipping BGRA -> RGBA.

Another benefit here is that the data ownership becomes more clear: the
lifetime of the returned pointer is not tied to the lifetime of the
RGBA32Buffer inside the ImageDecoder.  Holger says he hasn't seen any problems
in the Cairo port due to this, and I believe him.  I'm still not certain that
it's _not_ a problem for Cairo given that we had issues with Skia with this,
and I believe the Skia backend is far more well-tested on the web at large than
the other non-CG ports.  On net I'm not sure how much weight to give this
concern but I am not willing to dismiss it.

> Con:
> 1) We lose flexibility in the image-decoders as they now depend on
> platform/graphics

If you could elaborate more on this loss I'd be interested.  I'm not completely
sure what sorts of flexibility are actually disappearing; on the surface,
#include "FrameBuffer.h" where that header lives in graphics/ does not seem
different to me than if it lives in image-decoders/.

> 2) Duplication of effort with ImageBuffer

I consider this an active area of development interest.  To the degree that the
two are duplicated, we should definitely converge.

Note that if the two weren't precisely duplicated before, but are now and can
be combined, without loss of performance, then there is a net code (and
complexity) savings.  This isn't yet quantifiable since there aren't yet
concrete designs here.

> 3) Merging with mozilla versions becomes harder

I object to this for two reasons.  First, the Mozilla decoders used are pretty
much limited to the files in GIFImageReader.*; the JPEG/PNG decoders are just
wrappers around libjpg and libpng, the XBM/ICO/BMP decoders were written by
Google, and the files in GIFImageDecoder.* were written by hyatt long ago to
shim the Mozilla decoders into WebKit.  So when we focus narrowly on what's
actually been copied from Mozilla, we find that the series of patches on bug
25709 barely affected it at all, since they were to GIFImageDecoder.* rather
than GIFImageReader.*.  Thus I don't think merging has actually become harder.

Second, to my knowledge there is no maintainer of these decoders on the WebKit
side and has been no attempt to merge new code from Mozilla.  As far as I know
I've been the only one working with the GIF decoder's internals for a few
years, and Eric Seidel noted to me privately that whether I like it or not, I
have basically taken over ownership of these decoders.  In that light, it
doesn't seem like merging was actively happening or was an interest of any of
the other coders, at least not more than other responsibilities they had.

Finally, I am in fact interested in looking at more recent work by Mozilla,
especially color profile support, as this is a notable lack in the
cross-platform decoders compared to the CG ones.  So I expect that at some
point I will be attempting a merge, and if there are difficulties at that point
I am willing to resolve them.

> 4) Introducing copy+paste code which is is frowned upon. Cairo and WX (and
> presumably all future ports that wish to use this) have the exact same
> implementation of  ImageDecoderFoo except for the 'asNewNativeImage' method.

I agree that this occurred.  As I mentioned before, the current scope of this
is 83 lines duplicated in one file, and once pro (2) above is addressed (which
I intended to help do imminently) it will not occur at all.  Therefore I feel
this is a transient issue.  However, if this is seen as a huge problem, one
solution would be to temporarily introduce an ImageDecoder.cpp file alongside
ImageDecoder.h that contains the functions that are currently common to wx and
Cairo.  While I think this is something of a waste of time it would be
preferable (to me) to backing out much of the ImageDecoder work.

> Given the above, I'd like to know if it'd be possible for Skia to introduce a
> ctor for its SkBitmap that can take an existing buffer without copying, much as
> Cairo and Qt are doing now.  What would this lose the Skia port and/or how is
> it insufficient given #2 above?

I don't know how to judge what Qt is doing now (since you mention it), because
as I have said before I don't believe Qt is using the cross-platform decoders
or very much from ImageDecoder.h.  As to behaving like Cairo currently does, it
is possible for Skia to adopt an external buffer and not manage its memory;
however, this loses the lifetime safety that I mentioned above.  I am gravely
concerned about doing something like this if the sole benefit is to share more
of the RGBA32Buffer implementation, when I am fearful of the downsides and
convinced that at the very least wx does not benefit from this implementation.

> Where before the code was not
> particularly dependent on any platform, now it is.

Before the code _was_ dependent on platforms, in the form of having an
implementation for Cairo and wx and one for Skia.  I don't think it's fair to
consider only image-decoders/ImageDecoder.h and not also
image-decoders/skia/ImageDecoder.h in describing the prior state of the world.

I think it is inherently sensible that low-level pixel storage differ platform
to platform, and it already does elsewhere in the code.  What I am trying to do
is to work towards unifying all the different ways in which low-level storage
is achieved so the entire image subsystem, not just the decoders, is as simple,
consistent, safe, and performant as possible.  The origin of RGBA32Buffer was
as a fairly quick hack to get something working, and I think the larger picture
of the rest of the image system suggests that while it works to some degree it
is not the best design.

I believe Holger actually agrees with this last point, at least to some degree,
and mainly differs from me in that he would prefer the old ImageDecoder code to
the new code until that larger transition moves further, whereas I see the new
code as making it much more clear where the needs of the image decoders lie (as
we do the broader refactoring) and being generally simply on net (when Skia is
factored in) than the old situation.  Therefore one of us feels this is a step
backward and one a step forward, but I don't believe either of us believes that
this is the ideal endpoint for the code.

My proposed plan forward is:
* Merge all the ImageSource files
* Simplify the RGBA32Buffer interface slightly by e.g. replacing "width,
height" with IntSize, etc.
* Pull RGBA32Buffer out of image-decoders entirely into a class called
FrameBuffer in graphics/
* Consider pulling implementations of getAddr() out into the platform-specific
files to reduce #ifdefs in header
* Write Cairo- and wx-specific implementations of FrameBuffer
* Examine uses of ImageBuffer and attempt to merge it with FrameBuffer
* Examine uses of the merged class e.g. in the ImageDecoder versus as returned
from the ImageSource and see if ImageDecoder can be made simpler, in terms of
simply taking an empty image as input and writing into it rather than owning
any pointers
* Examine ImageSource and the other image system classes and attempt to
collapse/simplify where possible
* Publish design document for image system indicating control flow, data
ownership and lifetime, caching behavior, etc.

These last points (except the design doc) are more speculative and vague, of
course, but hopefully that gives a broad idea of what my goals are.

I had not originally planned to do all this immediately, but I am willing to
prioritize it highly given the concerns expressed on this bug.

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