Separate zeroing pixel data from setting alpha

third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 106 matching lines @@
   // Initialize the framebuffer if needed.
   DCHECK(m_buffer);  // Parent should set this before asking us to decode!
   if (m_buffer->getStatus() == ImageFrame::FrameEmpty) {
     if (!m_buffer->allocatePixelData(m_parent->size().width(),
                                      m_parent->size().height(),
                                      m_parent->colorSpaceForSkImages())) {
       return m_parent->setFailed();  // Unable to allocate.
     }
     m_buffer->zeroFillPixelData();
     m_buffer->setStatus(ImageFrame::FramePartial);
     // setSize() calls eraseARGB(), which resets the alpha flag, so we force

[Noel Gordon, 2017/03/20 21:09:03]

Comment seems out-of-date.

[cblume, 2017/03/20 22:40:28]

Oh, you're right.
In fact, the comment indicates that the following m_buffer->setHasAlpha(false);
is no longer required.

It sounds like it was only there because the old setSizeAndColorSpace() would
reset alpha to true. And this line had to set it back.

I think the comment only exists to explain that line. I think we can get rid of
both.

[Peter Kasting, 2017/03/20 22:42:04]

Just be careful that at that point the code has an undocumented implicit
assumption that frames are opaque by default.  If we wanted to change that
assumption globally, nothing would make it obvious that the code here would be
impacted.

[cblume, 2017/03/20 23:14:00]

You are right.
I think it would be better for the decoder to explicitly set the frames to what
they want.

I'll look at locations to do that. Maybe each decoder knows what it wants its
default to be when initializing the frame.

     // it back to false here.  We'll set it true below in all cases where
     // these 0s could actually show through.
     m_buffer->setHasAlpha(false);
 
     // For BMPs, the frame always fills the entire image.
     m_buffer->setOriginalFrameRect(IntRect(IntPoint(), m_parent->size()));
 
     if (!m_isTopDown)
       m_coord.setY(m_parent->size().height() - 1);
   }
@@ skipping 679 matching lines @@
         // images where all alpha values are 255; opaque images are
         // faster to draw.
         int alpha = getAlpha(pixel);
         if (!m_seenNonZeroAlphaPixel && !alpha) {
           m_seenZeroAlphaPixel = true;
           alpha = 255;
         } else {
           m_seenNonZeroAlphaPixel = true;
           if (m_seenZeroAlphaPixel) {
             m_buffer->zeroFillPixelData();
+            m_buffer->setHasAlpha(true);
             m_seenZeroAlphaPixel = false;
           } else if (alpha != 255)
             m_buffer->setHasAlpha(true);
         }
 
         setRGBA(getComponent(pixel, 0), getComponent(pixel, 1),
                 getComponent(pixel, 2), alpha);
       }
     }
 
     // Success, keep going.
     m_decodedOffset += paddedNumBytes;
     if (inRLE)
       return Success;
     moveBufferToNextRow();
   }
 
   // Finished decoding whole image.
   return Success;
 }
 
 void BMPImageReader::moveBufferToNextRow() {
   m_coord.move(-m_coord.x(), m_isTopDown ? 1 : -1);
 }
 
 }  // namespace blink