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 90 matching lines @@
   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());

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

Yeah, this DCHECK looks entirely bogus to me now.  We have called setSize() on
the image buffer well before we get here these days, right?  (Please check).

If that is true, that'd imply there is a width() and height(), so perhaps the
right DCHECK to use here now is

   DCHECK(isDecodedSizeAvailble());

or some such?

Second thing is: the original DCHECK was there to prevent this routine from
being called twice, since it did actually call setSize() back in the day, and
calling it twice did in fact leak memory.

Not sure if tryAllocPixel prevents such leaks, but that doesn't really matter
either if the rule is "only call the routine once".  No leaks then.  And I'd be
semi-suspicious on any code that tried calling this routine twice.  To enforce a
call-once rule, perhaps

   DCHECK(m_status == ImageFrame::FrameEmpty);

would get us part-way there.  That might bust MockImageDecoder mind, but we
could fix that, or find some other way to say with a DCHECK(),
call-me-once-only.

Agree with the premise of the patch; setSizeAndColorSpace no longer cuts it as a
name.  The code appears to set|setup|create the backing store for the
ImageFrame, the store being a SkBitmap now, which might be a SkSurface in
future.

So to begin the bike shed, maybe we should call it:

  createBackingStore(width, height, colorSpace);

or variations on that theme (createPixelStore or whatever).

[cblume, 2017/03/14 07:59:51]

I was talking to scroggo@ about the don't-leak-memory thing. I think you are
right, it will be backed by a SkBitmap and will not leak. That is, unless an
external allocator is supplied. Then I'm not sure if it could leak.

And I agree with renaming the function to something more appropriate now.

[f(malita), 2017/03/14 13:18:05]

+1 to name change.  Since this is supposed to be one-time initialization, I
would also consider init() or such.

[Noel Gordon, 2017/03/14 14:26:26]

Agree with the one-time initialization part. However, init() might imply that
the pixels in the backing store were initialized also?  Most we do is allocate,
doing a malloc-like allocation ...

  allocateBackingStore, allocateStorage, and so on.

[scroggo_chromium, 2017/03/14 14:28:59]

I think you're confused - this is ImageFrame, not ImageDecoder. No way to check
isDecodedSizeAvailable() from here, and width() and height() are not yet set.
They are set on the ImageDecoder, but this is where they get set in ImageFrame.
The comment is bogus, but width() and height() return the size of the bitmap,
which starts off as 0 x 0 and gets set down below, so I think the DCHECK itself
is fine. Checking m_status is also reasonable, except this method doesn't change
the status.

[scroggo_chromium, 2017/03/14 14:28:59]

The ExternalMemoryAllocator supplies an SkPixelRef which does not own its pixel
memory, because it is owned somewhere else. (You can trace through the function
calls to find the actual owner. In the software case at least, it is discardable
memory created by the SoftwareImageDecodeCache.) It would only leak memory if
someone used a custom allocator that does something stupid.

[Noel Gordon, 2017/03/14 14:49:10]

Ah yes, I was using ImageDecoder thinking, not ImageFrame thinking.

width() and height() are not yet set.
Right.
Agree, now I see that width() and height() are from the m_bitmap. The DCHECK is
fine.  The comment needs fixing, and the routine needs a new name.

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

Name updated.
Removed the comment.

 
   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