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

Mon Jun 15 13:09:35 PDT 2009

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

------- Comment #47 from treat at kde.org  2009-06-15 13:09 PDT -------
(In reply to comment #43)
> 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).

Notwithstanding what you write below about the data ownership issues, I don't
see how this will help the Cairo port at all versus the current situation where
it adopts the Vector<unsigned> of the RGBA32Buffer.  Am I missing anything?

Also, because you asked about Qt...  while it is true that the Qt port doesn't
currently use the image-decoders, that has not always been the case.  Holger
wrote a patch in the past that converted the Qt port to use the built-in
WebCore image-decoders.  That never went into mainline, but at Torch Mobile we
used a similar approach for our version of the Qt port.  It operated much like
the Cairo port does today: we use a QImage/QPixmap ctor that adopts the
underlying data of the RGBA32Buffer.  So I'm a bit familiar with this code.  As
an aside, we have other Torch Mobile devs who've made extensive changes and as
a consequence have some pretty deep knowledge of the image-decoders for use by
our windows mobile port.

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

Hmm, that is interesting.  Of course, another alternative would be to modify
wxBitmap so it could adopt an RGBA8888 buffer, but on to your other points...

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

Absolutely shouldn't be dismissed.  OTOH, I don't see this as a fundamental win
of the new design given that we have no concrete example of data lifetime
issues in the real world.  And certainly were we given such an example I see no
reason to believe that it wouldn't be fairly easily fixed given the previous
design.

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

For one we lose the ability to modify the image-decoders in anyway that might
depend upon the underlying data buffer.  Just as a brainstorm, if in the future
we wanted to modify the decoders so that they'd sample only 16 bit data and
sync it into a RGBA4444 buffer this becomes more difficult with the addition of
the native platform stores.  Or say we come up with a new way of optimizing the
jpeg decoder that is relies upon querying the underlying data array.  I'm just
brainstorming, but I think it is not entirely clear that we want to sacrifice
this flexibility if it can be reasonably avoided.

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

If we continue with this strategy for sure.  And looking at your outline below,
I think that would be a good map if we do continue with this change.

> > 3) Merging with mozilla versions becomes harder
>
> I object to this for two reasons.  First, the Mozilla decoders used are pretty
>

Point(s) taken.

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

Only because you'd be exacerbating the platform dependencies of the
image-decoders by moving Cairo and WX away from the Vector<unsigned> :)  Right
now, if Cairo and Wx were to remain as they are, then we really only need _two_
ImageDecoderFoo.cpp files: ImageDecoderSkia.cpp and
ImageDecoderCrossPlatform.cpp :)  asNativeImagePtr could be moved to
WebCore/platform/graphics :)

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

Mentioned above...

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

>From what I can tell the only benefit of these changes is this theoretical data
lifetime issue which has not been found in the wild yet.  Theoretically, the Wx
port could, along with Skia port, be changed to work in the same way that Cairo
and Qt now work: to adopt the underlying buffer of RGBA32Buffer.  To me this
would preserve the flexibility iherant in the old way of doing things.  All the
pro's without any of the con's (except the work involved of course :)

Now how much work do you think it'd be to modify Wx and Skia to work this way
and do you think it worthwhile given my answer to the flexibilty question
above?

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

If we're to continue with the platform dependent image-decoders then the above
sounds like a good plan.  I'm still not sure the platform dependent
image-decoders are necessary or wise given the alternative.  Thoughts?

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