Bug 25709 - Skia changes to image decoders should be merged into the cross-platform ones
Summary: Skia changes to image decoders should be merged into the cross-platform ones
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-11 17:25 PDT by Peter Kasting
Modified: 2009-08-14 08:53 PDT (History)
10 users (show)

See Also:


Attachments
Part one - v1 (108.23 KB, patch)
2009-05-11 17:39 PDT, Peter Kasting
eric: review-
Details | Formatted Diff | Diff
Part one - v2 (111.82 KB, patch)
2009-05-18 17:03 PDT, Peter Kasting
eric: review-
Details | Formatted Diff | Diff
part one, simplified (Landed r44051) (27.46 KB, patch)
2009-05-22 11:29 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part two (Landed r44167) (44.13 KB, patch)
2009-05-22 11:55 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Misc. work, not ready for review (31.56 KB, patch)
2009-06-08 16:17 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Misc. work, not ready for review, v2 (31.89 KB, patch)
2009-06-08 16:56 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Misc. work, not ready for review, v3 (31.89 KB, patch)
2009-06-08 17:06 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part three (Landed r44545) (12.72 KB, patch)
2009-06-08 17:20 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part four (Landed r44553) (30.27 KB, patch)
2009-06-09 16:49 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part five (Landed r44573) (12.76 KB, patch)
2009-06-09 21:01 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part six (Landed r44597) (19.09 KB, patch)
2009-06-10 17:10 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part seven (30.67 KB, patch)
2009-06-11 17:38 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part seven, v2 (30.69 KB, patch)
2009-06-11 17:59 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part seven, v3 (Landed r44631) (30.68 KB, patch)
2009-06-11 18:04 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part eight (266.93 KB, patch)
2009-06-12 15:44 PDT, Peter Kasting
eric: review+
Details | Formatted Diff | Diff
Free ImageDecoder.h from any knowledge about graphics libraries (3.72 KB, patch)
2009-06-13 06:54 PDT, Holger Freyther
darin: review+
Details | Formatted Diff | Diff
Free ImageDecoder.h from any knowledge about graphics libraries (2nd attempt) (4.08 KB, patch)
2009-06-13 08:05 PDT, Holger Freyther
darin: review+
Details | Formatted Diff | Diff
part nine (Landed r44652) (14.29 KB, patch)
2009-06-13 11:22 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part ten (Landed r44661) (22.37 KB, patch)
2009-06-13 16:06 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Do not silently cause crashes... (1.93 KB, patch)
2009-06-13 18:59 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
part eleven (9.56 KB, patch)
2009-06-13 20:14 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part eleven v2 (10.18 KB, patch)
2009-06-13 23:21 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
part eleven v3 (Landed r44669) (14.87 KB, patch)
2009-06-13 23:30 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-05-11 17:25:48 PDT
For expediency, the Chromium checkins included WebCore/platform/image-decoders/skia, which forked all the existing files in image-decoders/.  The Skia versions and the existing Cairo versions should be harmonized and merged into one.  As a side benefit, this will give Cairo working BMP/ICO/XBM decoders.
Comment 1 Peter Kasting 2009-05-11 17:39:54 PDT
Created attachment 30210 [details]
Part one - v1

This does lots of easy changes to get the two sets of files much closer together, mostly style fixes to the older Cairo code:
* Proper #include guard names
* Namespace names on the closing brackets
* Indent namespace contents in header files
* Class open brackets on same line
* Correct initializer list indenting
* NULL -> 0

Also a few other changes for better consistency:
* m_size -> size(), which is what the Skia code uses, and is slightly safer in the case of subclasses
* Add/move a couple trivial lines

After this patch, most diffs between the two code should be actual functional differences.
Comment 2 Eric Seidel (no email) 2009-05-14 21:52:10 PDT
Comment on attachment 30210 [details]
Part one - v1

Yay!

This patch is pretty big, making it difficult to review.

For example, all the places where you're only re-indenting code (like ImageDecoder.h) would be easier to review in a smaller patch...

Why is it OK to remove:
#if PLATFORM(CAIRO) || PLATFORM(QT) || PLATFORM(WX)
from BMPImageDecoder?  Is that file not included in the Windows build?  For a while WinCG and WinCairo were sharing a solution file...

WK Style:
 185         decode(GIFFullQuery, index+1); // Decode this frame.
would say index + 1 I think.

 407         unsigned data_size = num * sizeof(unsigned);
dataSize


What's the ADDRESS_TAG_BIT stuff you're removing?  You don't mention it in the ChangeLog...

I assume x is never used in this loop:
          for (unsigned i = 0; i < info->output_width; ++i) {
 483         for (unsigned x = 0; x < info->output_width; ++x) {

Style:
4 void PNGImageDecoder::decodingFailed() {
 225     m_failed = true;
 226 }

What's the skia/BMPImageDecoder.h change?  Line endings?
skia/BMPImageReader.h ?

Why is this OK?
 bool JPEGImageDecoder::isSizeAvailable()
421421         decode(true);
422422     }
423423 
424      return !m_failed && ImageDecoder::isSizeAvailable();
 424     return ImageDecoder::isSizeAvailable();
425425 }

This really mostly need more documentation in the ChangeLog.  And I'd prefer to land this in smaller pieces.

