Refactor ImageFrame::setSizeAndColorSpace()

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 60 matching lines @@
 void ImageFrame::clearPixelData() {
   m_bitmap.reset();
   m_status = FrameEmpty;
   // NOTE: Do not reset other members here; clearFrameBufferCache()
   // calls this to free the bitmap data, but other functions like
   // initFrameBuffer() and frameComplete() may still need to read
   // other metadata out of this frame later.
 }
 
 void ImageFrame::zeroFillPixelData() {
+  setHasAlpha(true);

[Noel Gordon, 2017/03/16 12:01:50]

Harmless, but also redundant.  It sets the m_bitmap alpha type to 

m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType

aka the bitmap backing store has alpha.  But allocatePixelData already did that
@line 111 below.  No real need to do it twice.

[cblume, 2017/03/17 05:35:14]

Wait....you're right.
I could have sworn it made perfect sense just yesterday.

All the existing call sites call allocatePixelData() before calling
zeroFillPixelData().

The other call site I care about which isn't shown here is the reason I created
this CR: So I can call setSizeAndColorSpace() without zero-filling. In that
case, I am not calling zeroFillPixelData().


I don't think there is a case where we need to do anything with the alpha inside
zeroFillPixelData(). I think we could have just removed the m_hasAlpha = true;
line. And I think we saw it and thought "Probably should be calling the
function."

I'll go ahead and remove it.

[Peter Kasting, 2017/03/17 06:35:50]

Because the BMP decoder needs it, no?

It allocates and zeros the pixel data and then explicitly calls
setHasAlpha(false).

Later when it calls zeroFillPixelData() again it expects that to mark the image
as having alpha.

I don't think this can be removed, because zeroFillPixelData() is not called
only at image creation time.

[cblume, 2017/03/17 18:05:45]

It is now being explicitly set in BMPImageReader.cpp:827 with

m_buffer->zeroFillPixelData();
m_buffer->setHasAlpha(true);

since this was the only location that needed both and the concepts are separate.

[Noel Gordon, 2017/03/20 13:24:52]

Not sure they're separate concepts.  Somewhat odd to me to zero the data and
then not set m_alpha = true.  (And then Peter spotted the BMP bug that would
have resulted).  I think the body should read:

m_bitmap.eraseARGB(0, 0, 0, 0);
m_hasAlpha = true;

as it was in diff-left, since your goal here was to just split the calling of
this function out, without change behavior :)

That way you wouldn't need to touch the BMP decoder, and the comments therein
(near line@127) seem to describe what is expected pretty well?

   m_bitmap.eraseARGB(0, 0, 0, 0);
-  m_hasAlpha = true;
 }
 
 bool ImageFrame::copyBitmapData(const ImageFrame& other) {
   DCHECK_NE(this, &other);
   m_hasAlpha = other.m_hasAlpha;
   m_bitmap.reset();
   return other.m_bitmap.copyTo(&m_bitmap, other.m_bitmap.colorType());
 }
 
 bool ImageFrame::takeBitmapDataIfWritable(ImageFrame* other) {
   DCHECK(other);
   DCHECK_EQ(FrameComplete, other->m_status);
   DCHECK_EQ(FrameEmpty, m_status);
   DCHECK_NE(this, other);
   if (other->m_bitmap.isImmutable())
     return false;
   m_hasAlpha = other->m_hasAlpha;
   m_bitmap.reset();
   m_bitmap.swap(other->m_bitmap);
   other->m_status = FrameEmpty;
   return true;
 }
 
-bool ImageFrame::setSizeAndColorSpace(int newWidth,
-                                      int newHeight,
-                                      sk_sp<SkColorSpace> colorSpace) {
-  // setSizeAndColorSpace() should only be called once, it leaks memory
-  // otherwise.
-  DCHECK(!width() && !height());
-
+bool ImageFrame::allocatePixelData(int newWidth,
+                                   int newHeight,
+                                   sk_sp<SkColorSpace> colorSpace) {
   m_bitmap.setInfo(SkImageInfo::MakeN32(
       newWidth, newHeight,
       m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,
       std::move(colorSpace)));
-  if (!m_bitmap.tryAllocPixels(m_allocator, 0))
-    return false;
-
-  zeroFillPixelData();
-  return true;
+  return m_bitmap.tryAllocPixels(m_allocator, 0);
 }
 
 bool ImageFrame::hasAlpha() const {
   return m_hasAlpha;
 }
 
 sk_sp<SkImage> ImageFrame::finalizePixelsAndGetImage() {
   DCHECK_EQ(FrameComplete, m_status);
   m_bitmap.setImmutable();
   return SkImage::MakeFromBitmap(m_bitmap);
@@ skipping 75 matching lines @@
   // If the frame is not fully loaded, there will be transparent pixels,
   // so we can't tell skia we're opaque, even for image types that logically
   // always are (e.g. jpeg).
   if (!m_hasAlpha && m_status == FrameComplete)
     return kOpaque_SkAlphaType;
 
   return m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType;
 }
 
 }  // namespace blink