Make image decoders faster by refactoring hot loops that fills the lines of ImageFrame.

The original idea came from the work of Allan Jensen landed in WebKit as r150252. He eliminated the branches in PNGImageDecoder::rowAvailable to make it benefit from auto vectorization. I was refactoring that to a more general form by adding helpers into ImageDecoder.h. Also I was changing the loops so that this change is a performance win without auto vectorization as well (which we do not use by default,
only with -O3 or -ftree-vectorize). I measured the performance gain with callgrind by comparing the inclusive cost of the affected function. I used simple local pages with a few image for benchmarking. These are the performance gain for differet image formats:
  * opaque PNG: 7.8% faster
  * PNG with alpha: 24% faster
  * JPG (without jpeg-turbo or with non-RGB/BGR format, like CMYK): 24% faster

[jochen (gone - plz use gerrit), 2013-05-24 06:48:52]

lgtm

[Peter Kasting, 2013-05-24 19:03:12]

When you post a patch for review, please use the publish+mail button to send a
message asking for review, or many people won't ever know you want review.

When you specify multiple reviewers, always say specifically what you want each
to review.

[Peter Kasting, 2013-05-24 22:39:01]

The idea of this patch is fine, but the execution isn't.

First, it's not correct to inline a bunch of setters into PNGImageDecoder.cpp
that duplicate functionality in ImageDecoder.h.  Instead we should update the
setters in ImageDecoder.h to be able to serve the needs of code like this. 
Basically, I support breaking setRGBA() into separate functions that can do the
no/non-premultiplied/premultiplied alpha cases, perhaps optionally with a
"wrapper function" that dispatches appropriately for code that wouldn't gain any
benefit from this sort of unrolling tactic, and then updating all the decoders
to use them.  I suspect PNG isn't the only one that would improve.

Second, we already have "fast premultiplied alpha" helper functions in
ImageDecoder.h -- see fixPointUnsignedMultiply() etc.  We also have similar code
in various places in Skia.  It seems like we should consolidate things here.  I
suggest bringing in Mike Reed to advise you on the best route to take.

Third, the code in PNGImageDecoder.cpp deserves some comments about why the
structure is important, so future maintainers don't say "hey, this is
unnecessarily verbose" and refactor it for "clarity", losing the speed benefits.

Fourth, IMAGE_DECODER_DOWN_SAMPLING is going away (maybe has already gone
away?), you might want to wait for/update to that so you can simplify slightly.

[Noel Gordon, 2013-05-25 04:40:56]

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/g...
File Source/core/platform/graphics/Color.cpp (right):

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/g...
Source/core/platform/graphics/Color.cpp:439: WTF::fastDivideBy255(color.blue() *
alpha + 254),
The Color.cpp change should really be a separate patch, with caveats:

So we want (color * alpha + 254) / 255 and propose to use fastDivideBy255, which
is 24 ops per RGB pixel.  However, (color * alpha + 254) / 255 is the same as
SkMulDiv255Ceiling

  https://bugs.webkit.org/attachment.cgi?id=76488&action=prettypatch
  https://codereview.appspot.com/3466042

which is 15 ops per RGB pixel.  Next, there's the faster ceiling implementation
from crbug.com/234071 which gets it down to 10 ops per RGB pixel.  Here's a test
program you can use to test the bit exactness of the various methods.

#include <stdio.h>

int main(int argc, char** argv)
{
  for (int color = 0; color < 256; ++color) {
    for (int alpha = 0; alpha < 256; ++alpha) {
      // Webkit int math: integer divide.
      int webkit = (color * alpha + 254) / 255;

      // SkMulDiv255Ceiling: no divide, 15 ops per RGB pixel.
      int skia_ceiling = (color * alpha + 255);
      int skia = (skia_ceiling + (skia_ceiling >> 8)) >> 8;
      if (skia != webkit)
        printf("skia diff on color %d alpha %d: skia %d, webkit %d\n",
            color, alpha, skia, webkit);

      // FastDiv255Ceiling: no divide, 10 ops per RGB pixel.
      const int ceilingFraction = 257 * 255;
      int fast_ceiling = alpha * 257;
      int fast = (color * fast_ceiling + ceilingFraction) >> 16;
      if (fast != webkit)
        printf("factored diff on color %d alpha %d: skia %d, webkit %d fast
%d\n",
            color, alpha, skia, webkit, fast);
    }
  }

  return 0;
}

