GIF decoding to Index8, unit tests and misusing unit test as benchmark

third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) 2008, 2009 Google, Inc.
  *
  * 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 76 matching lines @@
 bool ImageFrame::copyBitmapData(const ImageFrame& other)
 {
     if (this == &other)
         return true;
 
     m_hasAlpha = other.m_hasAlpha;
     m_bitmap.reset();
     return other.m_bitmap.copyTo(&m_bitmap, other.m_bitmap.colorType());
 }
 
-bool ImageFrame::setSize(int newWidth, int newHeight)
+bool ImageFrame::copyBitmapData(const ImageFrame& other, ColorType targetType)
+{
+    if (this == &other)
+        return true;
+
+    m_hasAlpha = other.m_hasAlpha;
+    m_bitmap.reset();
+    return other.m_bitmap.copyTo(&m_bitmap, static_cast<SkColorType>(targetType));
+}
+
+bool ImageFrame::setSizeIndex8(int newWidth, int newHeight, const Vector<PixelData>& colorMap, unsigned char backgroundIndex, bool backgroundIsTransparent)
+{
+    ASSERT(!width() && !height());
+
+    SkPMColor ctStorage[256];
+    SkPMColor* ctStorageIter(ctStorage);
+    // Copy color palette and set transparent pixel if set.
+    Vector<PixelData>::const_iterator colorMapEnd = (colorMap.size() <= 256) ? colorMap.end() : (colorMap.begin() + 256);

[scroggo_chromium, 2016/01/06 21:50:40]

The style guide seems to recommend using an index rather than an iterator.

[aleksandar.stojiljkovic, 2016/01/18 13:58:49]

Done.

+    for (Vector<PixelData>::const_iterator colorMapIter = colorMap.begin(); colorMapIter != colorMapEnd; )
+        *ctStorageIter++ = *colorMapIter++;
+    if (backgroundIsTransparent) {
+        ASSERT(backgroundIndex < colorMap.size());
+        ctStorage[backgroundIndex] = 0;
+    }
+    // Fill with backgroundIndex to the end.
+    int remaining = 256 - colorMap.size();
+    if (remaining > 0)
+        memset(ctStorage + 256 - remaining, 0, sizeof(SkPMColor) * remaining);
+    SkAutoTUnref<SkColorTable> ctable(new SkColorTable(ctStorage, 256));
+    size_t rowBytes = (newWidth + sizeof(size_t) - 1) & (~(sizeof(size_t) - 1));

[scroggo_chromium, 2016/01/06 21:50:40]

Why'd you choose this rowBytes? If you use zero, SkBitmap::setInfo will pick the
minimum one for you.

[aleksandar.stojiljkovic, 2016/01/18 13:58:49]

I think the 0 makes rowBytes equals to newWidth. 
What I needed was size_t aligned row (nearest multiplier of size_t size), so
that it is not needed to copy byte by byte but to utilize memory alignment so
that size_t words can be handled. Verified this to be a bit faster on android
arm...  

[scroggo_chromium, 2016/04/29 19:48:14]

Will you please add a comment explaining this? 

+    m_bitmap.setInfo(SkImageInfo::Make(newWidth, newHeight, kIndex_8_SkColorType, kPremul_SkAlphaType), rowBytes);
+    if (!m_bitmap.tryAllocPixels(m_allocator, ctable))
+        return false;
+    memset(getAddr8(0, 0), backgroundIndex, rowBytes * newHeight);
+    m_hasAlpha = backgroundIsTransparent;
+    return true;
+}
+
+bool ImageFrame::setSize(int newWidth, int newHeight, SkColor background)
 {
     // setSize() should only be called once, it leaks memory otherwise.
     ASSERT(!width() && !height());
 
     m_bitmap.setInfo(SkImageInfo::MakeN32Premul(newWidth, newHeight));
     if (!m_bitmap.tryAllocPixels(m_allocator, 0))
         return false;
 
-    zeroFillPixelData();
+    m_bitmap.eraseColor(background);
+    m_hasAlpha = !((background >> 24) == 0xFF);

[scroggo_chromium, 2016/01/06 21:50:40]

Why not SkColorGetA(background)?

[aleksandar.stojiljkovic, 2016/01/18 13:58:49]

Done. Thanks, didn't know about it.

+
     return true;
 }
 
 const SkBitmap& ImageFrame::bitmap() const
 {
     return m_bitmap;
 }
 
 bool ImageFrame::hasAlpha() const
 {
@@ skipping 21 matching lines @@
         // the bitmap has been marked immutable.
         notifyBitmapIfPixelsChanged();
         m_bitmap.setImmutable(); // Tell the bitmap it's done.
     }
 }
 
 void ImageFrame::zeroFillFrameRect(const IntRect& rect)
 {
     if (rect.isEmpty())
         return;
-
-    m_bitmap.eraseArea(rect, SkColorSetARGB(0, 0, 0, 0));
+    if (m_bitmap.colorType() == kIndex_8_SkColorType) {
+        int ymax = std::min(rect.y() + rect.height(), m_bitmap.height());
+        int xcount = std::min(rect.width(), m_bitmap.width() - rect.x());
+        for (int j = rect.y(); j < ymax; ++j) {
+            memset(m_bitmap.getAddr8(0, j), 0xFF, sizeof(uint8_t) * xcount);

[scroggo_chromium, 2016/01/06 21:50:40]

As stated before, I'm concerned about setting this to an arbitrary color in the
color table. Are we guaranteed that this one is transparent?

[aleksandar.stojiljkovic, 2016/01/18 13:58:49]

Fixed in new patch...

[scroggo_chromium, 2016/04/29 19:48:14]

It's not obvious to me that it is. See comments in the new patch.

+        }
+    } else {
+        m_bitmap.eraseArea(rect, SkColorSetARGB(0, 0, 0, 0));
+    }
     setHasAlpha(true);
 }
 
 } // namespace blink