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::copyBitmapData(const ImageFrame& other, ColorType targetType)

[scroggo_chromium, 2015/12/03 21:47:21]

This method is almost identical to the other one. Why not use the same method,
perhaps with a default parameter?

[aleksandar.stojiljkovic, 2015/12/04 00:07:35]

Again had the same reasoning - as this one is converting and other copying,
wanted to keep existing functionality as it is and maybe rename this one to
convertBitmapData. Also default parameter looked complicated since couldn't use
something like: targetType = other.targetType.

+{
+    if (this == &other)

[scroggo_chromium, 2015/12/03 21:47:21]

I wonder if we ever reach this condition, or if it's here just in case.

We have two cases where we call copyBitmapData; in GIF and WEBP. The code looks
the same for both ([1] and [2]), and it looks like we should never be copying to
itself.

In your version though, we return true even if the caller requested the other
ColorType, which seems wrong.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

Yes, that is good point. ASSERT and return false if equal? 

+        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, size_t indexOfTransparent)
+{
+    ASSERT(!width() && !height());
+
+    SkPMColor ctStorage[256];
+    SkPMColor* ctStorageIter(ctStorage);
+    // Copy color palette and set transparent pixel.
+    Vector<PixelData>::const_iterator colorMapEnd = (colorMap.size() <= 256) ? colorMap.end() : (colorMap.begin() + 256);
+    for (Vector<PixelData>::const_iterator colorMapIter = colorMap.begin(); colorMapIter != colorMapEnd; )
+        *ctStorageIter++ = *colorMapIter++;
+    if (indexOfTransparent < colorMap.size()) {
+        ctStorage[indexOfTransparent] = 0;
+    }
+    // Fill with transparent 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));
+    m_bitmap.setInfo(SkImageInfo::Make(newWidth, newHeight, kIndex_8_SkColorType, kPremul_SkAlphaType));

[scroggo_chromium, 2015/12/03 21:47:21]

FWIW, since the alpha is either 255 or zero, this would be a valid kPremul or
kUnpremul. We might need to make a choice based on m_premultiplyAlpha in case
any code cares to know. OTOH, Skia still does not support drawing from
kUnpremul, so maybe we should leave it kPremul (and maybe include a comment?)

+    if (!m_bitmap.tryAllocPixels(m_allocator, ctable))
+        return false;
+    size_t transparent = 0xFF;
+    if (indexOfTransparent < colorMap.size()) {

[scroggo_chromium, 2015/12/03 21:47:21]

Otherwise, we'll just initialize the bitmap to 255? That seems problematic if
the color table has 256 colors, but the last color is not transparent. Maybe we
should not use Index8 in that case?

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

Yes, partially downloaded content in that case would have random (255) color the
missing part at the bottom. 
Important to cover this in byte by byte tests.

+        transparent = indexOfTransparent;
+    }
+    for (int j = 0; j < newHeight; ++j) {
+        memset(getAddr8(0, j), transparent, sizeof(uint8_t) * newWidth);

[scroggo_chromium, 2015/12/03 21:47:21]

I think you could do this for the entire image with one memset. Since you did
not specify a rowBytes, the bitmap will be tightly packed, so it wouldn't even
write to extra memory.

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

Acknowledged.

+    }
+    m_hasAlpha = true;
+    return true;
+}
+
 bool ImageFrame::setSize(int newWidth, int newHeight)
 {
     // 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();
@@ skipping 31 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, 2015/12/03 21:47:21]

Again, this is going to erase to an arbitrary color in the colortable. I think
you actually want to use the color of the transparent index, if there is one.

[aleksandar.stojiljkovic, 2015/12/04 00:07:34]

Colortable returned by this decoder always have 256 colors, and all entries from
set of declared colors to the end are transparent color. For partially
downloaded content in opaque frames this seemed needed. Otherwise, I'll check if
making all colortables like this in skia could speed some of the CPU side
rendering (not to check the size)

[scroggo_chromium, 2015/12/08 22:24:51]

Except that if the image has 256 declared colors, the last one may not be
transparent, as you acknowledge above.
We need to do something. I'm not convinced that drawing a random color at the
bottom is the right approach. Doesn't GIF optionally have a background color? If
the frame is opaque, using the background color seems like a good option.
I don't follow. Make skia colortables like what?

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