So if RGBA32 premultipliedARGBFromColor(const Color& color) needs to be fast
(well, does it?) you could implement the calculation here as

    unsigned a = a * 257;
    r = (r * a + ceilingFraction) >> 16;
    g = (g * a + ceilingFraction) >> 16;
    b = (b * a + ceilingFraction) >> 16;

where ceilingFraction is a constant 257 * 255.

Again, I think it would be better to deal with the Color changes on a separate
patch.

[Noel Gordon, 2013-05-25 05:58:26]

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/i...
File Source/core/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/i...
Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:421: unsigned char r
= WTF::fastDivideBy255(pixel[0] * a);
Agree with Peter that we should consolidate these setters into ImageDecoder.h. 
I also note that fixPointUnsignedMultiply can be used to create a faster
implementation compared to WTF::fastDivideBy255.

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/i...
Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:425: *dest = a << 24
| r << 16 | g << 8 | b;
A second reason to consolidate is that here, and elsewhere, this expression
assumes BGRA little-endian pixel layout.  This would break Chrome Android for
example, which uses RGBA little-endian.  The setters in ImageDecoder.h have
tools to handle platform pixel layout, SkPackARGB32NoCheck(a, r, g, b) in
particular.

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/i...
Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:426:
nonTrivialAlphaMask |= (255 - a);
If nonTrivialAlphaMask was initialized to 255 say, could we update it with

  nonTrivialAlphaMask &= a;

in the setters, and then later test if it's still 255 to see if we have trivial
alpha?  If so, seems we could save another op per pixel.

https://chromiumcodereview.appspot.com/15466004/diff/1/Source/core/platform/i...
Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:529: #if
ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
Yes, was removed at https://codereview.chromium.org/15466003

[kbalazs_, 2013-05-28 23:39:34]

Looking at crbug.com/234071 it seems like you did not get to an agreement yet.
For now I would like to chose the algorithm we have in ImageDecoder.h now. This
can be changed to a Skia function or something else later.
The main purpose of this patch is to turn the loops that fills the rows of
ImageFrame in PNGImageDecoder::rowAvailable into a simpler form without branches
so the compiler can emit SIMD code for them. The changes in Color are not needed
for now. I guess Allan just changed them as a refactoring (he moved the
fastDivBy255 function to Color.h).
The problem with ImageFrame::setRGBA is that it has a branch so it's not good to
call it from the loop. I am planning to introduce an inline function that fills
a full row. With a bit of template trick we can handle all the case but just
inline the part that is needed to the call site.

[Peter Kasting, 2013-05-29 00:01:42]

On 2013/05/28 23:39:34, kbalazs wrote:
> The main purpose of this patch is to turn the loops that fills the rows of
> ImageFrame in PNGImageDecoder::rowAvailable into a simpler form without
branches
> so the compiler can emit SIMD code for them.

Yep.

> The problem with ImageFrame::setRGBA is that it has a branch so it's not good
to
> call it from the loop.

Agreed.  This is why I said that we should go ahead and break this function into
pieces, kind of like the patch does, just in ImageDecoder.h instead, and update
the other decoders to use them.  We may or may not want a wrapper function that
effectively duplicates the old setRGBA() at that point -- it depends on what the
other decoders do.

Really the point of my review comments is: this idea is fine, we just shouldn't
implement it in a PNGImageDecoder-specific way.

> I am planning to introduce an inline function that fills
> a full row. With a bit of template trick we can handle all the case but just
> inline the part that is needed to the call site.

