Reconsider image decoding architecture/APIs

• Component: Images
• Status: RESOLVED DUPLICATE

[Peter Kasting]

Several good questions came up while I worked on bug 25709, including:
* Why isn't RGBA32Buffer called FrameBuffer?
* Why doesn't it use WebKit types like IntSize instead of "int width, int height"?
* How is it different than ImageBuffer?  Can the two be combined?
* Or maybe should it become more private to ImageDecoder and have all the decoders go through the ImageDecoder object?
* Can all the #if PLATFORM(...) stuff in it move to implementation-specific files, maybe by using subclasses or PIMPL or something?
* Can all the people using the cross-platform decoders use a common ImageSource?
* Should ImageSource and ImageDecoder merge?

I'm sure I'm missing some, too.

This bug is a good place to discuss or post idea patches.  I don't feel like I fully understand all the image decoder classes so for the time being I'll probably stick with cleanup like the first bullet or two at the top.

CCing some people who might have input.

[Holger Freyther]

Some more:

* Where is the difference between ImageBuffer and RGBA32Buffer now?
* Why should RGBA32Buffer ever know how to construct a native image? I opt for a method/function that takes a RGBA32 and gives you a NativeImagePtr. Skia code can still use secrets of the RGBA32Buffer to convert that...

[Peter Kasting]

