Improve PNG decode performance: avoid branching in the row write loop

Source/platform/image-decoders/ImageFrame.h

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 144 matching lines @@
     inline PixelData* getAddr(int x, int y)
     {
         return m_bitmap.getAddr32(x, y);
     }
 
     inline void setRGBA(int x, int y, unsigned r, unsigned g, unsigned b, unsigned a)
     {
         setRGBA(getAddr(x, y), r, g, b, a);
     }
 
+    inline void setRGBA(PixelData* dest, unsigned r, unsigned g, unsigned b, unsigned a)
+    {
+        if (m_premultiplyAlpha)

[Peter Kasting, 2014/05/12 19:36:11]

We could simplify this to just:

    setRGBA<m_premultiplyAlpha>(dest, r, g, b, a);

[Noel Gordon, 2014/05/13 01:39:19]

Doesn't compile on any C++ compiler I know. VS2010, VS2013 complain with
"Template parameter must be a compile-time constant expression".

[Peter Kasting, 2014/05/13 01:54:56]

Huh, that's odd.  I could swear I've done precisely this before.  Oh well.  I
guess this should just manually call the two forms of setRGBA(), then.  (See
below)

+            setRGBA<true>(dest, r, g, b, a);
+        else
+            *dest = SkPackARGB32NoCheck(a, r, g, b);
+    }
+
     static const unsigned div255 = static_cast<unsigned>(1.0 / 255 * (1 << 24)) + 1;
 
+    template <bool Premultiply>
     inline void setRGBA(PixelData* dest, unsigned r, unsigned g, unsigned b, unsigned a)

[Peter Kasting, 2014/05/12 19:36:11]

This can be static, can't it?

[Noel Gordon, 2014/05/13 01:39:19]

Could be, but you'd then add function calling overhead to every pixel.

[Peter Kasting, 2014/05/13 01:54:56]

Why?  Can't something be both static and inline?  All marking it in static
should do is make it clear to the reader that we don't touch anything in |this|.

     {
-        if (m_premultiplyAlpha && a < 255) {
+        if (Premultiply && a < 255) {
             if (!a) {
                 *dest = 0;
                 return;
             }
 
             unsigned alpha = a * div255;
             r = (r * alpha) >> 24;
             g = (g * alpha) >> 24;
             b = (b * alpha) >> 24;
         }
 
         // Call the "NoCheck" version since we may deliberately pass non-premultiplied
         // values, and we don't want an assert.
         *dest = SkPackARGB32NoCheck(a, r, g, b);
     }
 
     inline void setRGBARaw(PixelData* dest, unsigned r, unsigned g, unsigned b, unsigned a)

[Peter Kasting, 2014/05/12 19:36:11]

Seems like this should just disappear, and callers should call setRGBA<false>().

[Noel Gordon, 2014/05/13 01:39:19]

Mentioned that in #15.  Was a little slower according to my measurements, so I
left it as is.

[Peter Kasting, 2014/05/13 01:54:56]

If the difference isn't large, let's make the change anyway.  Better to have
less code, and callers all calling one core function to set things, than having
duplicated SkPackARGB32NoCheck() calls in multiple places.

Though, there really isn't a good reason this ought to be slower.  I wonder if
somehow the optimizer is getting confused.  Does reversing the function
declaration order help?

In any case, if we don't end up with any calls to setRGBA<false>(), then
declaring it as a template arg makes no sense.  We should instead just rename
setRGBA<true> to setRGBAPremultiply, and setRGBARaw to setRGBANoPremultiply, and
be done.

     {
         *dest = SkPackARGB32NoCheck(a, r, g, b);
     }
 
     // Notifies the SkBitmap if any pixels changed and resets the flag.
     inline void notifyBitmapIfPixelsChanged()
     {
         if (m_pixelsChanged)
             m_bitmap.notifyPixelsChanged();
         m_pixelsChanged = false;
@@ skipping 30 matching lines @@
     // be read for image formats that do not have multiple frames.
     size_t m_requiredPreviousFrameIndex;
 #if !ASSERT_DISABLED
     bool m_requiredPreviousFrameIndexValid;
 #endif
 };
 
 } // namespace WebCore
 
 #endif