OK... I'm not sure I follow that and it sounds like a bigger change, if it
applies to all the decoders.

[kbalazs_, 2013-05-29 11:18:12]

> 
> OK... I'm not sure I follow that and it sounds like a bigger change, if it
> applies to all the decoders.

Forget about the template thing, it was a silly idea. Btw, can we just start
with the png decoder and change the others later?

[Peter Kasting, 2013-05-29 18:21:11]

On 2013/05/29 11:18:12, kbalazs wrote:
> Forget about the template thing, it was a silly idea. Btw, can we just start
> with the png decoder and change the others later?

Only if we do this by the mechanism I proposed.  Split setRGBA() into parts,
call the parts, and retain a wrapper function around them for other decoders.

But it's going to be the work of like 20 minutes to incorporate the other
decoders.  I think we should just do it now.

[kbalazs_, 2013-05-30 18:50:30]

I have to redefine the purpose of this change. Auto vectorization is only
available if we build in -O3 or with -ftree-vectorize. It is not enabled by
default (not in WebKit as well). 

After doing some benchmark it turned out that removing the fast paths from
setRGBA when alpha is 0 or 255 cause a significant regression. However, rolling
out the loops in simpler form to avoid the branches is still a significant win.
I will do that.

About the other decoders I was not very successful. Gif is using a color palette
and BMP is using a non-constant bitmask table so both do more work for each
pixel. JPEG has a setRGBA path but it is not used too often because
libjpeg-turbo can swizzle the pixels for us with the common formats (so it's
already fast with rgba and bgra). Fortunately I could test my code with cmyk
coded images. WebP does not need swizzling again. It has a setRGBA path but only
if QCMS_WEBP_COLOR_CORRECTION is 1 which is not the case by default (at least
here). However it's the same change - even simpler - than in PNGImageDecoder so
I did that.

[kbalazs_, 2013-05-31 09:55:52]

Hm, I did not even notice I already got an lgtml on this. Can I ask you to
review it again, since it has changed a lot. Or maybe I should create a new
record?

[Peter Kasting, 2013-06-01 01:22:12]

Cool, this is more what I was thinking.  Some comments below on minor issues.

I know you mentioned why GIF/BMP aren't as amenable to this sort of change.  I
don't care as much about BMP since it's rarer (although maybe we could still get
some wins there), but I'm wondering if we can get any speedup at all on GIF,
even though it has a colormap.

