Refactor ImageFrame::setSizeAndColorSpace()

third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  *
  * Portions are Copyright (C) 2001-6 mozilla.org
  *
  * Other contributors:
  *   Stuart Parmenter <stuart@mozilla.com>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ skipping 923 matching lines @@
   if (buffer.getStatus() == ImageFrame::FrameEmpty) {
     DCHECK_EQ(info->output_width,
               static_cast<JDIMENSION>(m_decodedSize.width()));
     DCHECK_EQ(info->output_height,
               static_cast<JDIMENSION>(m_decodedSize.height()));
 
     if (!buffer.setSizeAndColorSpace(info->output_width, info->output_height,
                                      colorSpaceForSkImages()))
       return setFailed();
 
+    buffer.zeroFillPixelData();

[Noel Gordon, 2017/03/14 06:30:41]

Here and elsewhere you made this change, consider using

buffer.zeroFillPixelData();
DCHECK(buffer.hasAlpha());

to maybe remind everyone of that important contract?  (Since it so often comes
as a discussion point in image decoder changes).

[Peter Kasting, 2017/03/14 07:34:40]

Hmm.

It worries me that zeroFillPixelData() sets m_hasAlpha = true, but setHasAlpha()
also calls m_bitmap.setAlphaType().  DCHECK(buffer.hasAlpha()) implies one
doesn't need to call setHasAlpha(true), as we currently do e.g. below.  But
unless the eraseARGB() call in zeroFillPixelData() does this automatically,
omitting that will subtly modify the effect of the code.

I wonder if zeroFillPixelData() should be calling setHasAlpha(true) instead of
just twiddling m_hasAlpha.

[cblume, 2017/03/14 17:30:50]

I think both of you had really good points.
I would be happy to remind people that zero filling means we're also talking
about alpha.
But I also don't want to mislead people into thinking they don't need to call a
function which is required.

I don't immediately see a reason why we set m_hasAlpha = true (inside
zeroFillPixelData()) and don't just call setHasAlpha(true). Is there a reason to
set m_hasAlpha = true without also calling m_bitmap.setAlphaType() ?

I feel like it would be lovely if zeroFillPixelData() called
m_bitmap.setAlphaType() and we could get rid of the call to
buffer.setHasAlpha(true) below. But there might be a reason we cannot do that.

+
     // The buffer is transparent outside the decoded area while the image is
     // loading. The image will be marked fully opaque in complete().
     buffer.setStatus(ImageFrame::FramePartial);
     buffer.setHasAlpha(true);
 
     // For JPEGs, the frame always fills the entire image.
     buffer.setOriginalFrameRect(IntRect(IntPoint(), size()));
   }
 
 #if defined(TURBO_JPEG_RGB_SWIZZLE)
@@ skipping 55 matching lines @@
   // has failed.
   if (!m_reader->decode(onlySize) && isAllDataReceived())
     setFailed();
 
   // If decoding is done or failed, we don't need the JPEGImageReader anymore.
   if (isComplete(this, onlySize) || failed())
     m_reader.reset();
 }
 
 }  // namespace blink