Use sk_sp<SkBitmap::Allocator> instead of raw pointers or WTF::RefPtr<T>.

third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp

 /*
  * Copyright (C) 2012 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:
  * 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 106 matching lines @@
     ImageDecodingStore::instance().removeCacheIndexedByGenerator(this);
 }
 
 bool ImageFrameGenerator::decodeAndScale(SegmentReader* data, bool allDataReceived, size_t index, const SkImageInfo& info, void* pixels, size_t rowBytes)
 {
     if (m_decodeFailed)
         return false;
 
     TRACE_EVENT1("blink", "ImageFrameGenerator::decodeAndScale", "frame index", static_cast<int>(index));
 
-    RefPtr<ExternalMemoryAllocator> externalAllocator = adoptRef(new ExternalMemoryAllocator(info, pixels, rowBytes));
+    sk_sp<ExternalMemoryAllocator> externalAllocator(new ExternalMemoryAllocator(info, pixels, rowBytes));
 
     // This implementation does not support scaling so check the requested size.
     SkISize scaledSize = SkISize::Make(info.width(), info.height());
     ASSERT(m_fullSize == scaledSize);
 
-    // TODO (scroggo): Convert tryToResumeDecode() and decode() to take a
-    // sk_sp<SkBitmap::Allocator> instead of a bare pointer.
-    SkBitmap bitmap = tryToResumeDecode(data, allDataReceived, index, scaledSize, externalAllocator.get());
+    SkBitmap bitmap = tryToResumeDecode(data, allDataReceived, index, scaledSize, std::move(externalAllocator));
     if (bitmap.isNull())
         return false;
 
     // Check to see if the decoder has written directly to the pixel memory
     // provided. If not, make a copy.
     ASSERT(bitmap.width() == scaledSize.width());
     ASSERT(bitmap.height() == scaledSize.height());
     SkAutoLockPixels bitmapLock(bitmap);
     if (bitmap.getPixels() != pixels)
         return bitmap.copyPixelsTo(pixels, rowBytes * info.height(), rowBytes);
@@ skipping 27 matching lines @@
     if (decoder->decodeToYUV()) {
         setHasAlpha(0, false); // YUV is always opaque
         return true;
     }
 
     ASSERT(decoder->failed());
     m_yuvDecodingFailed = true;
     return false;
 }
 
-SkBitmap ImageFrameGenerator::tryToResumeDecode(SegmentReader* data, bool allDataReceived, size_t index, const SkISize& scaledSize, SkBitmap::Allocator* allocator)
+SkBitmap ImageFrameGenerator::tryToResumeDecode(SegmentReader* data, bool allDataReceived, size_t index, const SkISize& scaledSize, sk_sp<SkBitmap::Allocator> allocator)
 {
     TRACE_EVENT1("blink", "ImageFrameGenerator::tryToResumeDecode", "frame index", static_cast<int>(index));
 
     ImageDecoder* decoder = 0;
 
     // Lock the mutex, so only one thread can use the decoder at once.
     MutexLocker lock(m_decodeMutex);
     const bool resumeDecoding = ImageDecodingStore::instance().lockDecoder(this, m_fullSize, &decoder);
     ASSERT(!resumeDecoding || decoder);
 
     SkBitmap fullSizeImage;
-    bool complete = decode(data, allDataReceived, index, &decoder, &fullSizeImage, allocator);
+    bool complete = decode(data, allDataReceived, index, &decoder, &fullSizeImage, std::move(allocator));
 
     if (!decoder)
         return SkBitmap();
 
     // If we are not resuming decoding that means the decoder is freshly
     // created and we have ownership. If we are resuming decoding then
     // the decoder is owned by ImageDecodingStore.
     std::unique_ptr<ImageDecoder> decoderContainer;
     if (!resumeDecoding)
         decoderContainer = wrapUnique(decoder);
@@ skipping 39 matching lines @@
     MutexLocker lock(m_alphaMutex);
     if (index >= m_hasAlpha.size()) {
         const size_t oldSize = m_hasAlpha.size();
         m_hasAlpha.resize(index + 1);
         for (size_t i = oldSize; i < m_hasAlpha.size(); ++i)
             m_hasAlpha[i] = true;
     }
     m_hasAlpha[index] = hasAlpha;
 }
 
-bool ImageFrameGenerator::decode(SegmentReader* data, bool allDataReceived, size_t index, ImageDecoder** decoder, SkBitmap* bitmap, SkBitmap::Allocator* allocator)
+bool ImageFrameGenerator::decode(SegmentReader* data, bool allDataReceived, size_t index, ImageDecoder** decoder, SkBitmap* bitmap, sk_sp<SkBitmap::Allocator> allocator)
 {
     ASSERT(m_decodeMutex.locked());
     TRACE_EVENT2("blink", "ImageFrameGenerator::decode", "width", m_fullSize.width(), "height", m_fullSize.height());
 
     // Try to create an ImageDecoder if we are not given one.
     ASSERT(decoder);
     bool newDecoder = false;
     bool shouldCallSetData = true;
     if (!*decoder) {
         newDecoder = true;
         if (m_imageDecoderFactory)
             *decoder = m_imageDecoderFactory->create().release();
 
         if (!*decoder) {
             *decoder = ImageDecoder::create(data, allDataReceived, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied).release();
             // The newly created decoder just grabbed the data.  No need to reset it.
             shouldCallSetData = false;
         }
 
         if (!*decoder)
             return false;
     }
 
     if (!m_isMultiFrame && newDecoder && allDataReceived) {
         // If we're using an external memory allocator that means we're decoding
         // directly into the output memory and we can save one memcpy.
         ASSERT(allocator);
-        (*decoder)->setMemoryAllocator(allocator);
+        (*decoder)->setMemoryAllocator(std::move(allocator));

[Łukasz Anforowicz, 2016/09/09 20:10:04]

|allocator| comes from |decodeAndScale|, which will destroy |allocator| via the
stack-allocated smart pointer.  I don't understand why in the old code it was
safe to pass |allocator| (short-lived, owned by callstack frame of
|decodeAndScale|) into |decoder| (potentially long-lived, right?).  At any rate,
I think the new code is safe in that regard.

[scroggo_chromium, 2016/09/09 20:51:04]

It's safe (but arguably not safe from future changes) because we call
setMemoryAllocator(0) down below (line 297 in this version).

Before we had sk_sp, I started changing this to use RefPtr. It's probably worth
looking at Florin's comments in that CL:

https://codereview.chromium.org/1880993003/#msg10

[Łukasz Anforowicz, 2016/09/12 20:50:09]

My first priority is to eradicate RefPtr<SkFoo> (because this blocks Chromium
style migration).  I am not sure how to get there:

1. Minimal CL (i.e. only 2 line change: RefPtr->sk_sp + adoptRef->sk_sp
contructor).

2. #1 + also switch from raw pointer to smart pointer in decode and
tryToResumeDecode methods.

3. #2 + also switch from raw pointer to smart pointer the
ImageFrame::m_allocator field.

4. Long-term refactoring (i.e. remove setAllocator from ImageFrame and from
ImageDecoder + pass a raw pointer to allocator through the whole method call
chain between ImageDecoder::frameBufferAtIndex and
ImageFrame::setSizeAndColorProfile).


There are two difficulties I see with #4.

- Size of the required refactoring.  I tried to look into this today and it
seems to be a massive refactoring (based on looking in Cr code search at the
size of the call tree of ImageFrame::setSizeAndColorProfile).

- Deriving from SkRefCnt - this cannot be changed without changing Skia and it
would feel weird/fragile to be stack-allocating an object derived from SkRefCnt.


So - WDYT?  Should I be trying to do #1, #2, #3 or #4? :-)  It sounds that #1
might be easiest to land, right?

[scroggo_chromium, 2016/09/13 14:54:42]

Another note: it would *not* be safe if we did not call setMemoryAllocator(0)
down below, even with your changes to use sk_sp, because the
ExternalMemoryAllocator has a pointer to memory it does not own. So this might
be suggesting some safety that isn't really there.
In theory, I would vote for #4, but there's no reason for that to hold you up.
To help you eradicate RefPtr, I think it makes more sense to stack allocate with
some comments explaining why it's safe (as discussed in
https://codereview.chromium.org/1880993003/#msg10).
FWIW, we do this elsewhere, and I think it is safe so long as you know what you
are doing.

Either way, this code is already fragile, and I don't think switching to sk_sp
as in this patch set (which I think corresponds to option #3) makes it less so.
If you're dead set against stack allocation, I suppose you could do #1, although
to be honest that is essentially the same (except it has a heap allocation!).

[Łukasz Anforowicz, 2016/09/13 18:17:31]

Thanks for pointing this out - I agree that using sk_sp<T> would be wrong,
because in this case would be giving an incorrect sense of safety.
Agreed - stack allocation makes sense to me now.

I think we don't necessarily need the comments from
https://codereview.chromium.org/1880993003/#msg10
that wonder whether ref is taken at any time.  We only care
that no extra ref is taken when the stack allocated object
gets destroyed.  Therefore, I think the comment + DCHECK
I have in the latest patchset should be sufficient.
Ack.  The DCHECK(x.unique()) makes me happier about this idea.
I see now how options #1 through #3 are wrong, because they don't offer
any extra safety (because the allocator keeps wrapping a short-lived pointer).

     }
 
     if (shouldCallSetData)
         (*decoder)->setData(data, allDataReceived);
     ImageFrame* frame = (*decoder)->frameBufferAtIndex(index);
 
     // For multi-frame image decoders, we need to know how many frames are
     // in that image in order to release the decoder when all frames are
     // decoded. frameCount() is reliable only if all data is received and set in
     // decoder, particularly with GIF.
@@ skipping 43 matching lines @@
         return false;
 
     // Setting a dummy ImagePlanes object signals to the decoder that we want to do YUV decoding.
     std::unique_ptr<ImagePlanes> dummyImagePlanes = wrapUnique(new ImagePlanes);
     decoder->setImagePlanes(std::move(dummyImagePlanes));
 
     return updateYUVComponentSizes(decoder.get(), sizeInfo->fSizes, sizeInfo->fWidthBytes);
 }
 
 } // namespace blink