Otherwise, this looks fine.  I assume you ran the WebKit Windows (Apple's port) still built fine after this?

r-, mostly due to lack of explanation in the ChangeLog and the above nits.  Again, I'd rather land this in smaller pieces, but I'll happily review whatever you post next.
Comment 3 Peter Kasting 2009-05-18 17:03:21 PDT
Created attachment 30455 [details]
Part one - v2

This is nearly identical to the first patch, except for fixing "index+1" into "index + 1", and adding much more commentary to the ChangeLog, hopefully answering the questions from the first review.
Comment 4 Eric Seidel (no email) 2009-05-19 15:04:18 PDT
Comment on attachment 30455 [details]
Part one - v2

I honestly can't review this.  It's too big for my little brain.  I'd consider giving you a rubber-stamp and just trusting, but I can't actually review this whole change.  If you break it into smaller pieces (litterally just slicing the diff on file boundaries) I think you'd have little trouble finding many people willing to review this (including me).  It looks like a great bunch of cleanup!
Comment 5 Eric Seidel (no email) 2009-05-22 06:53:49 PDT
Comment on attachment 30455 [details]
Part one - v2

r-'d based on my above "too big to review" comments.  Thanks again for the patch and nicely updated ChangeLog.  I think just a little slicing and dicing will make all the difference (or if we had a better review tool...)
Comment 6 Peter Kasting 2009-05-22 11:29:46 PDT
Created attachment 30584 [details]
part one, simplified (Landed r44051)

OK, splitting the original patch into pieces.  Here's the first and largest piece.  The only thing this does is indent the namespace contents in the header files, to comply with the style guide.  There is no code change, this should be rubber-stampable.

I'm also going to ditch the DOS line endings stuff in the original patch since I think something is screwy with my client and that's actually not desirable.  So after this goes in, the remainder of the original patch may be small enough to be reviewable.
Comment 7 Peter Kasting 2009-05-22 11:55:07 PDT
Created attachment 30587 [details]
part two (Landed r44167)

Here's the remaining bits from the original "part one" patch, hopefully now small enough to review.
Comment 8 Eric Seidel (no email) 2009-05-22 19:31:21 PDT
Comment on attachment 30587 [details]
part two (Landed r44167)

style:
 407         unsigned data_size = num * sizeof(unsigned);
Maybe that's matching the style of the file?
style:
void PNGImageDecoder::decodingFailed() {
 225     m_failed = true;

Looks fantastic.
Comment 9 Peter Kasting 2009-05-22 20:57:26 PDT
(In reply to comment #8)
> (From update of attachment 30587 [details] [review])
> style:
> void PNGImageDecoder::decodingFailed() {
>  225     m_failed = true;

Not sure what style nit is being reported here?
Comment 10 Jan Alonzo 2009-05-22 21:28:36 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 30587 [details] [review] [review])
> > style:
> > void PNGImageDecoder::decodingFailed() {
> >  225     m_failed = true;
> 
> Not sure what style nit is being reported here?

I think the bracket should be in a separate line.

Comment 11 Eric Seidel (no email) 2009-05-22 22:25:34 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 30587 [details] [review] [review] [review])
> > > style:
> > > void PNGImageDecoder::decodingFailed() {
> > >  225     m_failed = true;
> > 
> > Not sure what style nit is being reported here?
> 
> I think the bracket should be in a separate line.
> 

Exactly:
http://webkit.org/coding/coding-style.html
Braces
Function definitions: place each brace on its own line.

We really need a WebKit linter. :(
Comment 12 Brent Fulgham 2009-05-26 17:21:49 PDT
Landed in @r44167.
Comment 13 Peter Kasting 2009-05-26 21:04:14 PDT
Not fixed yet, these patches are just the start :).

Did you correct both nits in comment 8?  If not I will address them in the next patch here.  Thanks for landing for me, I was out sick today.
Comment 14 Peter Kasting 2009-05-26 21:04:58 PDT
Comment on attachment 30587 [details]
part two (Landed r44167)

Obsoleting this now-landed patch so it doesn't show up in the landing queue.
Comment 15 Peter Kasting 2009-06-08 16:17:56 PDT
Created attachment 31069 [details]
Misc. work, not ready for review

This is for bfulgham to test the Cairo build.  Patch should be applied from inside WebCore/platform/image-decoders.
Comment 16 Peter Kasting 2009-06-08 16:56:45 PDT
Created attachment 31071 [details]
Misc. work, not ready for review, v2

Fixed a few compile errors.
Comment 17 Peter Kasting 2009-06-08 17:06:51 PDT
Created attachment 31072 [details]
Misc. work, not ready for review, v3

Sigh
Comment 18 Peter Kasting 2009-06-08 17:20:09 PDT
Created attachment 31073 [details]
part three (Landed r44545)

This is the Skia portion of the "misc. work" patch above, broken out separately for ease of review, with a commented ChangeLog.
Comment 19 Eric Seidel (no email) 2009-06-08 17:40:21 PDT
Comment on attachment 31073 [details]
part three (Landed r44545)

+          http://code.google.com/p/chromium/issues/detail?id=3643.

Can we just add the test as part of this change?  (I don't need to see the test, but you could land it with this.)

Would be nice to eventually add a setColor function which took an IntPoint and a Color instead of:
+            m_frameBufferCache.first().setRGBA(m_coord.x(), m_coord.y(), red, green, blue, alpha);

obviously not for this patch. :)

It's not clear to me from the ChangeLog why the removal of these lines:
-            buffer->setStatus(RGBA32Buffer::FrameComplete);

is correct.  But I trust you.

Very sad that setSize() doesn't take an IntSize :)

I'm assuming this:
-                      buffer->setRGBA(bitmap.getAddr32(x, y), 0, 0, 0, 0);
is not needed because x/y are already 32-bit checked or some such?  Unclear why the change.

I am not the right person to review this technically.  But I don't think that person exists in the set of webkit reviewers.  I trust here that you've checked any needed parts with people who do have Skia knowledge.

You have my rubber stamp.

Thank you for making this so small.
Comment 20 Peter Kasting 2009-06-08 18:12:50 PDT
(In reply to comment #19)
> (From update of attachment 31073 [details] [review])
> +          http://code.google.com/p/chromium/issues/detail?id=3643.
> 
> Can we just add the test as part of this change?  (I don't need to see the
> test, but you could land it with this.)

I've... um... never written a LayoutTest.  I could go find out how...

> Would be nice to eventually add a setColor function which took an IntPoint and
> a Color instead of:
> +            m_frameBufferCache.first().setRGBA(m_coord.x(), m_coord.y(), red,
> green, blue, alpha);
> 
> obviously not for this patch. :)

This could probably be changed eventually, in a pass to "clean up ImageDecoder APIs to use higher-level WK types as much as possible".

> It's not clear to me from the ChangeLog why the removal of these lines:
> -            buffer->setStatus(RGBA32Buffer::FrameComplete);
> 
> is correct.  But I trust you.

Because that setStatus() call has now moved into the RGBA32Buffer::setSize() implementation that all these places were calling.

> Very sad that setSize() doesn't take an IntSize :)

See above comment re: color.

> I'm assuming this:
> -                      buffer->setRGBA(bitmap.getAddr32(x, y), 0, 0, 0, 0);
> is not needed because x/y are already 32-bit checked or some such?  Unclear why
> the change.

The version that takes an (x, y) coordinate pair instead of a pointer is just safer and easier to read, that's all.

> I am not the right person to review this technically.  But I don't think that
> person exists in the set of webkit reviewers.  I trust here that you've checked
> any needed parts with people who do have Skia knowledge.

I've asked Brett Wilson to look the patch over.  I'll avoid committing until he OKs.
Comment 21 Brett Wilson (Google) 2009-06-09 14:38:03 PDT
Comment on attachment 31073 [details]
part three (Landed r44545)

I have almost no chance to find subtle issues in a patch like this, but I don't see any problems.

> +            return total_size > ((256 * 256 * 256 * 32) - 1);

I'm not clear why you did the computation this way since I can't tell what it means. How about (1 << 28) - 1?
Comment 22 Peter Kasting 2009-06-09 14:41:40 PDT
(In reply to comment #21)
> > +            return total_size > ((256 * 256 * 256 * 32) - 1);
> 
> I'm not clear why you did the computation this way since I can't tell what it
> means. How about (1 << 28) - 1?

I was just trying to break things into countable chunks (256 = 2^8, 32 = 2^5).  Your idea is better, I didn't think of it.  But I think you mean 1 << 29.

Will commit and then post some more stuff for review.
Comment 23 Peter Kasting 2009-06-09 15:01:43 PDT
Comment on attachment 31073 [details]
part three (Landed r44545)

Landed part three, clearing its flags.
Comment 24 Peter Kasting 2009-06-09 16:49:25 PDT
Created attachment 31112 [details]
part four (Landed r44553)

This is much of the remaining work on the Cairo side.  bfulgham already built and ran with this patch locally yesterday and verified that things still work as expected.
Comment 25 Brent Fulgham 2009-06-09 17:24:58 PDT
This patch looks good to me.  It applies cleanly and works properly under the Windows (Cairo) build.
Comment 26 Eric Seidel (no email) 2009-06-09 18:13:24 PDT
Comment on attachment 31112 [details]
part four (Landed r44553)

Looks sane enough to me.

Nits:
indent:
       if (clientptr) {
 521         if (!clientptr->sizeNowAvailable(screen_width, screen_height))
 522           return false;
 523       }

buffer.setRect(IntRect(0, 0, size().width(), size().height()));
setRect(IntRect(IntPoint(), size())) would be more concise.
Comment 27 Peter Kasting 2009-06-09 19:23:48 PDT
Comment on attachment 31112 [details]
part four (Landed r44553)

Landed, clearing flags.
Comment 28 Peter Kasting 2009-06-09 21:01:14 PDT
Created attachment 31123 [details]
part five (Landed r44573)

The main thing in this patch is a rewrite of Skia's GIFImageDecoder::haveDecodedRow() to not use raw pointers to the data inside the framebuffer, and instead do everything through coordinates.  This makes the code a lot more readable too (IMO).

After this patch, none of the Skia image decoders has anything Skia-specific or unimplementable by Cairo inside it.  I'll then need to write a similar patch for the Cairo side adding the Cairo versions of the new ImageDecoder.h functions, and then we can start actually merging the files together.
Comment 29 Eric Seidel (no email) 2009-06-10 00:34:48 PDT
Comment on attachment 31123 [details]
part five (Landed r44573)

Shouldn't we add some ASSERTS?
128         // NOTE: This function does not sanity-check its arguments!  Callers
 129         // MUST not pass invalid values or this will corrupt memory.
 130         void copyRowNTimes(int startX, int endX, int startY, int endY)
 131         {
 132           const int rowBytes = (endX - startX) * sizeof uint32_t;
 133           const uint32_t* const startAddr = m_bitmap.getAddr32(startX, startY);
 134           for (int destY = startY + 1; destY < endY; ++destY)
 135               memcpy(m_bitmap.getAddr32(startX, destY), startAddr, rowBytes);
 136         }

also, the indent is off.

Looks sane to me.  r=me with more ASSERTs.
Comment 30 Peter Kasting 2009-06-10 09:59:19 PDT
(In reply to comment #29)
> (From update of attachment 31123 [details] [review])
> Shouldn't we add some ASSERTS?

I can ASSERT in the Skia code, but I'm not sure I'll be able to in the Cairo code, because unlike Skia Cairo doesn't know dimensions of its own bitmap.  I'll have to add a width to make these APIs work but I wasn't necessarily going to add a height...

In any case, I can make a best-effort attempt.
Comment 31 Peter Kasting 2009-06-10 10:47:39 PDT
Comment on attachment 31123 [details]
part five (Landed r44573)

Landed, clearing flags
Comment 32 Peter Kasting 2009-06-10 17:10:16 PDT
Created attachment 31146 [details]
part six (Landed r44597)

I need to land this for the skia/ImageDecoder.h non-MSVC compile fix; the rest of the changes are less-important stuff that's just sitting in my tree at the moment and so is kind of tagging along.
Comment 33 Eric Seidel (no email) 2009-06-10 17:18:59 PDT
Comment on attachment 31146 [details]
part six (Landed r44597)

looks fine.
Comment 34 Peter Kasting 2009-06-10 17:31:55 PDT
Comment on attachment 31146 [details]
part six (Landed r44597)

Landed, clearing flags.
Comment 35 Peter Kasting 2009-06-11 17:38:44 PDT
Created attachment 31183 [details]
part seven

This patch makes the Cairo and Skia ImageDecoder.h implement the same public APIs, so the decoders can be the same files.

The _will_ break Qt/GTK+/anybody else who is pulling in ImageDecoder.h and isn't Cairo.  I will abstract the Cairo- (and Skia-) specific bits back out in the next patch or two, although these ports might still need architectural changes.
Comment 36 Peter Kasting 2009-06-11 17:59:17 PDT
Created attachment 31185 [details]
part seven, v2

Try to fix Cairo build error
Comment 37 Peter Kasting 2009-06-11 18:04:52 PDT
Created attachment 31186 [details]
part seven, v3 (Landed r44631)

Sigh
Comment 38 Brent Fulgham 2009-06-11 20:58:15 PDT
I've applied and confirmed that the "part seven, v3" patch builds and works perfectly on Windows Cairo.
Comment 39 Eric Seidel (no email) 2009-06-12 14:37:09 PDT
Comment on attachment 31186 [details]
part seven, v3 (Landed r44631)

LGTM.  Thanks again for taking this on.
Comment 40 Peter Kasting 2009-06-12 15:44:04 PDT
Created attachment 31217 [details]
part eight

This looks huge but it's almost all just deleting the now-unnecessary copies of the image decoders in skia/ or moving the non-boilerplate ones from in there to replace the boilerplate ones in bmp/, ico/, and xbm/.  So it should basically be a rubber-stamp.
Comment 41 Eric Seidel (no email) 2009-06-12 15:52:00 PDT
Comment on attachment 31217 [details]
part eight

rs=me
Comment 42 Holger Freyther 2009-06-12 21:19:35 PDT
I'm so not impressed by these changes. The amount of fall outs so far has been so big, I don't want to continue wasting my time dealing with these fallouts. I don't see the reason the Skia decoders were landed in the first place and I don't like the idea of putting cairo stuff into ImageDecoder.h at all. Could we please just revert all of these changes?

Could you please explain me why ImageDecoder now needs to depend on a specific graphics library? this was not the case before and things worked just fine, why does this need to be changed? Why can't skia just work like cairo?

Before merging any more of this stuff, could we please take the step back and see where we are heading to?

I'm also specially frustrated to see changes in the code as this makes any attempt to remerge code with mozilla more difficult...

*currently dealing with fallouts*
Comment 43 Brent Fulgham 2009-06-12 21:45:26 PDT
The Cairo build of Windows WebKit works properly, and correctly displays BMP, ICO, and XBM now.
Comment 44 Brent Fulgham 2009-06-12 23:21:19 PDT
Just for the record, I would oppose backing these updates out.  This is a nice improvement for the Cairo-based ports of WebKit.  I think that a small change, such as making the cairo-specific calls in ImageDecoder.h generic signatures (similar to GraphicsContext, and pushing the specific implementation details into specialized sub-files (just as is done with the various platform files now) would be a more desirable approach.
Comment 45 Holger Freyther 2009-06-13 06:22:57 PDT
(In reply to comment #44)
> Just for the record, I would oppose backing these updates out.  This is a nice
> improvement for the Cairo-based ports of WebKit.  I think that a small change,
> such as making the cairo-specific calls in ImageDecoder.h generic signatures
> (similar to GraphicsContext, and pushing the specific implementation details
> into specialized sub-files (just as is done with the various platform files
> now) would be a more desirable approach.

To get the record straight. In WebCore/platform/image-decoders is nothing that is specific to any graphics library. So calling it "cairo decoders" is misleading. dhyatt confirmed on irc that the decoders should simply operate on buffers. I strongly oppose making decoders cairo/skia depended. In fact the decoders are used by the WinCE port of torchmobile (IIRC), likely to be used by WX, there is experimental support for QtWebKit to use the decoders again.

On top of that ImageDecoder.h is an interface used by the Qt port to implement image decoding using Qt routines, so any change to that file requires adjustment to Qt (2nd part of the misconception to call that cairo related).

It is really not friendly to force so much breakage on others for three new decoders...
Comment 46 Holger Freyther 2009-06-13 06:26:24 PDT
(In reply to comment #35)
> Created an attachment (id=31183) [review]
> part seven
> 
> This patch makes the Cairo and Skia ImageDecoder.h implement the same public
> APIs, so the decoders can be the same files.
> 
> The _will_ break Qt/GTK+/anybody else who is pulling in ImageDecoder.h and
> isn't Cairo.  I will abstract the Cairo- (and Skia-) specific bits back out in
> the next patch or two, although these ports might still need architectural
> changes.

nice attitude, I'm totally disappointed by this. :(

Comment 47 Darin Adler 2009-06-13 06:51:32 PDT
I don't know why people started calling the cross-platform image decoders "the Cairo ones".
Comment 48 Holger Freyther 2009-06-13 06:54:17 PDT
Created attachment 31234 [details]
Free ImageDecoder.h from any knowledge about graphics libraries

Remove cairo dependency from ImageDecoder.h. The great thing about the image-decoders is that they work on buffers (not by accident) and this way any graphics library can adopt the RGBA8888 surface making them generally usable... there should not be any knowledge about graphics libraries in this code...
Comment 49 Darin Adler 2009-06-13 07:16:25 PDT
Comment on attachment 31234 [details]
Free ImageDecoder.h from any knowledge about graphics libraries

> +    return cairo_image_surface_create_for_data((unsigned char*)buffer->bytes().data(),
> +                                               CAIRO_FORMAT_ARGB32,
> +                                               size().width(),
> +                                               size().height(),
> +                                               size().width()*4);

Any reason to use a C-style cast here instead of the C++-style casts in the old code? Also, why the hardcoded "4" instead of sizeof(PixelData)?

r=me
Comment 50 Holger Freyther 2009-06-13 08:05:18 PDT
Created attachment 31237 [details]
Free ImageDecoder.h from any knowledge about graphics libraries (2nd attempt)

Sorry darin to have wasted your time. This patch adds the sizeof(PixelData) instead of the magic number, changes the c cast to c++ reinterpret_cast and adds one of the two bytes() symbols back to ImageDecoder.h. This should fix the hidden Wx build bustage as well.
Comment 51 Darin Adler 2009-06-13 08:07:00 PDT
Comment on attachment 31237 [details]
Free ImageDecoder.h from any knowledge about graphics libraries (2nd attempt)

The original patch had a const_cast as well as the reinterpret_cast. Will this compile without the const_cast?

r=me
Comment 52 Holger Freyther 2009-06-13 08:12:52 PDT
(In reply to comment #51)
> (From update of attachment 31237 [details] [review])
> The original patch had a const_cast as well as the reinterpret_cast. Will this
> compile without the const_cast?

it did here (gcc4.4). Originally ImageDecoder.h had

const Vector& bytes() const { return m_bytes }
Vector &bytes() { return m_bytes; }

which might explain the const cast. I have only added back the non-const symbol as it felt strange to use a const version and then cast it away. What do you think?

Comment 53 Peter Kasting 2009-06-13 09:22:05 PDT
Non-Cairo folks:

I'm well aware that other ports besides Cairo use ImageDecoder.h.  I don't intend to have it mandatorily depend on Cairo, Skia, or anything else; the current state is a brief interim step before I pull all the Cairo bits back out.  Please, don't land the "Free ImageDecoder.h from knowledge about graphics libraries" patch; I intend to free that header from this knowledge in a much cleaner way than reverting the patch.  My plan was to land that change Monday.  If you need, I can come in to work and get it written up today.

Also, I didn't just cavalierly break the other ports; I discussed plans with people on IRC and got OKs to go the route I'm going, so this isn't happening in a vacuum.
Comment 54 Holger Freyther 2009-06-13 09:34:14 PDT
(In reply to comment #53)
> Non-Cairo folks:
> 
> I'm well aware that other ports besides Cairo use ImageDecoder.h.  I don't
> intend to have it mandatorily depend on Cairo, Skia, or anything else; the
> current state is a brief interim step before I pull all the Cairo bits back
> out.  Please, don't land the "Free ImageDecoder.h from knowledge about graphics
> libraries" patch; I intend to free that header from this knowledge in a much
> cleaner way than reverting the patch.  My plan was to land that change Monday. 
> If you need, I can come in to work and get it written up today.

You really can't break things on Friday and wait until Monday to fix it, specially with contributors not on a pay roll. Why did you hardcode cairo knowledge in the first place, how did this make anything better? You willingly broke WX and Qt with that and this is what honestly annoys me.
Comment 55 Peter Kasting 2009-06-13 09:42:32 PDT
(In reply to comment #54)
> You really can't break things on Friday and wait until Monday to fix it,
> specially with contributors not on a pay roll. Why did you hardcode cairo
> knowledge in the first place, how did this make anything better? You willingly
> broke WX and Qt with that and this is what honestly annoys me.

First let me outline where things are going so I have context to answer:

* Pull all implementation code that isn't cross-platform out of ImageDecoder.h into ImageDecoder.cpp
* Create cairo/ subdirectory and move Cairo version of ImageDecoder.cpp there; fix Cairo build to include this file
* Create Skia version of ImageDecoder.cpp in skia/ and remove the ImageDecoder.h there

At this point ImageDecoder.h is a completely cross-platform header with no knowledge of graphics libraries.  In addition, it has no knowledge of precisely how a particular backend implements the backing store inside RGBA32Buffer, so ports can use a direct pixel store or some other store managed by their graphics library (Skia already does this and after talking with Brent I think Cairo will move this direction).  Plus it's more readable.

The first patch to be landed will do the first two steps above and hopefully make ImageDecoderQt.h compile as-is.  The next patch will do the third item and only be noticed by the Skia folks.  I can come in right now and right the first patch so it will unbreak you.  Sorry for the trouble.

Now, as to answering your question:  Cairo knowledge was coded into ImageDecoder.h for two reasons:
* So I could get to a point where Skia and Cairo were both building using an ImageDecoder.h with the same interface, to ensure the interface contained everything necessary before pulling the actual implementation out of it
* So the two different ImageDecoder.hs would be as close as possible for easier diffing

In other words, this was to make my life easier, not because it's the only way things had to happen.  The reason I did that was because I asked on IRC if it was OK to break Qt while I did that and got an OK; I normally wouldn't commit changes that knowingly break other ports but I was very worried about goofing up the contents of the ImageDecoder headers since they had been tricky to diff against each other.

Again, I'm sorry you feel frustrated and that I'm not paying attention.  I will do what I can to fix things immediately.  If you have review powers it should be pretty easy for me to get you a patch you can r+.
Comment 56 Holger Freyther 2009-06-13 09:54:01 PDT
It is night where I am and I'm quite tired but as of now I don't think it is the right thing to do, specially as we have been there with the Qt port and opted for other things.

You silently turn the ImageDecoder.h into a ImageSource. The question is if that is the way to go or not. So after you are done, where do you think is the border between ImageSource and ImageDecoder?
Comment 57 Peter Kasting 2009-06-13 10:47:35 PDT
(In reply to comment #56)
> You silently turn the ImageDecoder.h into a ImageSource. The question is if
> that is the way to go or not. So after you are done, where do you think is the
> border between ImageSource and ImageDecoder?

To sum up: some discussion happened on IRC.  At the moment I'm not changing anything about ImageDecoder's API.  After stuff is unforked it probably makes sense to look closely at the general interplay between the various image classes to see if several ports' implementations should merge together, if some of these classes should be killed, etc.  At the moment I'm going to go ahead and get things unforked first, that seems like a necessary prerequisite to figuring out the best cross-platform design.
Comment 58 Peter Kasting 2009-06-13 11:22:48 PDT
Created attachment 31241 [details]
part nine (Landed r44652)

This removes Cairo-specific implementation from ImageDecoder.h.  This header should again be safe to include by any port.

Nest step will be to merge the Skia ImageDecoder.h into this file, which should barely change anything.
Comment 59 Eric Seidel (no email) 2009-06-13 11:29:15 PDT
Comment on attachment 31241 [details]
part nine (Landed r44652)

Looks sane to me.  Peter promised me this would not start any flame wars or break any builds.  He also said zecke seemed to give the go-ahead over irc (I was not there).
Comment 60 Peter Kasting 2009-06-13 13:34:01 PDT
(In reply to comment #47)
> I don't know why people started calling the cross-platform image decoders "the
> Cairo ones".

Because at this point they are, for all practical purposes.  Safari and Chromium/Mac use CG, Chromium/Win and Linux use the Skia decoders, QT uses its own decoders, GTK and Cairo/Win use the "cross-platform" decoders, and Wx doesn't build (and hasn't in a long time).

So no one except Cairo was actually using the various gif/, jpeg/, etc. decoders.  Now Cairo and, after part eight landed, Skia, are both using them, and in the future perhaps Qt will too (my aim is to make that easier rather than harder).  Supposedly there are experimental patches lying around (somewhere) to accomplish this last bit.

Qt currently _does_ use ImageDecoder.h, but they're not really using the cross-platform decoders, just one of the headers; I'm not completely sure what ImageDecoder.h is really used for since some of it is stubbed out and much of the rest isn't called.
Comment 61 Peter Kasting 2009-06-13 16:06:19 PDT
Created attachment 31245 [details]
part ten (Landed r44661)

Here is the last part on this bug.  There is more work that can probably be done on these files (cleaning up interfaces to use more WebKit types, determining whether all platforms using these decoders can now use a shared ImageSource, etc.) but this will finish the "unforking" portion.
Comment 62 Eric Seidel (no email) 2009-06-13 16:51:44 PDT
Comment on attachment 31245 [details]
part ten (Landed r44661)

Looks good.  So many minus lines!

It's hard to see how RGBA32Buffer is different from ImageBuffer after all these changes?  I'm starting to wonder what the design goal of RGBA32Buffer is?

Style:
 64 bool RGBA32Buffer::setSize(int new_width, int new_height)

Style:
 97 void RGBA32Buffer::setStatus(FrameStatus s)

s should really be an english word, like "status"

LGTM.
Comment 63 Peter Kasting 2009-06-13 18:09:14 PDT
Fixed style nits and committed in r44661.

I don't know whether perhaps ImageBuffer and RGBA32Buffer could merge into one object.  I feel like now that things are unforked it's finally possible to consider broader architectural questions like "how should image decoding actually work".  I have filed bug 26379 for that; interested parties that aren't already CCed there can feel free.
Comment 64 Holger Freyther 2009-06-13 18:29:12 PDT
What to think about now:
     - When RGBA32Buffer is more than a buffer we should rename it
     - Where is the the conceptual difference between RGBA32Buffer and ImageBuffer?
     - All WX, Cairo, Skia ImageSource*.cpp can be deleted now
     - Why should a class called RGBA32Buffer know how to create a native image? Seems to be broken.
     - There are still #ifdef's left in ImageDecoder.h. There were not there before, all image-decoders where true cross-platform, I want than back.

Comments on the process:
     - I agreed on irc to continue as we are in between. But honestly do we want to continue with breaking existing concepts/implementations and get not well thought out things just to merge Skia/Chromium and maybe one day get back to a proper design?
Comment 65 Holger Freyther 2009-06-13 18:57:24 PDT
getAddr will not return anything for other ports (if called) and will silently fail which is bad...
Comment 66 Holger Freyther 2009-06-13 18:59:42 PDT
Created attachment 31247 [details]
Do not silently cause crashes...

Not the final solution but go from something that silently will fail to an assert and a default return...
Comment 67 Peter Kasting 2009-06-13 19:19:49 PDT
(In reply to comment #66)
> Created an attachment (id=31247) [review]
> Do not silently cause crashes...

Does this cause any crashes?  I didn't think Qt used this function or any of the functions that call this.

FWIW, this change is fine with me.  But I would sincerely like to know if it actually makes a difference.
Comment 68 Peter Kasting 2009-06-13 19:20:36 PDT
(In reply to comment #64)
>      - When RGBA32Buffer is more than a buffer we should rename it

Right now I don't think it is more than a buffer, or at least the interface to a buffer.  (But I think we should rename it FrameBuffer anyway, as that would be clearer.)

>      - Where is the the conceptual difference between RGBA32Buffer and
> ImageBuffer?

This is one of the questions posed on bug 26379.  Patches to modify how things should work welcome there.  I don't understand ImageBuffer as well as I understand RGBA32Buffer so I don't think I have a good answer for you right now.  Perhaps there is none and they should be merged.

>      - All WX, Cairo, Skia ImageSource*.cpp can be deleted now

Again, the merging of ImageSources is mentioned as a possibility on bug 26379.  I am not sure it's a slam dunk but probably we could go a long ways pretty easily.  It would definitely be nice to eliminate more code.

>      - Why should a class called RGBA32Buffer know how to create a native
> image? Seems to be broken.

Platforms need a way of getting at the platform-specific storage inside the buffer.  Because of the way that the entire image decoding/caching framework is written, platforms generally need to take ownership of some kind of pointer to the data the point where they read it out.  For efficiency, it is best to avoid copying during this process if possible (which is why both CG and Skia use refcounting objects for this).  Taken together, these constraints mean it's convenient and sensible to say that RGBA32Buffer, which may contain platform-specific storage, can return that storage as a NativeImagePtr, a type that platforms understand.  This doesn't bring knowledge of the specific storage types or copying mechanisms into the header.

If I understand your underlying concerns right, you'd prefer that _all_ ports using this header store their data as a Vector<unsigned>.  But that imposes dramatic, and completely unnecessary, memory and speed costs on ports without gaining anything in return.  The implementation in the Cairo port that currently does this is, I think, inefficient compared to using a Cairo surface directly, and I've discussed this briefly with Brent; this bug has made solving that inefficiency dramatically simpler as it modifies the ImageDecoders to be careful about how they ask that the data be stored and copied.

If I am missing a benefit here, I'd like to know.  Even code readability does not seem hindered by this change -- I think anyone can agree that the current, shorter, more commented header, with functionality broken into small blocks that share cross-platform implementations where possible, is more readable than the original code, especially when the logic in the image decoders themselves is taken into account (compare the inner loop of the GIF decoder before and after this set of changes).

>      - There are still #ifdef's left in ImageDecoder.h. There were not there
> before, all image-decoders where true cross-platform, I want than back.

True, but I don't see why this is inherently problematic.  There are platform-specific #ifdefs all over the entire WebKit source tree in "cross platform" code where specific platforms have slightly different needs.  In this case the ifdefs could be reduced if we're willing to take an efficiency hit or are not trying to hoist more logic to ImageDecoder.h _to make life easier on implementers_.  However, generally WebKit frowns on things like virtual functions and inheritance, especially in speed-critical code, which this most certainly is.  I don't see good alternatives, but if you have some I'm certainly happy to consider them.  This certainly seems better (to me) than simply dropping the parallel implementations of RGBA32Buffer and the image decoders into one source file and #ifdefing the entire thing, or having completely separate files that were nearly entirely copies of each other.

Honestly, I am a bit confused what your goal with this complaint is.  AFIAK the Qt port doesn't use the cross-platform decoders (yet) and also doesn't use most of ImageDecoder.h.  How is this design hurting Qt?  And if it is hurting something, can you propose a superior alternative, that doesn't sacrifice efficiency, readability, or compatibility with both Cairo and Skia?  Constructive criticism is useful, but without more details this complaint does not seem constructive.  And if the problem is not Qt but some sort of general principle, then perhaps the maintainers of the GTK, Cairo/Win, and Skia backends can have some input into that principle as well?

> Comments on the process:
>      - I agreed on irc to continue as we are in between. But honestly do we
> want to continue with breaking existing concepts/implementations and get not
> well thought out things just to merge Skia/Chromium and maybe one day get back
> to a proper design?

I think this is begging the question.

Asserting that this is "not well thought out" and "not a proper design" ignores the fact that I spent quite a bit of my time discussing and designing things with various people before and during the progress on this bug.  From the beginning my motivations were to help, not hurt, other ports and to be a good WebKit citizen -- certainly there was no immediate, direct benefit to the Skia backend in this unforking.  Along the way I watched trees, checked in bustage fixes, and helped out when things went wrong.  The patches to fix Qt were extremely small (and even smaller in some cases after I consulted with people on IRC about the right ways to do them) and while I'm certainly not perfect and did not always get things fixed instantly, until this morning no one had expressed any complaints about process.  Before you expressed your frustrations here you had not emailed me or contacted me on IRC expressing any of these concerns privately so I could help with them.  Nor did you contact my reviewer, or the Cairo port maintainer I was apparently working with.  I can understand that it's very frustrating when someone else breaks you, especially when you have limited time to work on WebKit -- look in my own early commit history for the number of times I tried to fix other peoples' port bustage.  But please give me some credit in that I have tried to do my best at all times.  Certainly I was not aware that having things broken last night/this morning would significantly hinder anyone else's contributions, or I absolutely would not have landed things the way I did -- but I did make a good-faith effort to check on this before I landed it.

In summary, I am very receptive to concrete designs that take into account different platforms' needs.  And I am downright eager to help the Qt port use the cross-platform decoders if you wish; I think the changes made in this bug make that significantly more feasible.  I think the most positive thing at this point would be for you to post patches on bug 26379 that move the design the direction you want to go, so the rest of the affected parties here can have precisely the sort of well-thought-out discussion process you want.  I hope that helps.
Comment 69 Peter Kasting 2009-06-13 20:14:31 PDT
Created attachment 31250 [details]
part eleven

Try to get wx working.
Comment 70 Peter Kasting 2009-06-13 23:21:44 PDT
Created attachment 31252 [details]
part eleven v2

Fixes some problems with the wx blind fix based on actual testing on wx.  Also fixes a silly Cairo build bustage introduced in part ten.
Comment 71 Peter Kasting 2009-06-13 23:30:23 PDT
Created attachment 31253 [details]
part eleven v3 (Landed r44669)

I neglected to update webcore-wx.bkl.
Comment 72 Peter Kasting 2009-06-13 23:46:10 PDT
As far as I know everyone builds now, the wx and Cairo ports both have the option of moving to a more efficient storage mechanism, and everything is unforked.  And I included the patch to ASSERT if you run getAddr() on an unsupported platform.  So I think this can stay closed this time?
Comment 73 Holger Freyther 2009-06-16 02:52:54 PDT
(In reply to comment #38)
> I've applied and confirmed that the "part seven, v3" patch builds and works
> perfectly on Windows Cairo.

Animated gif support is broken due this patch (according to git bisect and my manual testing). The problem seems to be at the end of one animation cycle, showing an empty (sometimes black) image...


Comment 74 Peter Kasting 2009-06-16 09:42:14 PDT
(In reply to comment #73)
> Animated gif support is broken due this patch (according to git bisect and my
> manual testing). The problem seems to be at the end of one animation cycle,
> showing an empty (sometimes black) image...

Can you go ahead and file a new regression bug against me on this?  I will investigate today.

If you can provide the specific revision (from your git bisect testing) and tell me which platform(s) you know are affected, and provide the test image that fails, that'll help a lot.
Comment 75 Peter Kasting 2009-06-16 10:33:54 PDT
I filed bug 26447 for comment 73.  More details there would be great.
Comment 76 Yong Li 2009-08-14 08:52:48 PDT
>RGBA32Buffer::m_height and all associated functions have
>been removed (set but never used) and some places now rely on superclass
>implementations.

RGBA32Buffer::m_height is very useful for JPEG images. When image decoding isn't finished, browser can display the partial image without showing black.

You probably do have this problem because ImageSource::frameHasAlphaAtIndex() is hacked and lies.

But RGBA32Buffer::m_height is a better solution, which tells how many scanlines are available, and this is used in our WINCE port. If hacking hasAlpha() is a must, it should be done in particular decoders, rather than in ImageSource.
Comment 77 Yong Li 2009-08-14 08:53:27 PDT
(In reply to comment #76)
> >RGBA32Buffer::m_height and all associated functions have
> >been removed (set but never used) and some places now rely on superclass
> >implementations.
> 
> RGBA32Buffer::m_height is very useful for JPEG images. When image decoding
> isn't finished, browser can display the partial image without showing black.
> 
> You probably do have this problem because ImageSource::frameHasAlphaAtIndex()
> is hacked and lies.

oops, should "do NOT have this problem", here

> 
> But RGBA32Buffer::m_height is a better solution, which tells how many scanlines
> are available, and this is used in our WINCE port. If hacking hasAlpha() is a
> must, it should be done in particular decoders, rather than in ImageSource.