Remove duplication of encoded image data

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

 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2008 Apple 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
@@ skipping 100 matching lines @@
 
 void BitmapImage::destroyDecodedData()
 {
     m_cachedFrame.clear();
     for (size_t i = 0; i < m_frames.size(); ++i)
         m_frames[i].clear(true);
     m_source.clearCacheExceptFrame(kNotFound);
     notifyMemoryChanged();
 }
 
+PassRefPtr<SharedBuffer> BitmapImage::data()

[hiroshige, 2016/06/29 07:04:06]

The name data() here (and ImageSource) looks like just an accessor, but actually
it re-creates SharedBuffer if needed. How about renaming it to a name that looks
like more costly?

[hajimehoshi, 2016/07/04 10:46:25]

This method is overriding Image::data. Do you mean we should rename Image::data
and its all usages?

+{
+    if (RefPtr<SharedBuffer> data = cachedData())
+        return data;

[f(malita), 2016/06/30 18:21:46]

Nit: return data.release();

[hajimehoshi, 2016/07/04 10:46:25]

Done.

+    return m_source.data();

[f(malita), 2016/06/30 18:21:46]

IIUC we clear the cached data as soon as we can, and m_source.data() allocates a
new SharedBuffer and makes a copy of the decoder data.

So while previously Resource::resourceBuffer() calls used to be always cheap,
now that's no longer the case for lazy-decoded images.  Is this going to impact
performance?

[hajimehoshi, 2016/07/04 10:46:25]

IIUC the situation where this function is limited: clipboard, serializing to
MHTML or WebGL. Among them, I think only WebGL can be a problem. I'll try to
check this with Telemetry.

+}
+
+PassRefPtr<SharedBuffer> BitmapImage::cachedData()

[scroggo_chromium, 2016/06/28 15:33:44]

A couple of thoughts on this method:

It is very simple, and we could almost inline it at the callsites, except that
you want to call it from a test.

I find the name confusing. It's not obvious to me which one is "cached" and
which one is not. I guess we call it cached because Image may throw away its
data?

[hiroshige, 2016/06/29 07:04:06]

FYI using "cached" prefix near loading code might be confusing; There are
prefixes "cached" in loader code (e.g. |cachedImage|), but most of them are just
due to historical reasons (CachedImage is the old class name of ImageResource)
and don't have much meaning.

[hajimehoshi, 2016/07/04 10:46:25]

Removed.

+{
+    return Image::data();
+}
+
 void BitmapImage::notifyMemoryChanged()
 {
     if (getImageObserver())
         getImageObserver()->decodedSizeChangedTo(this, totalFrameBytes());
 }
 
 size_t BitmapImage::totalFrameBytes()
 {
     const size_t numFrames = frameCount();
     size_t totalBytes = 0;
@@ skipping 80 matching lines @@
         // cleared-on-a-previous-pass) frames!
         if (m_frames[i].m_haveMetadata && !m_frames[i].m_isComplete) {
             m_frames[i].clear(true);
             if (i == m_cachedFrameIndex)
                 m_cachedFrame.clear();
         }
     }
 
     // Feed all the data we've seen so far to the image decoder.
     m_allDataReceived = allDataReceived;
-    ASSERT(data());
-    m_source.setData(*data(), allDataReceived);
+    if (cachedData()) {
+        m_source.setData(*cachedData(), allDataReceived);
+        if (m_source.hasData()) {
+            // Remove the cached data (encoded image data) from BitmapImage. Now
+            // there is an encoded image data in DefferedImageDecoder on a skia

[scroggo_chromium, 2016/06/28 15:33:44]

nit: "there is encoded image data" (no need for "an" here)

nit: *DeferredImageDecoder (one 'f', two 'r's)

nit: I don't think "on a skia side" adds useful information. Maybe it's more
clear to say something like "in DeferredImageDecoder's SkRWBuffer"?

[hajimehoshi, 2016/07/04 10:46:25]

Done.

+            // side, we won't need the same data in BitmapImage.
+            resetCachedData();

[scroggo_chromium, 2016/06/28 15:33:44]

Just to make sure I understand - this will drop the Image's reference to the
SharedBuffer (m_encodedImageData), but the actual SharedBuffer will only get
freed (thus saving memory) if the caller (e.g. ImageResource) also drops its
reference?

Is it possible the Image will drop its reference, resulting in having to copy
from the SkRWBuffer, in cases where the SharedBuffer is still around, so we
could have skipped copying from the SkRWBuffer without holding onto extra
memory? If so, would it be possible to only reset it if we know that that the
ImageResource *also* dropped its reference? (Or does this CL already do that and
I'm missing something?)

[hajimehoshi, 2016/07/04 10:46:25]

Yes.
Hmm, I think it's possible but it'd be very hard since we'd need hack
Resource::m_data without breaking other resource classes (hiroshige, do you
agree?) If we need this, I want to do it in another CL.

+            DCHECK(!cachedData());
+        }
+    }
 
     m_haveFrameCount = false;
     return isSizeAvailable();
 }
 
 bool BitmapImage::hasColorProfile() const
 {
     return m_source.hasColorProfile();
 }
 
@@ skipping 357 matching lines @@
 
     // We need to draw this frame if we advanced to it while not skipping, or if
     // while trying to skip frames we hit the last frame and thus had to stop.
     if (skippingFrames != advancedAnimation)
         getImageObserver()->animationAdvanced(this);
 
     return advancedAnimation;
 }
 
 } // namespace blink