Staring at GIFImageDecoder::haveDecodedRow(), we've already effectively pulled
the GetAddr() call out of the loop, but the rowBuffer[...] calculation could be
simplified to a straight dereference (and increment when the loop ticks over),
the frameContext->isTransparent and writeTransparentPixels checks could be
template args, and the setRGBA calls could be a straight SkPackARGB32NoCheck()
call (where the alpha value is 255) and a *dest = 0 (where it's 0).  I wonder if
those changes buy us any speed.  Do you have the ability to measure that?

https://codereview.chromium.org/15466004/diff/14001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/15466004/diff/14001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:181: inline void
fillRowFromRGBSource(int rowIndex, const unsigned char* src)
I don't think we gain much from having these first 4 wrapper functions and then
making the more complex ones private.  The wrappers basically just add a
getAddr() call, and getAddr() is (and will remain) public already, so I'd say
nuke these four and publicize the new functions below, then call those directly.

https://codereview.chromium.org/15466004/diff/14001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:214: inline static void
updateAlphaMask(unsigned char alpha, unsigned char& mask)
Nit: Honestly, I'm not convinced pulling this into its own function really adds
much.  I'd just inline this.

https://codereview.chromium.org/15466004/diff/14001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:260: ALWAYS_INLINE void
fillRowFromRGBASource(PixelData* dst, const unsigned char* src, int numPixels,
bool& hasNonTrivialAlpha)
Nit: If you take a bool* as the last arg, we can pass NULL instead of declaring
a |dummy| above.

https://codereview.chromium.org/15466004/diff/14001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:296: dst[i] =
SkPackARGB32NoCheck(255, c, m, y);
It strikes me that this function is effectively identical to
fillRowFromRGBASourcePremultiply except for:

* That function adds an updateAlphaMask() call; callers of this could pass false
for that template arg
* That function adds fastpaths for 0/255; seems like those might be a win here
if they're a win there?  (Not sure)
* This function passes 255 to the first arg of SkPackARGB32NoCheck()

If the last bullet becomes the only difference, we could basically combine these
two into a single function which takes an is_cmyk template arg and passes
"is_cmyk ? 255 : a" as the first arg to SkPackARGB32NoCheck().

I am always a fan of condensing code when it can be done without compromising
performance.

[Noel Gordon, 2013-06-03 15:08:23]

The current change appears to break webp decoding of alpha images with color
profiles.  It's also more code than I expected.  I was thinking of a simpler
change.  If we added the following ImageFrame routine

    inline void setRGBARaw(PixelData* dest, unsigned r, unsigned g, unsigned b,
unsigned a)
    {
        *dest = SkPackARGB32NoCheck(a, r, g, b);
    }

we could write the jpeg row decoder in terms of that (a two-line change) and
also not need to move details about jpeg inverted cmyk handling into ImageFrame.

Similarly, why move details about nonTrivalAlpha into ImageFrame if the png
decoder is the only decoder that cares about that currently?  If we had the
following ImageFrame routine

    inline void setRGBAPremultiply(PixelData* dest, unsigned r, unsigned g,
unsigned b, unsigned a)
    {
        if (a < 255) {
            unsigned alphaMult = a * fixPointMult;
            r = fixPointUnsignedMultiply(r, alphaMult);
            g = fixPointUnsignedMultiply(g, alphaMult);
            b = fixPointUnsignedMultiply(b, alphaMult);
        }

        *dest = SkPackARGB32NoCheck(a, r, g, b);
    }

we could write the png row decoder in terms of that ...

    ImageFrame::PixelData* address = buffer.getAddr(0, y);
    unsigned maskedAlpha = 255;
    int width = size().width();

    png_bytep pixel = row;
    if (!hasAlpha) {
        ASSERT(colorChannels == 3);
        for (int x = 0; x < width; ++x, pixel += 3)
            buffer.setRGBARaw(address++, pixel[0], pixel[1], pixel[2], 255);
    } else if (!buffer.premultiplyAlpha()) {
        ASSERT(colorChannels == 4);
        for (int x = 0; x < width; ++x, pixel += 4) {
            buffer.setRGBARaw(address++, pixel[0], pixel[1], pixel[2],
pixel[3]);
            maskedAlpha &= pixel[3];
        }
    } else {
        ASSERT(colorChannels == 4);
        for (int x = 0; x < width; ++x, pixel += 4) {
            buffer.setRGBAPremultiply(address++, pixel[0], pixel[1], pixel[2],
pixel[3]);
            maskedAlpha &= pixel[3];
        }
    }

    if (maskedAlpha != 255 && !buffer.hasAlpha())
        buffer.setHasAlpha(true);

and achieve the effect we want.  Thoughts?

[kbalazs_, 2013-06-05 11:04:35]

On 2013/06/01 01:22:12, Peter Kasting wrote:
> Cool, this is more what I was thinking.  Some comments below on minor issues.
> 
> I know you mentioned why GIF/BMP aren't as amenable to this sort of change.  I
> don't care as much about BMP since it's rarer (although maybe we could still
get
> some wins there), but I'm wondering if we can get any speedup at all on GIF,
> even though it has a colormap.
> 
> Staring at GIFImageDecoder::haveDecodedRow(), we've already effectively pulled
> the GetAddr() call out of the loop, but the rowBuffer[...] calculation could
be
> simplified to a straight dereference (and increment when the loop ticks over),
> the frameContext->isTransparent and writeTransparentPixels checks could be
> template args, and the setRGBA calls could be a straight SkPackARGB32NoCheck()
> call (where the alpha value is 255) and a *dest = 0 (where it's 0).  I wonder
if
> those changes buy us any speed.  Do you have the ability to measure that?

Yes but I would really like to postpone this to a next patch.

[kbalazs_, 2013-06-05 11:05:56]

You are right Noel, the patch has been grown too huge. I am going to refactor it
according to your suggestions.

[kbalazs_, 2013-06-05 11:24:46]

On 2013/06/03 15:08:23, Noel Gordon (Google) wrote:
> The current change appears to break webp decoding of alpha images with color
> profiles.  It's also more code than I expected.  I was thinking of a simpler
> change.  If we added the following ImageFrame routine
> 
>     inline void setRGBARaw(PixelData* dest, unsigned r, unsigned g, unsigned
b,
> unsigned a)
>     {
>         *dest = SkPackARGB32NoCheck(a, r, g, b);
>     }
> 
> we could write the jpeg row decoder in terms of that (a two-line change) and
> also not need to move details about jpeg inverted cmyk handling into
ImageFrame.
 
But than we cannot use the fast fixed point multiply for jpeg. (For jpeg we need
to do the same multiplication and set alpha to opaque.)
If we don't want to leak the the multiply logic from ImageDecoder we still need
a helper for cmyk.

[kbalazs_, 2013-06-05 11:29:26]

On 2013/06/03 15:08:23, Noel Gordon (Google) wrote:
> The current change appears to break webp decoding of alpha images with color
> profiles.  

I don't see what is the problem with that part. Could you elaborate on that?

[kbalazs_, 2013-06-05 12:28:29]

On 2013/06/05 11:29:26, kbalazs wrote:
> On 2013/06/03 15:08:23, Noel Gordon (Google) wrote:
> > The current change appears to break webp decoding of alpha images with color
> > profiles.  
> 
> I don't see what is the problem with that part. Could you elaborate on that?

Nevermind, I see, I should take care of premultiply.

[Noel Gordon, 2013-06-05 13:57:40]

On 2013/06/05 12:28:29, kbalazs wrote:
 
> Nevermind, I see, I should take care of premultiply.

Yes, and there are tests for it.  Since urvang@ is trying to land animated webp
on https://codereview.chromium.org/13980003 perhaps we should wait until he's
done?

[Noel Gordon, 2013-06-05 14:20:15]

On 2013/06/05 11:24:46, kbalazs wrote:
> On 2013/06/03 15:08:23, Noel Gordon (Google) wrote:
< 
> > we could write the jpeg row decoder in terms of that (a two-line change) and
> > also not need to move details about jpeg inverted cmyk handling into
ImageFrame.
>  
> But than we cannot use the fast fixed point multiply for jpeg. (For jpeg we
need
> to do the same multiplication and set alpha to opaque.)

We don't need to do a the fast fixed point multiply for jpeg since alpha=255 for
all jpeg pixels.
In that case (alpha=255) buffer.setRGBA() is the same as *dest =
SkPackARGB32NoCheck(255, r, g, b), or buffer.setRGBARaw(r, g, b, 255) in other
words.  So in JPEGImageDecoder::setPixel() change

https://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decode...

- buffer.setRGBA(pixel, jsample[0], jsample[1], jsample[2], 0xFF);
+ buffer.setRGBARaw(pixel, jsample[0], jsample[1], jsample[2], 0xFF);

and change
https://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decode...

- buffer.setRGBA(pixel, jsample[0] * k / 255, jsample[1] * k / 255, jsample[2] *
k / 255, 0xFF);
+ buffer.setRGBARaw(pixel, jsample[0] * k / 255, jsample[1] * k / 255,
jsample[2] * k / 255, 0xFF);

and the jpeg tests should still pass.

[Noel Gordon, 2013-06-05 14:28:19]

Another way to think about it: for jpeg images, the premultiplied pixels are
equal to the unpremultiplied pixels because alpha is 255 everywhere.

[kbalazs_, 2013-06-05 21:20:33]

On 2013/06/05 14:20:15, Noel Gordon (Google) wrote:
> On 2013/06/05 11:24:46, kbalazs wrote:
> > On 2013/06/03 15:08:23, Noel Gordon (Google) wrote:
> < 
> > > we could write the jpeg row decoder in terms of that (a two-line change)
and
> > > also not need to move details about jpeg inverted cmyk handling into
> ImageFrame.
> >  
> > But than we cannot use the fast fixed point multiply for jpeg. (For jpeg we
> need
> > to do the same multiplication and set alpha to opaque.)
> 
> We don't need to do a the fast fixed point multiply for jpeg since alpha=255
for
> all jpeg pixels.
> In that case (alpha=255) buffer.setRGBA() is the same as *dest =
> SkPackARGB32NoCheck(255, r, g, b), or buffer.setRGBARaw(r, g, b, 255) in other
> words.  So in JPEGImageDecoder::setPixel() change
> 
>
https://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decode...
> 
> - buffer.setRGBA(pixel, jsample[0], jsample[1], jsample[2], 0xFF);
> + buffer.setRGBARaw(pixel, jsample[0], jsample[1], jsample[2], 0xFF);
> 
> and change
>
https://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decode...
> 
> - buffer.setRGBA(pixel, jsample[0] * k / 255, jsample[1] * k / 255, jsample[2]
*
> k / 255, 0xFF);
> + buffer.setRGBARaw(pixel, jsample[0] * k / 255, jsample[1] * k / 255,
> jsample[2] * k / 255, 0xFF);
> 
> and the jpeg tests should still pass.

It's not about premultiplying with alpha. I'm just trying to utilize that
computation wise the cmyk<->rgba conversation and alpha premultiplication is
similar.
We have a fast way to multiply with a/255 so we can reuse it for multiplying
with k/255. And it's better to only have it at one place. Does that sound
reasonable?

[kbalazs_, 2013-06-12 10:18:29]

ping

[Peter Kasting, 2013-06-12 18:47:06]

I don't know if you were waiting on me here.  I basically defer to Noel.  LGTM
from my end.

https://codereview.chromium.org/15466004/diff/27001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/15466004/diff/27001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:244: void
checkAddress(PixelData* address)
I would just omit this.  The previous code had a setRGBA() function that wrote
to a pointer without checking and I think it's OK for the new functions to do so
too.  This just adds verbosity and debug-mode slowness, callers should be doing
something legal here.

[kbalazs_, 2013-06-15 09:02:52]

>
https://codereview.chromium.org/15466004/diff/27001/Source/core/platform/imag...
> Source/core/platform/image-decoders/ImageDecoder.h:244: void
> checkAddress(PixelData* address)
> I would just omit this.  The previous code had a setRGBA() function that wrote
> to a pointer without checking and I think it's OK for the new functions to do
so
> too.  This just adds verbosity and debug-mode slowness, callers should be
doing
> something legal here.

Done.

Noel, could you take a final look?

[kbalazs_, 2013-06-18 19:35:51]

I'm going to land it if there is no opposition.

[commit-bot: I haz the power, 2013-06-19 08:03:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@sisa.samsung.com/15466004/35001

[commit-bot: I haz the power, 2013-06-19 08:23:05]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[Noel Gordon, 2013-06-19 13:22:54]

On 2013/06/18 19:35:51, kbalazs wrote:
> I'm going to land it if there is no opposition.

The opposition appears to be tests. 

c:\src\chrome\src\third_party\WebKit>python Tools\Scripts\run-webkit-tests
--chromium  LayoutTests
\fast\images
Using port 'chromium-win-win7'
Test configuration: <win7, x86, release>
Placing test results in c:\src\chrome\src\webkit\Release\layout-test-results
Baseline search path: chromium-win -> generic
Using Release build
Pixel tests enabled
Regular timeout: 6000, slow test timeout: 30000
Command line: c:\src\chrome\src\build\Release\content_shell.exe
--dump-render-tree -

Found 86 tests; running 84, skipping 2.
Running 2 content_shells in parallel.

[2/84] fast/images/png-suite/test.html failed unexpectedly (image diff)
[3/84] fast/images/animated-gif-with-offsets.html failed unexpectedly (image
diff)
[18/84] fast/images/favicon-as-image.html failed unexpectedly (image diff)
[19/84] fast/images/gif-large-checkerboard.html failed unexpectedly (image diff)
[20/84] fast/images/gif-loop-count.html failed unexpectedly (image diff)
[21/84] fast/images/gif-short-app-extension-string.html failed unexpectedly
(image diff)
[24/84] fast/images/icon-0colors.html failed unexpectedly (image diff)
[25/84] fast/images/icon-decoding.html failed unexpectedly (image diff)
[32/84] fast/images/image-map-anchor-children.html failed unexpectedly (image
diff)
[37/84] fast/images/imagemap-circle-focus-ring.html failed unexpectedly (image
diff)
[39/84] fast/images/imagemap-focus...tly-inherited-from-map.html failed
unexpectedly (image diff)
[40/84] fast/images/imagemap-focus...not-inherited-from-map.html failed
unexpectedly (image diff)
[41/84] fast/images/imagemap-focus-ring-outline-color.html failed unexpectedly
(image diff)
[44/84] fast/images/imagemap-focus-ring-zoom.html failed unexpectedly (image
diff)
[45/84] fast/images/imagemap-focus-ring.html failed unexpectedly (image diff)
[46/84] fast/images/imagemap-polygon-focus-ring.html failed unexpectedly (image
diff)
[55/84] fast/images/paint-subrect-grid.html failed unexpectedly (image diff)
[64/84] fast/images/png_per_row_alpha_decoding.html failed unexpectedly (image
diff)
[68/84] fast/images/rgb-jpeg-with-adobe-marker-only.html failed unexpectedly
(image diff)

Retrying 19 unexpected failure(s) ...

[Noel Gordon, 2013-06-19 13:34:59]

Here's my proposal:

1) As mentioned before, could we remove the webp changes form this patch while
urvang@ is trying to land the animated webp changes.  We could file a bug about
it and deal with on that bug once he is done.  That would make things easier for
him, sorry for the delay.

2) Could we do the JPEG changes on separate bug?  CMYK and RGB JPEG's are not
"hot loops" to my knowledge.

3) Could deal with the PNG changes on this bug, and update the change-log, and
get the tests passing.  I'd like to see a setRGBARaw() routine and a
setRGBAPremultiply() routine, as I've mentioned before, and have the PNG changes
written in terms of those new routines.  Possible?

[Noel Gordon, 2013-06-19 13:39:28]

https://codereview.chromium.org/15466004/diff/35001/Source/core/platform/imag...
File Source/core/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/15466004/diff/35001/Source/core/platform/imag...
Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:500:
buffer.setRGBAPremultiply(address + x, pixel[3], pixel[0], pixel[1], pixel[2]);
This and (line 505) can't be right and it's the cause of the test failures.

[kbalazs_, 2013-06-19 14:22:14]

On 2013/06/19 13:39:28, Noel Gordon (Google) wrote:
>
https://codereview.chromium.org/15466004/diff/35001/Source/core/platform/imag...
> File Source/core/platform/image-decoders/png/PNGImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/15466004/diff/35001/Source/core/platform/imag...
> Source/core/platform/image-decoders/png/PNGImageDecoder.cpp:500:
> buffer.setRGBAPremultiply(address + x, pixel[3], pixel[0], pixel[1],
pixel[2]);
> This and (line 505) can't be right and it's the cause of the test failures.

D'oh, finally I messed it up. Let's then go with just png first (which was my
original intent btw). I'm going to close this and upload it to a clean one.