(In reply to comment #1)
> * Where is the difference between ImageBuffer and RGBA32Buffer now?

Yes, this was asked in comment 0.

> * Why should RGBA32Buffer ever know how to construct a native image? I opt for
> a method/function that takes a RGBA32 and gives you a NativeImagePtr. Skia code
> can still use secrets of the RGBA32Buffer to convert that...

If you can whip up a patch I'd be happy to look at it, even though I can't r+.

[Holger Freyther]

Okay, let me start with patches to make the code cross-platform again. It was cross-platform in a sense that the decoded image buffer could be simply consumed (e.g. uploaded to the graphics server). There is no reason to not support this default case, it is useful to Qt (external patches), Torchmobile, the EFL port...

[Holger Freyther]

Created attachment 31258 [details]
Restore cross platform behavior

Restore the old behavior of RGBA32Buffer this includes only special casing Skia to have something else as buffer. This is restoring the capability to just consume the decoded image without undoing the Skia part.

[Holger Freyther]

Created attachment 31259 [details]
Make ImageDecoder.cpp cross-platform

There is no need to force every port to have the exact same implementation of RGBA32Buffer, the only symbol that is platform depended is the conversion to a NativeImage and that does not belong into the buffer class anyway.

[Holger Freyther]

Created attachment 31260 [details]
Move the secret how to create a cairo_surface_t back to ImageSourceCairo.cpp

This makes ImageDecoder.cpp truly cross-platform again (without any #ifdef).

[Holger Freyther]

Created attachment 31261 [details]
Unfork the WX, no need to have an exact copy of the RGBA32Buffer... use the common code

Move the WX code back to ImageSourceWX.cpp and attempt to update the buildsystem. The whitespace is left there on purpose as this is mostly moving stuff around and not intended as a cleanup.

[Holger Freyther]

Created attachment 31262 [details]
Call the thing by its name and rename ImageDecoder.cpp to RGBA32Buffer.cpp

The only symbols in this file are RGBA32Buffer symbols, rename the file to match the reality.

[Holger Freyther]

Created attachment 31263 [details]
Unfork the various ImageSource*.cpp copies

Unfork the three copies of ImageSource.cpp and have a common copy, the only symbol that is platform specific is to convert the RGBA32Buffer into a native image pointer....

[Holger Freyther]

Created attachment 31264 [details]
Take out asNativeImagePtr

Proposal to take out the asNativeImagePtr from RGBA32Buffer and move it to skia specific files.

The name "RGBA32Buffer" indicates that the secret of the class is to hold a buffer for RGBA8888 pixel manipulation, I don't think the class should know how to construct a image, someone else should know how to turn the buffer into a NativeImage...

The only #ifdef's left in the code are for Skia header files and members and there is nothing at this point we can do to avoid that. So I will stop here.

[Peter Kasting]

(In reply to comment #4)
> Restore the old behavior of RGBA32Buffer this includes only special casing Skia
> to have something else as buffer. This is restoring the capability to just
> consume the decoded image without undoing the Skia part.

I don't think this patch should be landed.  The Cairo and wx ports are about to move away from using Vector<unsigned> to store their data, which this patch prevents.  And no one else is actually using this.  Anyone who did would be better served by using their platform-specific storage type in the backend.

I don't think you grasp what a perf difference it makes not to use Vector<unsigned>.  This is necessary for anyone who wants to ship a performant renderer.

[Peter Kasting]

(In reply to comment #5)
> There is no need to force every port to have the exact same implementation of
> RGBA32Buffer, the only symbol that is platform depended is the conversion to a
> NativeImage and that does not belong into the buffer class anyway.

Again, this should not be landed.  This hinders the backends using appropriate per-platform storage classes.

[Peter Kasting]

(In reply to comment #9)
> Created an attachment (id=31263) [review]
> Unfork the various ImageSource*.cpp copies

This is the only patch in this series I support.  I think this is fine, and in fact if we leave asNewNativeImage() (which I think is important to do) we can collapse the files entirely and not partially.

[Peter Kasting]

I haven't given explicit "no" replies to every single patch, but that's because I think all but the ImageSource collapsing are really a single patch that simply undoes all of the ImageDecoder unforking.  You have never stated how you intend these ports to obtain acceptable performance or what concrete benefits this has.  I have worked with the image decoders for several years and know enough to say that this is a damaging set of patches that we should not land.  I think your only motivation is a mistaken sense of some sort of "cross-platform" idealism that fails to account for the real constraints here.

The original decoders weren't written the way they were because it was better.  They were written for one port as a quick experiment to get something onscreen.  Since then a few other ports have done the minimal amount of work to get images onscreen, but Chromium is the only one that has actively worked to make this code production-ready.  Don't undo this work when you don't have the background on why it was done.

[Brent Fulgham]

I feel like this bug is being hijacked to flip things back to the "bad" old way, rather than being a useful conversation about how to make the whole image decoder better.

When Apple abandoned these decoders in favor of the CoreGraphics routines, they effectively headed down a road that provided superior performance through internal use of CGImageRef types (which abstract the various image types).  Peter's patches brought the image-decoder classes more in line with this philosophy.

It seems to me that the Qt port's objection is addressed entirely in the current sources, with the one small nit that there are now some #ifdefs in the header.


Perhaps we can figure out how to remove the #ifdef code from the header through PIMPL or some other technique.

[Brent Fulgham]

Another point in all this work is that the benefit of unforking Chromium's code is a huge benefit to the WebKit project, especially ports like GTK+/Windows Cairo that do not have access to the same development resources as Qt or Apple.  Prior to Peter's modifications, all the Cairo ports were unable to view XBM/ICO/BMP files, and with virtually no effort on our parts we now have these features.  I look forward to receiving other great new functionality and bug fixes as Google moves forward updating these now-shared image decoders as their team finds bugs, fixes performance issues, and so forth.

I am frankly dismayed by the poor attitude displayed in IRC and these bugs towards this work.  Google did not receive any benefit from doing this work, but the Linux and redistributable Windows ports gained significant useful functionality.  And the fact that as far as I can tell the majority of the objection boils down to a tirade over the presence of #ifdef guards in the header (perhaps temporarily) seems like a major overreaction.

It's nice that various embedded projects use the Qt environment and perhaps use the ImageDecoder stuff (which as far as I can tell is unimpeded by these changes).  But there are also cell phone companies, Windows API-compatible embedded projects, the Titanium project, and people that wish to have a redistributable WebKit on Windows that use the Windows Cairo port, as well as the myriad of Linux-based teams that use the GTK+ port, that will greatly benefit from these changes.

Backing Peter's changes out now will adversely affect all of these projects.

Now, can we please discuss how to make the image decoders *better*, rather than trying to undo history?

[Peter Kasting]

(Adding Kevin Ollivier since he might have perspective on the wx side of things.)

Thanks for the comments, Brent.  For background, a chief reason to go the direction I have so far with RGBA32Buffer is to allow platforms to write directly into a native buffer that they can then avoid copying.  If you consider a single 1024 x 768 JPEG, storing the data in a Vector<unsigned> and then copying it to a native storage location costs 3 MB and the cpu time to copy.  Animated GIFs can be far worse than this since they have many frames, although the decoders go to some effort to scavenge memory for large GIFs.  And finally pages can have many images.  If you start paying 1 - 100 MB of extra memory, plus the time to memcpy it, on a page, you will quickly understand why the current design is useful.  And for backends that don't want to take the time to use this model yet, it still accommodates the Vector<unsigned> implementation quite well.

Now, since I read Holger's main concern as the existence of #ifdefs in ImageDecoder.h, I think there are ways to avoid them.  I'm thinking of pulling RGBA32Buffer out into a separate header that can live in graphics/ instead of image-decoders/, which I hope would make Holger a bit more comfortable.  Given the little I have heard about ImageBuffer, I think its goals are probably similar enough to RGBA32Buffer that the two can be combined into a single class that represents a single native frame with per-pixel write access.  An Image can then be one or more of these frames and an ImageDecoder can write into an image or its frame(s).  The exact details will take some care to avoid memory corruption; over the lifetime of the Chromium port we've managed to subtlely break image memory management several times, because it's quite tricky to get right.  I will also need to better understand the image caching system.

I don't know whether ImageSource in its current state has much utility or should be folded into another class, but I certainly like Holger's idea of collapsing all the separate implementaions into one for starters.  Another piece of work I know needs to be done is to make the BMP decoder decode on demand rather than when setData() is called; I wrote that decoder a couple years ago before I fully understood the ImageDecoder class, so it works but can waste CPU/memory.  Luckily BMP and ICO are not too common on the web so this doesn't turn out to be a huge headache.

I hope the suggestions above not only provide concrete ideas to move forward but go at least some way towards satisfying Holger's concerns.  While I don't think his patches on this bug are valid, I do think we can avoid #ifdefs in ImageDecoder if that's a real sticking point for any ports.  In the meantime, please let me know if the current system actively hinders any ports; comment 3 seems to imply that it is a problem for a number of people but I haven't heard specific feedback from any of them.  By contrast, the GTK and wx folks were very helpful last night on IRC in pointing out exactly what was and wasn't working, so I was able to work with them.

[Holger Freyther]

Unforking Chromium meant breaking Gtk+, WX, EFL, Torchmobile Windows CE. The cost of you "unforking" is that _now_ every port needs to copy and paste (fork) +115 lines of code. It is not a fair trade.... there is no reason to create this mess.

Performance: As with every thing it depends. For Skia and even the current Cairo usage I agree that the additional malloc, memcpy, free create problems in terms of memory copying and memory fragmentation and still in some cases this doesn't matter... E.g. you might have a memory strategy where you keep the compressed image in memory and move the uncompressed image to another process/system (e.g. GL, or Xserver memory) or you have API that can operate on the allocated memory without a memcpy.

And I strongly discourage forcing every port to copy and paste the exact +115 lines of RGBA32Buffer.cpp for absolutely no gain about the status quo of yesterday. It is no acceptable solution to force this duplication on ports.


Your changes have so many conceptually weaknesses that you make every other port suffer and this needs to change. I see you have a hardtime admitting that there is _no_ improvement for cairo, wx, EFL, yet. Instead you talk about a mystique future improvement... Copy and Pasting RGBA32Buffer is not the future. You should evaluate how to adjust the ImageBuffer interface and make ImageDecoder truly corss-platform again bringing your improvement to everyone with client side image memory.

Until then I strongly urge to unbreak the damage that was done to RGBA32Buffer.

[Holger Freyther]

(In reply to comment #14)
> I haven't given explicit "no" replies to every single patch, but that's because
> I think all but the ImageSource collapsing are really a single patch that
> simply undoes all of the ImageDecoder unforking.  You have never stated how you
> intend these ports to obtain acceptable performance or what concrete benefits
> this has.  I have worked with the image decoders for several years and know
> enough to say that this is a damaging set of patches that we should not land. 
> I think your only motivation is a mistaken sense of some sort of
> "cross-platform" idealism that fails to account for the real constraints here.

You impose a big penalty on other ports for _no_ performance gain at all. You force to copy and paste code just to unfork Skia. You could nuke RGBA32Buffer completely in favor for extending ImageBuffer.

For your performance statement... let me cite the cairo documentation for me.

 * @data: a pointer to a buffer supplied by the application in which
 *     to write contents. This pointer must be suitably aligned for any
 *     kind of variable, (for example, a pointer returned by malloc).
 *
 * Creates an image surface for the provided pixel data. The output
 * buffer must be kept around until the #cairo_surface_t is destroyed
 * or cairo_surface_finish() is called on the surface.  The initial
 * contents of @buffer will be used as the initial image contents; you
 * must explicitly clear the buffer, using, for example,
 * cairo_rectangle() and cairo_fill() if you want it cleared.

so even for Cairo no _more_ changes than using a Vector<char> are needed (unless I'm mistaken). So for no gain at all we have to live with a copy and paste of RGBA32Buffer... it is no gain at all... the only gain for you is to have unforked Chromium at the cost of forcing everyone else to copy and paste.

[Brent Fulgham]

(In reply to comment #18)
> Unforking Chromium meant breaking Gtk+, WX, EFL, Torchmobile Windows CE. The
> cost of you "unforking" is that _now_ every port needs to copy and paste (fork)
> +115 lines of code. It is not a fair trade.... there is no reason to create
> this mess.

Can we please tone down the hyperbole?  Let's review the actual events:
1.   Unforking only broke GTK+ very temporarily.  By the end of the day (PST) Friday the Windows Cairo and GTK+ code was in great shape and working better than ever.
2.  Once Kevin Ollivier was available on-line, Peter was able to get the wxWidgets port working properly and I believe things are fine there as well.
3.  As for the EFL and Windows CE ports, these are basically Qt ports, so this sounds akin to arguing that "pkasting broke the Qt port, and he also broke the Qt port and the other Qt port to boot."

I don't think it's reasonable to demand source-level compatibility with projects that are not part of the WebKit tree.  Do you expect developers to scour the web searching for WebKit forks that might be affected by changes?  Should we feel guilty about breaking the code for groups that don't respond on the mailing lists and don't communicate on IRC?  For that matter, where is the Qt build bot to help ensure that although architectural issues might be introduced, at least build and stability regressions will not occur?

Finally, I disagree that "every" port must create a forked file.  It is more accurate to say that "some graphical backends needs an implementation".

[Brent Fulgham]

(In reply to comment #5)
> Created an attachment (id=31259) [review]
> Make ImageDecoder.cpp cross-platform
> 
> There is no need to force every port to have the exact same implementation of
> RGBA32Buffer, the only symbol that is platform depended is the conversion to a
> NativeImage and that does not belong into the buffer class anyway.

I think there is a good reason for these methods to be backend-dependent.  As with Skia, the Cairo port will refer to the internal image buffer (rather than the array of bytes) to provide the implementations for these method calls.

I agree it looks a little silly right now, since the wx and Cairo ports are nearly identical, but I am planning to revise the Cairo version of this file to use internal Cairo buffer/image handling routines which will remove the seeming duplication here.

[Brent Fulgham]

(In reply to comment #7)
> Created an attachment (id=31261) [review]
> Unfork the WX, no need to have an exact copy of the RGBA32Buffer... use the
> common code

This is related to the issue in comment #5 -- they are identical *today*, but in a few days they will NOT be the same.

I don't believe this change is desirable.

[Peter Kasting]

(In reply to comment #20)
> 3.  As for the EFL and Windows CE ports, these are basically Qt ports, so this
> sounds akin to arguing that "pkasting broke the Qt port, and he also broke the
> Qt port and the other Qt port to boot."

Plus, I asked repeatedly both on bugs and IRC for input from Qt port maintainers to ensure that my changes didn't break Qt, and it was my impression that Qt _isn't_ broken.  None of the Torchmobile or EFL people have asked for help in any way.  The only bustage to Qt I'm aware of was the the temporary bustage about 48 hours ago that I _had already said I was planning to fix_ and that _was_ in fact worked around with a roughly 3-line patch (not a "115 line copy").

These comments boil down to "you kept other ports working by copying a few functions' implementations temporarily.  THIS IS AN ENORMOUS PENALTY!"  I think it's clear that this isn't an "enormous penalty" or in fact much of any penalty, since it applies to exactly _two_ ports (not CG, the most widely used port as part of Safari; not Skia, the second most widely used port as part of Chromium; and not QT, the port I broke so apparently badly), and in fact _I_ was the one that made the changes for those other ports, not those ports' maintainers (although kollivier on IRC last night said the changes "sounded easy" before I went ahead and wrote the wx patch, so even that wasn't a big deal).  I could alternately have provided an ImageDecoder.cpp that implemented these two ports' functions for things they have in common at the moment, but this seemed like just causing the ports additional hassle since as Brent said in comment 21 things are about to change even more.

In summary, Holger, you are arguing that I have caused great pain to wx and Cairo, but the only thing I've caused is my _own_ patches to implement some functions that are shortly going to differ between the two ports, the Cairo maintainer doesn't agree with your position, and the wx maintainer has not spoken.  And you give little indication as to whether the idea of moving RGBA32Buffer into a class in graphics/ would satisfy you at all: some of your comments sound as if it would, but they are so tangled in comments about how I must reverse my patches (rather than simply move toward merging these two classes, which is surely easier now if I've actually made RGBA32Buffer more like ImageBuffer) that I don't know what to think.

This isn't productive dialog at all.  Be reasonable and stop trying to use other ports as weapons against me when their maintainers disagree with you or are silent.

[Kevin Ollivier]

I don't really want to get in the middle of all this, but for the record, I really feel that only people who actually build and run the wx port are entitled to speak for it. I'm certainly not happy about port breakages, but that's really a side-effect of not being on the build bot, so I don't hold committers personally responsible for breakages they can't easily become aware of. 

As for the rest, honestly, I just don't see what is so contentious about this change. I certainly haven't seen any evidence (e.g. benchmarks, etc.) of the kind of performance or functional regression that would warrant a revert. The rest of the criticisms seem to be issues of design and/or style, and while platform #ifdefs aren't pretty, they aren't exactly forbidden in WebCore, so I really don't see how their existence is somehow grounds for revert. It looks like some interesting ideas have already been floated about in regards to removing them, though, so I think it's a shame that the focus of this discussion has mostly been on reverting rather than on a patch (or patches) to improve what's already there. 

[Holger Freyther]

(In reply to comment #23)
> (In reply to comment #20)
> > 3.  As for the EFL and Windows CE ports, these are basically Qt ports, so this
> > sounds akin to arguing that "pkasting broke the Qt port, and he also broke the
> > Qt port and the other Qt port to boot."

I don't even know where to start to get the record straight...

1.) EFL is using EFL... and not Qt...
2.) Torchmobile is a Windows CE port
3.) Me using image-decoders on Qt, yes is Qt related...


> Plus, I asked repeatedly both on bugs and IRC for input from Qt port
> maintainers to ensure that my changes didn't break Qt, and it was my impression
> that Qt _isn't_ broken.  None of the Torchmobile or EFL people have asked for
> help in any way.  The only bustage to Qt I'm aware of was the the temporary
> bustage about 48 hours ago that I _had already said I was planning to fix_ and
> that _was_ in fact worked around with a roughly 3-line patch (not a "115 line
> copy").

I don't event want to go in the process. Landing patches with a reviewer raising his flag is not nice and the responses I got on irc are not making me feel comfortable about Chromium at all. But leave that out for now.



> 
> These comments boil down to "you kept other ports working by copying a few
> functions' implementations temporarily.  THIS IS AN ENORMOUS PENALTY!"  I think
> it's clear that this isn't an "enormous penalty" or in fact much of any
> penalty, since it applies to exactly _two_ ports (not CG, the most widely used
> port as part of Safari; not Skia, the second most widely used port as part of
> Chromium; and not QT, the port I broke so apparently badly), and in fact _I_
> was the one that made the changes for those other ports, not those ports'
> maintainers (although kollivier on IRC last night said the changes "sounded
> easy" before I went ahead and wrote the wx patch, so even that wasn't a big
> deal).  I could alternately have provided an ImageDecoder.cpp that implemented
> these two ports' functions for things they have in common at the moment, but
> this seemed like just causing the ports additional hassle since as Brent said
> in comment 21 things are about to change even more.

In a way I fundamentally disagree with.
 
> In summary, Holger, you are arguing that I have caused great pain to wx and
> Cairo, but the only thing I've caused is my _own_ patches to implement some
> functions that are shortly going to differ between the two ports, the Cairo
> maintainer

which is alp and me as we inherited it and not brent!




> This isn't productive dialog at all.  Be reasonable and stop trying to use
> other ports as weapons against me when their maintainers disagree with you or
> are silent.

Sigh.


To summarize:

I disagree with the changes to RGBA32Buffer. I think Skia should move towards making ImageBuffer suitable for use and nuke RGBA32Buffer all together or make truly cross platform. It is no acceptable temporarily solution to create a copy of clones (cairo, wx, Qt... whatever port)... it is moving in the wrong direction and my patch is reinforcing it.


And second. There is no reason to change the default behavior (no reason to #ifdef for CAIRO,WX), it should work for everyone (with eventually some performance impacts):

   1.) sometimes the decoded image is just copied somewhere else
   2.) some graphics libraries (e.g. cairo) allow you to adopt the memory and there will be exactly one conversion to image triggered the BitmapImage::cacheFrame.

and again my patch is restoring sanity.


So from my point of view your changes shouldn't have landed as they are a step back and not one ahead. The way ahead is to move the code over to ImageBuffer which I'm very supportive of and until then I want the sanity of the old solution back.

[Brent Fulgham]

(In reply to comment #25)
> I don't even know where to start to get the record straight...
> 
> 1.) EFL is using EFL... and not Qt...
> 2.) Torchmobile is a Windows CE port
> 3.) Me using image-decoders on Qt, yes is Qt related...

I based the EFL comment on the EFL Wiki ("EFL WebKit is a project aiming at porting this WebKit to the Enlightenment Foundation Libraries, and as the developers also work on the Qt port, we try to keep the implementation as close as possible to that of QtWebKit.").  I guess on closer reading that the Qt stuff was not actually related to the EFL work.

Regarding Torchmobile, I ran across "Our source code is now imported into the git server at git://code.staikos.net/ alongside the QtWebKit and Torch Mobile Qt WebKit code, and we hope to move active development into git in the near future.", but I now see that there are two Torch Mobile ports.

[Holger Freyther]

(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #20)

Or in more simple words:
      1.) The WebCore/image-decoders where cross-platform without a performance impact on Cairo
          (as the image data was not copied and the cairo_image_surface was created exactly once, at least to what I think is right)

      2.) Now we have multiple copies of RGBA32Buffer.cpp and every port that wants to use the decoders need to copy it. Which makes the WebCore/image-decoders not cross-platform

      3.)
         a) Make them cross-platform again (this series) as they are just fine for Cairo (again up to my knowledge).
         b) Make them fast and cross-platform for everyone (including Skia) by using ImageBuffer.


We are at 2nd, I did 3a, you can do 3b and the amount of users does not matter, we are at a technical issue here (otherwise one should work on the IE Team).

[Holger Freyther]

(In reply to comment #26)
> (In reply to comment #25)
> > I don't even know where to start to get the record straight...
> > 
> > 1.) EFL is using EFL... and not Qt...
> > 2.) Torchmobile is a Windows CE port
> > 3.) Me using image-decoders on Qt, yes is Qt related...
> 
> I based the EFL comment on the EFL Wiki ("EFL WebKit is a project aiming at
> porting this WebKit to the Enlightenment Foundation Libraries, and as the
> developers also work on the Qt port, we try to keep the implementation as close
> as possible to that of QtWebKit.").  I guess on closer reading that the Qt
> stuff was not actually related to the EFL work.
> 
> Regarding Torchmobile, I ran across "Our source code is now imported into the
> git server at git://code.staikos.net/ alongside the QtWebKit and Torch Mobile
> Qt WebKit code, and we hope to move active development into git in the near
> future.", but I now see that there are two Torch Mobile ports.


And just for the record. I don't mind source level breakage. Apple is doing it all the time and it is the right thing to do as things progress in a good way. I just have so many issues with the RGBA32Buffer approach, technically  it is not the right one(tm).

[Peter Kasting]

I had a long reply, but I see no purpose in it.  I will restrict my comments to this:

(In reply to comment #28)
> And just for the record. I don't mind source level breakage. Apple is doing it
> all the time and it is the right thing to do as things progress in a good way.

I find this disingenuous.

> I just have so many issues with the RGBA32Buffer approach, technically  it is
> not the right one(tm).

While I respect your position as a port maintainer, given your comments on IRC and the past couple bugs, I believe that the people disagreeing with you have superior knowledge of this particular area of the code and how the various ports have been affected.  As Kevin noted, there are no performance or bustage problems with my patch and I don't think your dislike of 83 lines (I counted) of common C++ implementation copied to _one_ additional location for the time being is a sufficient reason for reverting.  So I think we are simply going to have to disagree on the approach.

That said, given that you would like to see this code merged with ImageBuffer, I would like to ask (as a way of burying the hatchet) if you know the ImageBuffer code well enough to provide some sample patches on how this can be done.  Clearly you're capable of whipping up patches very quickly, and as I've already mentioned, ImageBuffer is a class I'm not as familiar with.  If you can help with this I'd be most grateful, as that was one of the reasons I reported the bug in the first place.

We can keep arguing about what to do in the interim, or we can try to work together to get to an end state that we can both be happy with.  Please, work with me here, not against me.

[Brent Fulgham]

(In reply to comment #17)
> Now, since I read Holger's main concern as the existence of #ifdefs in
> ImageDecoder.h, I think there are ways to avoid them.  I'm thinking of pulling
> RGBA32Buffer out into a separate header that can live in graphics/ instead of
> image-decoders/, which I hope would make Holger a bit more comfortable.  Given
> the little I have heard about ImageBuffer, I think its goals are probably
> similar enough to RGBA32Buffer that the two can be combined into a single class
> that represents a single native frame with per-pixel write access.  An Image
> can then be one or more of these frames and an ImageDecoder can write into an
> image or its frame(s).  The exact details will take some care to avoid memory
> corruption; over the lifetime of the Chromium port we've managed to subtlely
> break image memory management several times, because it's quite tricky to get
> right.  I will also need to better understand the image caching system.

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.

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.
 
> [...] but I certainly like Holger's idea of collapsing all the separate implementaions into one for
> starters.

Agreed.

[Peter Kasting]

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

[Brent Fulgham]

Looking some of this stuff over, the fact that getImageData always returns a copy seems inefficient.  Consider the code in svg/graphics/SVGResourceMasker.cpp::applyMask:

    PassRefPtr<CanvasPixelArray> srcPixelArray(m_mask->getImageData(intImageRect)->data());
    PassRefPtr<ImageData> destImageData(luminancedImage->getImageData(intImageRect));

    for (unsigned pixelOffset = 0; pixelOffset < srcPixelArray->length(); pixelOffset++) {
        unsigned pixelByteOffset = pixelOffset * 4;

        unsigned char r = 0, g = 0, b = 0, a = 0;
        srcPixelArray->get(pixelByteOffset, r);
        srcPixelArray->get(pixelByteOffset + 1, g);
        srcPixelArray->get(pixelByteOffset + 2, b);
        srcPixelArray->get(pixelByteOffset + 3, a);

        double luma = (r * 0.2125 + g * 0.7154 + b * 0.0721) * ((double)a / 255.0);

        destImageData->data()->set(pixelByteOffset + 3, luma);
    }

I *think* the srcPixelArray is a full copy of the data (based on the ImageBuffer source), but is only ever read.  I guess the copy is made to provide an "intImageRect"-sized window into the source, but I wonder if this copy could be avoided somehow.

What if the intImageRect is the size of the source image?  It seems like having a way to return an immutable view of the original image data would be useful.

[Peter Kasting]

(In reply to comment #27)
>           (as the image data was not copied and the cairo_image_surface was
> created exactly once, at least to what I think is right)

BTW Brent, I think this is correct, and may actually be a bug.  From my reading of some of the Cairo docs today, cairo_image_surface_create_for_data() doesn't copy the data, which means your returned surface is at the merge of the lifetime of the RGBA32Buffer -- which is managed by the ImageDecoder, which may delete it (see e.g. clear()).  I don't think it's safe to have the frames returned from the ImageSource depend on the vagaries of the ImageDecoder.  I assumed this code was copying since wx copies, Skia refs, and when we used to have a different backing implementation here, we had all kinds of problems until we ensured that the lifetimes were separate.

So you may want to see if you can write this in a way that allocates the buffer in some kind of a refcounted object.

[Holger Freyther]

(In reply to comment #33)
> (In reply to comment #27)
> >           (as the image data was not copied and the cairo_image_surface was
> > created exactly once, at least to what I think is right)
> 
> BTW Brent, I think this is correct, and may actually be a bug.  From my reading
> of some of the Cairo docs today, cairo_image_surface_create_for_data() doesn't
> copy the data, which means your returned surface is at the merge of the
> lifetime of the RGBA32Buffer -- which is managed by the ImageDecoder, which may
> delete it (see e.g. clear()).  I don't think it's safe to have the frames
> returned from the ImageSource depend on the vagaries of the ImageDecoder.  I
> assumed this code was copying since wx copies, Skia refs, and when we used to
> have a different backing implementation here, we had all kinds of problems
> until we ensured that the lifetimes were separate.

I'm not aware of any crash/issue with the Cairo backend, I don't remember seeing a crash in it since 2007... anyway. I take that as an ACK that the default implementation of Vector<PixelData> can be the "optimized" version for some platforms?

See, it would be so much easier if you would ack when you mess things up... Until now you are not even able to ACK that you made something that was cross-platform need platform specific code. Instead you defend it by saying "Copy and Paste ain't bad." and that Skia is the 2nd most often used platform. Please at least ack that you removed the cross platform property. 

See I honestly care about all ports and making their life more easy and I'm just afraid you continue this way (making code less portable).

 greetings from your general reviewer, Gtk+, Qt maintainer and janitor.

PS: Maybe just ack that your changes made things less portable?

[Peter Kasting]

(In reply to comment #34)
> anyway. I take that as an ACK that the
> default implementation of Vector<PixelData> can be the "optimized" version for
> some platforms?

No, I don't think the existing implementation, as-is, is appropriate for any platform.

However, I will certainly acknowledge that I believed the Cairo call in question copied its source pointer when it in fact didn't.

> See, it would be so much easier if you would ack when you mess things up...

I would prefer if you did not take a patronizing tone with me.

> See I honestly care about all ports and making their life more easy

I guess I will take your word for that.  Perhaps you could take my word that that is also the motivation for these changes.  And perhaps we could please, please, for crying out loud, stop discussing bug 25709 and start talking about something productive.

[Adam Treat]

<quote>While I respect your position as a port maintainer, given your comments on IRC
and the past couple bugs, I believe that the people disagreeing with you have
superior knowledge of this particular area of the code and how the various
ports have been affected.</quote>

This looks like an argument from authority to me and was in response to a technical argument made by Holger that the RGBA32Buffer class should, in fact, reflect its actual name.  Also, it is not clear to me that the argument to authority in this particular instance even works conceptually.  Regardless, I believe technical arguments should be given technical responses rather than relying upon logical fallacies.

From my vantage it seems that Holger has raised specific qualms with the design introduced by these recent changes and met with the following responses:

1) The changes are already in the tree...
2) It fixes a performance problem in some ports (although I haven't seen a rebuttal to Holger's point that the GTK+ port did not suffer such a problem.)
3) Changes that will be made in the future will address the design problem.

None of these seem to be responsive to me.  One is clearly not an answer. Two might well be true, but this doesn't absolve the changes with regard to the stated design problem.  Three is also clearly not an answer.

Whatever the merits of the recent changes I think Holger's objections based on a fundamental design problem should be given due attention and resolved before proceeding.

[Kevin Ollivier]

(In reply to comment #36)
> <quote>While I respect your position as a port maintainer, given your comments
> on IRC
> and the past couple bugs, I believe that the people disagreeing with you have
> superior knowledge of this particular area of the code and how the various
> ports have been affected.</quote>
> 
> This looks like an argument from authority to me and was in response to a
> technical argument made by Holger that the RGBA32Buffer class should, in fact,
> reflect its actual name.  Also, it is not clear to me that the argument to
> authority in this particular instance even works conceptually.  Regardless, I
> believe technical arguments should be given technical responses rather than
> relying upon logical fallacies.

I have to say that a problem with the naming of the RGBA32Buffer class doesn't sound like a technical argument at all to me, as different people may conceptualize tbe name differently, and I'd like to know what empirical technical data could be used to prove one side or the other's conceptualization as correct.

Technical arguments can be proven or disproven with performance benchmarks, unit tests, or other empircal data, and I have yet to see anything of the sort that shows a technical problem in Peter's design. Do you or Holger have such empirical data to show? Because you have not yet done so according to the bug tracker. Without empirical data, what's left is people's opinions, and frankly I think mostly this has been a long debate about a difference of opinion that has gotten really heated. In that case, I think what's best is for everyone to take a step back and cool down and re-think whether this whole debate over reverting is really the only way to go about resolving the problem, or whether it can really have a productive outcome. 

[Brett Wilson (Google)]

(In reply to comment #34)
> See I honestly care about all ports and making their life more easy and I'm
> just afraid you continue this way (making code less portable).
> 
>  greetings from your general reviewer, Gtk+, Qt maintainer and janitor.
> 
> PS: Maybe just ack that your changes made things less portable?

I was the main maintainer of the Chromium image decoders before Peter was. I had originally moved them completely out of the WebCore port directory with the intention of not sharing with other platforms at all since I figured it might be too much bother.

Instead Peter is spending a bunch of time trying to unify our code (which, though in some places is quite messy, generally works much better than ports other than CG). There is no reason for this work other than helping other ports in WebKit as far as I can tell, and your accusations are generally upsetting to me.

[Brent Fulgham]

(In reply to comment #34)
>  greetings from your general reviewer, Gtk+, Qt maintainer and janitor.
> 
> PS: Maybe just ack that your changes made things less portable?
> 

Hi Holger -- Setting all that aside, do you have any suggestions regarding Peter's request regarding ImageBuffer modifications:

> [...] [do] you know the ImageBuffer code well enough to provide some sample patches on how
> [merging RGBA32Buffer and ImageBuffer] can be done?  Clearly you're capable of whipping up
> patches very quickly, and as I've already mentioned, ImageBuffer is a class I'm not as familiar with.
> If you can help with this I'd be most grateful, as that was one of the reasons I reported
> the bug in the first place.

I also don't know this area as well as I would like, and I think you could help us avoid any further architectural
faux pas that don't help us advance the project.

[Peter Kasting]

(In reply to comment #36)
> Whatever the merits of the recent changes I think Holger's objections based on
> a fundamental design problem should be given due attention and resolved before
> proceeding.

I have spent over twenty hours in the past two days trying to respond to Holger's concerns.  If I have not given them "due attention" it has certainly not been for lack of trying.

While I tend to agree with Kevin Ollivier's recent comment that this is more an issue of semantics that cannot truly be resolved with facts, I will do my best to elaborate my responses to anything technical I can discern:

As far as I understand it the specific concern raised is that RGBA32Buffer should use a Vector<unsigned> as its low-level storage type.  The reason for this seems to be:
(1) That's what it was doing before
(2) This is sufficient for all ports

My answer to this technical concern was that (1) is not really a justification for anything and (2) is not accurate.  Vector<unsigned> is not usable by the Skia port today, which is the primary concern; it is in my opinion inappropriate for the wx port long term and not the best design for the Cairo port, though these are secondary issues.  (I was about to write up some patches which would, in fact, convert these ports to using a different backing store, so the question of "future" work here was not very abstract.)

There was an additional concern raised that other ports now needed to do extra work to use the image decoders.  I don't believe this is a practical objection as I am aware of no other ports making such an effort.  I don't feel a design should be based on elevating the concerns of hypothetical other ports over the real needs of existing ports.

There was an additional concern raised that using NativeImagePtr from ImageDecoder.h is a "layering violation".  I have worked with the image decoders for several years now and have never been made aware of any layering requirement to avoid this type.

Both sides expressed interest in merging the functionality of RGBA32Buffer and ImageBuffer and the proposed difference in action seemed solely to be, whether or not to revert some changes in ImageDecoder.* before doing this work.  Given that I see no gain from doing this, I don't understand how this is anything other than a time cost and a hindrance to doing the work.

In summary, I believe the technical concerns raised were mainly that the work done on bug 25709 decreased the "cross-platformness" of ImageDecoder and related classes, to which my response was that these were certainly not sufficiently cross-platform before to account for all platforms, but now are; and that the changes made to accommodate ports like Skia were not harmful to other ports (and thus should be backed out and rethought) but instead helpful (now, for minor things like removing ImageSourceXXX.cpp files, and in the future, for writing clearer or more efficient backend storage functions).  As far as I know Holger continues to disagree with me on this and I do not know how to further resolve this disagreement, which is why I proposed attempting to reach a state where ImageBuffer and RGBA32Buffer are merged as quickly as possible.

I hope this is helpful.

[Adam Treat]

I appreciate that a lot of time has been spent trying to rectify the disagreements over the scope of these changes and that everyone involved is earnestly looking for the correct and proper way forward, but respectfully, I don't think the issue is simply one of the now improper name of RGBA32Buffer.  Were that the case, RGBA32Buffer could simply be renamed to something more appropriate.  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.

From a technical perspective let's list the pros and cons mentioned as we currently stand:

Pro:
1) As Skia is currently written (to my knowledge) this is a definite performance advantage as it removes the necessity for a copy of the buffer into the native Skia bitmap type.
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.

Con:
1) We lose flexibility in the image-decoders as they now depend on platform/graphics
2) Duplication of effort with ImageBuffer
3) Merging with mozilla versions becomes harder
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.

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?

RE: Kevin 

Techical arguments do not always require empirical data of the sort you are requesting.  Patches are often turned down based on technical reasons other than a lack of unit tests or benchmarks.

RE: Brett

Peter has made very valuable contributions to the image-decoders.  No one disputes that.  However, _so far_ I have a hard time seeing the benefits to the other ports in the recent changes other than the opportunity to share the code with the Skia port.  Maybe I'm not reading something correctly...

RE: Peter

I don't think you can honestly object that the changes _have_ made the image-decoders less _cross-platform_.  Where before the code was not particularly dependent on any platform, now it is.  Anyways, can you elaborate on the pro #2 above and the related question of whether it'd be sufficient to introduce a new ctor to SkBitmap instead to operate on the underlying RGB32Buffer without a memcpy?

[Brent Fulgham]

(In reply to comment #41)
> 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.

As Holger pointed out, Cairo can 'adopt' memory (without copy) so there does appear to be less of a performance impact than initially thought.  However, there might be some advantages gained by calling some of the cairo_surface_t-based calls rather than performing math on the raw vectors, and I think this should be considered as a potential performance gain.

> Con:
> 2) Duplication of effort with ImageBuffer

Well, the point of this current bug is to move much of the RGBA32Buffer logic to ImageBuffer, so perhaps this isn't really a con so much as an active development goal.

> RE: Brett
> 
> Peter has made very valuable contributions to the image-decoders.  No one
> disputes that.  However, _so far_ I have a hard time seeing the benefits to the
> other ports in the recent changes other than the opportunity to share the code
> with the Skia port.  Maybe I'm not reading something correctly...

We now have image-decoder implementations for BMP, XBM, and ICO, which did not previously exist.  GTK+, wxWindows, and Cairo can now use these implementations which I do consider to be benefits to these other ports.

[Peter Kasting]

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

[Adam Treat]

> 
> We now have image-decoder implementations for BMP, XBM, and ICO, which did not
> previously exist.  GTK+, wxWindows, and Cairo can now use these implementations
> which I do consider to be benefits to these other ports.
> 

While this is true, I don't think (and please correct me if I'm wrong) these decoders are dependent on the objected parts of the recent changes.  Thus, it is kind of sideways to the discussion :)

[Brent Fulgham]

(In reply to comment #44)
> > We now have image-decoder implementations for BMP, XBM, and ICO, which did not
> > previously exist.  GTK+, wxWindows, and Cairo can now use these implementations
> > which I do consider to be benefits to these other ports.
> 
> While this is true, I don't think (and please correct me if I'm wrong) these
> decoders are dependent on the objected parts of the recent changes.  Thus, it
> is kind of sideways to the discussion :)

These implementations were Skia-specific, so the expected interface (via ImageDecoder.h) was the Skia-specific style.  Peter's changes to ImageDecoder brought these more in line with the Skia version, allowing a single set of image-decoders (including these new ones).

The desire to roll back all of the changes would mean a loss of these new decoders, at least until they had been revised to work with the original style image decoder.

I would really prefer not to roll any of this back, and instead try to address the various concerns with the current source base and move forward.

My understanding is that Holger's proposal to undo all of the original bug's changes would mean a loss of this new functionality.  I strongly oppose this.

[Peter Kasting]

(In reply to comment #45)
> The desire to roll back all of the changes would mean a loss of these new
> decoders, at least until they had been revised to work with the original style
> image decoder.
> 
> I would really prefer not to roll any of this back, and instead try to address
> the various concerns with the current source base and move forward.
> 
> My understanding is that Holger's proposal to undo all of the original bug's
> changes would mean a loss of this new functionality.  I strongly oppose this.

I don't think this is actually what Holger is asking for.  I think the disputed pieces of the code could probably be modified without losing the new decoders.

My own objection to Holger's proposal is not along the lines of losing new image decoders but rather of gaining no practical advantage (see my detailed responses to Adam Treat's list of cons above) and yet losing clarity and simplicity.  Basically, I claim it makes moving forward harder and costs time to what I see as no good purpose (obviously Holger disagrees).

[Adam Treat]

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

[Eric Seidel (no email)]

So the debate boils down to if it's OK to add a image abstraction around the Vector<unsigned>.  Correct?  RGBA32Buffer used to be a thin layer you could grab through.  Now Peter's changed it to be an image abstraction, like ImageBuffer.

I don't see why this breaks ports?  Can't we have a default ImageBuffer which uses Vector<unsigned> under the covers?  Although it seems that would only be needed by platforms which don't provide an Image type to decode into, or?

For what it's worth, CoreGraphics (what I tend to use as my reference for graphics APIs), decodes into an opaque image type:
http://developer.apple.com/documentation/graphicsimaging/reference/CGImage/Reference/reference.html#//apple_ref/c/func/CGImageCreate

I feel much more informed after all this, and am looking forward to peter's next round of patches to share more platform/graphics code between all ports.

[Peter Kasting]

(In reply to comment #47)
> 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?

Brent posted some ideas at the beginning of comment 42.  Besides those (if they pan out), the lifetime issue, and the forward motion I proposed with the classes as a whole, no, there are not further benefits to Cairo I am aware of.  But I think those are not things that should necessarily be discounted.

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

Thanks, this is helpful background for me.

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

I am not sure how to address it with a design that writes to a Vector<unsigned> unless either (a) the ImageSource is invoked to provide the vector, so that it is managing the lifetime (seems cumbersome) or (b) the data is copied-on-exit, like wx currently does, which has performance issues.

As you say though, it is very hard to quantify this.

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

This is precisely the sort of optimization I'm attempting to enable, by allowing a port where this matters (for example, a mobile device with low memory) to downsample inside the functions that write data to the backing store.  I don't see this class of optimizations as something we'd ever want to apply globally to all ports.  Perhaps I am mistaken?

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

I don't think moving to a per-platform storage system necessarily prevents this if it ever wants to occur.  We already have APIs (that I added) that abstract writing the pixel data as the decoders needed, it would certainly be possible to add abstractions to read it as well (and simple, given that all these systems give users ways of getting at the underlying data buffer directly).  So I don't think this is lost flexibility.

Even if I am mistaken that we lose no flexibility here, I am hard-pressed to think of realistic probable modifications along the lines you suggest.  I'm also very reluctant to optimize a design for a low-likelihood future possibility that is not yet present versus current needs.  From the larger perspective of the image system as a whole, is seems clear that writing pixel elements into some sort of storage and being able to display the result is work that was being done in multiple places, and it seems like trying to combine those is sensical.

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

Note that my intent was to work on this "in the next few days", not at an unspecified future date.  I filed this bug in hopes of getting the precise technical details needed to do this work immediately.  So the question should only be whether we even want to see this end state happen at all.

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

But I have already mentioned that certainly wx does have concrete perf impact from the current design, so at the very least it would _want_ to move away from the "cross platform" code.  At that point, whether or not Cairo used Vector<unsigned>, the implementation would only be used by Cairo.  And of course this still sets aside the three points mentioned at the top of this replay as to possibilities why this might make sense.

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

As I said, we saw concrete issues with Skia here during our testing.  It was well over a year ago so I no longer recall how to reproduce.  Certainly without a reproduction case I cannot say whether it would affect other ports; it could be that it was due to an interaction of this code with other parts of the Skia image framework.

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

I do not know wx APIs well enough to say whether this is in fact true.

Note that moving asNewNativeImage() from RGBA32Buffer back to ImageSource prevents completely unifying the ImageSource class into one that is entirely cross-platform.  To me this makes the question feel somewhat like "six of one, a half-dozen of the other".

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

...And the con of basically closing down any ability to further simplify the overall image system architecture.

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

I do not know for Wx.  For Skia I would be unwilling to do this modification without guaranteeing the data lifetime irrespective of the ImageDecoder implementation.  As I said above, I am not convinced the solution to that is clean and easy.  Given that I believe the current system to be equally flexible to the old system (modulo writing a call to read data out), the benefit seems to me to be zero while the costs (ignoring the actual work to do this) are nonzero.  As always, I could be wrong.

A possibility that eseidel alludes to somewhat above is that I could continue by attempting to merge RGBA32Buffer and ImageBuffer while seeking to maintain the current Vector<unsigned> as the low-level implementation of the pixel storage in this class, at least for the time being.  In this scenario I would probably collect the Cairo and wx implementations into one, but leave Skia separate (and still using an SkBitmap) for now.  I'm not sure exactly who allocates the storage when in this model but I'm sure that could be worked out, perhaps ignoring the lifetime issue (which does not seem to be affecting wx and Cairo in practice at the moment anyway).  Given that due to not using a Vector<unsigned for Skia, ImageDecoder would still have to speak to an (apparently) opaque backing store class (the unified ImageBuffer/RGBA32Buffer) I'm not sure this satisfies the concerns raised, but perhaps it does.

Thank you again for your detailed replies.  This is the sort of background that is helpful to me.

[Eric Seidel (no email)]

I talked more with pkasting over email, and Adam over IRC.  I'd like to talk more with Holger (zecke) over IRC, but Adam and I seem to agree that letting Peter continue with his changes is a good thing (at least not harmful).  I intend to obsolete all the patches on this bug once I can teach my fancy bugzilla-tool how to obsolete a patch. :)  Thanks for them, but my understanding is Peter intends to work on these further and plans to take things in a slightly different direction than your patches.

Holger, let's talk over IRC when you're next around.  I think we're close to an understanding here.

[Kevin Ollivier]

(In reply to comment #43)
> (In reply to comment #41)
> 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.

BTW, while I probably won't have time to delve too deeply into this over the next couple days, I did want to mention a possible source of confusion if you plan on looking at the wx port. 

As I mentioned in our discussion on IRC, the wx port has two different graphics rendering APIs. There is wxDC, a bitmap-based API which uses GDI, GDK and CoreGraphics APIs to draw, and wxGraphicsContext, a vector-based API which uses GDI+, Cairo and CoreGraphics to draw. Use of wxGraphicsContext is designated in wx code using #if USE(WXGC) blocks. Of course, whenever possible we want to use wxGraphicsContext since WebCore::GraphicsContext is based around a vector-based drawing API. Unfortunately, even if we use wxGraphicsContext, wxBitmap stores the bitmap data using the native type expected by wxDC, rather than wxGraphicsContext. This is why we do a conversion (and data copy, really) from wxBitmap to wxGraphicsBitmap in RGBA32Buffer::asNewNativeImage, because when USE(WXGC) is true (eventually always), we need the image data to be in a format usable by wxGraphicsContext. To make matters worse, the APIs for types like wxGraphicsBitmap are very minimal and essentially only allow conversion from the standard wx data type (e.g. wxBitmap to wxGraphicsBitmap), so there's currently no way to do things like get direct pixel access for reading or writing with these types. 

So, unfortunately, even if we use wxBitmap as the image buffer type, we will still have a copy operation to go from wxBitmap to wxGraphicsBitmap. My concern performance-wise is whether or not we can continue to efficiently cache the wxGraphicsBitmap type if the underlying wxBitmap buffer can change at any time. e.g. we'd need a way to know when the buffer has been 'dirtied' and needs a new wxGraphicsBitmap to be created from it, so that we're only creating new wxGraphicsBitmap types when necessary and not any time a wxGraphicsBitmap is being asked for. At the same time, we probably don't want to recreate it after every write operation, so we'd probably need some mechanism to return a cache if not dirty or recreate the image if so.

I realize this sucks, but it's what I've got to work with right now. :/ I can look at extending the wxGraphicsBitmap API, of course, but it will take some time to both implement and then have the change propagate into a release wx version, so we'll need a workaround in place in the short term.

RE: Adam

> Techical arguments do not always require empirical data of the sort you are
> requesting.  Patches are often turned down based on technical reasons other
> than a lack of unit tests or benchmarks.

Yes, but we're discussing a patch that has already gone through the review process and been approved, meaning that the person or persons who reviewed the patch did not consider whatever flaws they saw as serious enough to outweigh the benefit of getting the patch into the tree. (An assessment I'd agree with in this case.) While no one is perfect, given that, I think as a practical matter there should be a higher burden of proof to show that the flaws are serious enough to warrant a revert. 

Anyway, I think things are going in right direction now and people are getting on the same page, so it's probably a moot point now. :)

[Eric Seidel (no email)]

I spoke with Holger (zecke) over IRC just now.  I think we're all much closer to the same page now.  Peter will continue with his refactoring work.  Peter says that the above patches are unfortunately in conflict with his next set.  I've encouraged him to file a new bug to track the next round of refactoring work and mark this as a dupe.

Thank you again for your patches Holger, and your thoughts on this issue.

[Peter Kasting]

Holger, I want to extend a public apology to you for comment 14 on this bug which was completely inappropriate, as well as any other unprofessional comments I made on IRC, on bug 25709, or on this bug.  I am sorry for caring insufficiently about other ports in my original patch set, for losing my temper with you, and for being insufficiently informed up front to fully understand the alternatives to my design or the objections shared here.  For anything you have said which has been out of line, I completely forgive you.  I sincerely believe you have the best interests of the codebase as a whole at heart, and I am happy to have your input as a long-time contributor.

[Peter Kasting]

With that said, at eseidel's request I am closing this bug in favor of a new one, to try and gain clarity.  I have CCed most of the folks on this bug there, and have tried to cover things completely and without bias.  Please correct me where I have misstated or overlooked anything.


*** This bug has been marked as a duplicate of 26467 ***

[Adam Treat]

Comment on attachment 31258 [details]
Restore cross platform behavior

We've moved onto another bug report I believe and zecke has indicated a new direction is acceptable.

[Adam Treat]

Comment on attachment 31263 [details]
Unfork the various ImageSource*.cpp copies

I think there was agreement unifying the ImageSource copies floating around is the correct approach, but I don' think it applies cleanly now?

[Peter Kasting]

(In reply to comment #56)
> (From update of attachment 31263 [details] [review])
> I think there was agreement unifying the ImageSource copies floating around is
> the correct approach, but I don' think it applies cleanly now?

True, and I also found on closer inspection that the patch here overlooked some subtle differences between the sources.  I'm working towards unifying the ImageSources as we speak, I need to correct some issues with Skia's usage of the ICO decoder to do so.