Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155973002:
Save a bitmap copy when advancing to dependent GIF and WebP animation frames (Closed)
|
Created:
4 years, 5 months ago by aleksandar.stojiljkovic Modified:
4 years, 2 months ago Reviewers:
Ken Russell (switch to Gerrit), cblume, Peter Kasting, reed2, dcheng, f(malita), scroggo_chromium, bsalomon, reed1 CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSave a copy when advancing to dependent GIF and WebP animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements for GIF file are available in the crbug.com/490895
BUG=490895, 425474
Committed: https://crrev.com/3a38c33c398189620281ad19a0f8e5b7f6ac3e69
Cr-Commit-Position: refs/heads/master@{#422125}
Patch Set 1 #
Total comments: 6
Patch Set 2 : nits #
Total comments: 1
Patch Set 3 : nit #Patch Set 4 : +webp. fix test failures. #
Total comments: 22
Patch Set 5 : finalizePixelsAndGetImage. #
Total comments: 25
Patch Set 6 : review #34 fixes. #Patch Set 7 : fixes. Remove the rest of external bitmap().setImmutable. #Patch Set 8 : DCHECK in ImageFrame::copy/take. #
Total comments: 18
Patch Set 9 : Rebase. #Patch Set 10 : Nits. Thanks pkasting@. #
Total comments: 4
Patch Set 11 : Remove unecessary color check in ImageBitmap. Thanks dcheng@. #
Total comments: 8
Patch Set 12 : DCHECK m_status in ImageFrame. Shortened commit message. Thanks kbr@. #Messages
Total messages: 69 (32 generated)
Description was changed from
==========
Save a copy when advancing to dependent frames in GIFImageDecoder
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP after refactoring, blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
to
==========
Save a copy when advancing to dependent frames in GIFImageDecoder
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP after refactoring, blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
aleksandar.stojiljkovic@intel.com changed reviewers: + pkasting@chromium.org, scroggo@chromium.org
Description was changed from
==========
Save a copy when advancing to dependent frames in GIFImageDecoder
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP after refactoring, blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
to
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP after refactoring, blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
Description was changed from
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP after refactoring, blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
to
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:96: // Takes over other's frame pixel data. Other becomes empty after this call. Nit: To match declaration above as closely as possible: // Moves the bitmap data from the provided frame to this one, leaving the // provided frame empty. void moveBitmapData(ImageFrame*); This also fixes a violation of http://google.github.io/styleguide/cppguide.html#Reference_Arguments . https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364: // the next frame would also use it. Nit: How about: The next frame will also use |prevBuffer| as its starting state, so we can't take over its image data as we do below. Copy the data instead. https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:368: // Use the required frame as the starting state for this frame. Nit: How about: This is the only frame to use |prevBuffer| as its starting state, and we'll always clear the old frame data after initializing this frame anyway, so we can save time by just moving its data over. Also, if the above is true, why is it true (that we will always clear)? Is it because of ImageFrameGenerator::decode() calling clearCacheExceptFrame()? If so why do we also need |m_purgeAggressively|? I'm confused about all this.
On 2016/07/19 03:04:17, Peter Kasting (slow) wrote: > https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): > > https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:96: // Takes over > other's frame pixel data. Other becomes empty after this call. > Nit: To match declaration above as closely as possible: > > // Moves the bitmap data from the provided frame to this one, leaving the > // provided frame empty. > void moveBitmapData(ImageFrame*); > > This also fixes a violation of > http://google.github.io/styleguide/cppguide.html#Reference_Arguments . > > https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp > (right): > > https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364: > // the next frame would also use it. > Nit: How about: > > The next frame will also use |prevBuffer| as its starting state, so we can't > take over its image data as we do below. Copy the data instead. > > https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:368: > // Use the required frame as the starting state for this frame. > Nit: How about: > > This is the only frame to use |prevBuffer| as its starting state, and we'll > always clear the old frame data after initializing this frame anyway, so we can > save time by just moving its data over. > > Also, if the above is true, why is it true (that we will always clear)? Is it > because of ImageFrameGenerator::decode() calling clearCacheExceptFrame()? If so > why do we also need |m_purgeAggressively|? I'm confused about all this. cc +cblume. It is true because of both - as discussed here [1] but after the patch [1] went in, it is good to call code under |m_purgeAggressively| always, not only for files with large resolutions. Note that |m_purgeAggressively| is used for cases when e.g. user scrolls away from animation and then scrolls back so that animation needs to catchup and decode many frames (Issue ) In a way, my patch here removes a need for patch [1] and purge aggressively approach, except for the case when there are frames with DisposeOverwritePrevious dependency. Since I don't know constraining that is, I left the purge aggressively code as it is. Anyway, cblume@ is going to refactor purge aggressively code while working on WebP support. [1] https://codereview.chromium.org/2075093002/#msg48 Another thing, verified that this patch here addresses the gifs in Issue 425474 too.
Description was changed from
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895
==========
to
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895, 425474
==========
https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:96: // Takes over other's frame pixel data. Other becomes empty after this call. On 2016/07/19 03:04:17, Peter Kasting (slow) wrote: > Nit: To match declaration above as closely as possible: > > // Moves the bitmap data from the provided frame to this one, leaving the > // provided frame empty. > void moveBitmapData(ImageFrame*); > > This also fixes a violation of > http://google.github.io/styleguide/cppguide.html#Reference_Arguments . Done. https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364: // the next frame would also use it. On 2016/07/19 03:04:17, Peter Kasting (slow) wrote: > Nit: How about: > > The next frame will also use |prevBuffer| as its starting state, so we can't > take over its image data as we do below. Copy the data instead. Done. https://codereview.chromium.org/2155973002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:368: // Use the required frame as the starting state for this frame. On 2016/07/19 03:04:17, Peter Kasting (slow) wrote: > Nit: How about: > > This is the only frame to use |prevBuffer| as its starting state, and we'll > always clear the old frame data after initializing this frame anyway, so we can > save time by just moving its data over. > > Also, if the above is true, why is it true (that we will always clear)? Is it > because of ImageFrameGenerator::decode() calling clearCacheExceptFrame()? If so > why do we also need |m_purgeAggressively|? I'm confused about all this. Done.
cblume: WDYT of this patch? It seems like with this, and if the bit about always purging after decoding is true, the only case where m_purgeAggressively does anything is the case where we were trying to save a frame because it was going to be needed as the starting point of the next frame after this one -- so purging it is going to result in needing to redecode from further back as soon as we want the next frame. Given that this variable is only set for large GIFs, this basically means it kicks in only in cases where that's going to result in a significant CPU hit. So I'm worried that maybe that member is flat harmful. It seems like the only case where it wouldn't be harmful is if we know we're going to be looping around and decoding an earlier frame than this one. But in that case, rather than m_purgeAggressively, it seems like we want some sort of function call that allows the decoder owner to just force its cache clear when it knows we'll loop around? I'm confused by all the seemingly-overlapping efforts here and I want to make sure we don't have different pieces of code trying to achieve the same things, or actively being counterproductive. Otherwise, this change LGTM, assuming my rationale below is correct. https://codereview.chromium.org/2155973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:371: // frame anyway, so we can save time by just moving its data over. Nit: I still think you should explicitly say what ensures we always clear the old frame data, so if that changes in the future it will be more clear. It doesn't seem like this is true because of m_purgeAggressively, since that itself is not always set. So it seems like this must be due to ImageFrameGenerator::decode() calling clearCacheExceptFrame(), or else your statement that this always happens is in fact incorrect.
aleksandar.stojiljkovic@intel.com changed reviewers: + cblume@chromium.org
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
aleksandar.stojiljkovic@intel.com changed reviewers: + reed@chromium.org
On 2016/07/28 10:24:07, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Problem with immutable pixel ref (we set it when decoding is complete) - it should be fine to reuse the bitmap's pixelref for other frame bitmap, avoid copy and make pixelref mutable again only if it SkPixelRef>>fRefCnt is one - that way we ensure that there is no other reference to it using ImageFrame::bitmap. I'll start work on skia support for this. cc: +reed@
reed@google.com changed reviewers: + bsalomon@google.com, fmalita@chromium.org, reed@google.com
Is it possible to decode (directly write) into a SkSurface instead? Surface already has a copy-on-write mechanism. That could eliminate the need to look at changing the pixelref contract.
On 2016/07/28 14:13:21, reed1 wrote: > Is it possible to decode (directly write) into a SkSurface instead? Surface > already has a copy-on-write mechanism. That could eliminate the need to look at > changing the pixelref contract. It seems problematic to use SkSurface instead of SkBitmap in ImageFrame. This we need to reconsider: a. Single frame images are decoded directly to discardable memory (SkResourceCache earlier and now replaced by SoftwareImagedecodecontroller [1]). On a related note, it is possible to extend this from single frame images to independent (that is frames that no other frame depends on) frames for animations, too. b. SkSurface_Raster uses skMallocPixelRef and doesn't seem to allow setting other allocator to (SkBitmap) SkSurface_Raster::fBitmap.[2] We use allocator to decode directly to discradable memory [3] as mentioned in a. c. ImageFrame::bitmap() API and the bitmap's immutable flag is already used on other places in blink. [4] Often, these bitmaps end up being used in SkBaseDevice::drawImageRect - would using SkSurface instead of SkBitmap bring some improvement there? [1] https://cs.chromium.org/chromium/src/cc/tiles/software_image_decode_controlle... [2] https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkSurface_Ras... [3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... [4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Ima... There is a way to do this without changing Skia - we could manage SkBitmap pixelref memory and allocator in Blink and that way avoid copy but it seemed to me like a useful feature to add it to Skia (to try to make bitmap mutable again should be fine if pixelref refcount is 1).
On 2016/07/28 14:03:44, aleksandar.stojiljkovic wrote: > Problem with immutable pixel ref (we set it when decoding is complete) - it > should be fine to reuse the bitmap's pixelref for other frame bitmap, avoid copy > and make pixelref mutable again only if it SkPixelRef>>fRefCnt is one - that way > we ensure that there is no other reference to it using ImageFrame::bitmap. Can you try to instrument the code and evaluate how effective this is going to be in practice? Between display item recordings and ImageDecodeController caching (unless they're purged super-aggressively), I wouldn't be surprised if the refcount is > 1 most of the time. On 2016/07/28 15:38:43, aleksandar.stojiljkovic wrote: > a. Single frame images are decoded directly to discardable memory > (SkResourceCache earlier and now replaced by SoftwareImagedecodecontroller [1]). AFAIK ImageDecodeController only sees and produces SkImages, not SkBitmaps (it wraps externally-managed DM as SkImages). I think Mike is suggesting switching only ImageFrame from a SkBitmap buffer to a SkSurface buffer. > b. SkSurface_Raster uses skMallocPixelRef and doesn't seem to allow setting > other allocator to (SkBitmap) SkSurface_Raster::fBitmap.[2] > We use allocator to decode directly to discradable memory [3] as mentioned in a. For the initial surface construction there's SkSurface::MakeRasterDirect which allows you to wrap externally allocated/managed memory. But makeImageSnapshot will probably need some work to support external allocators. > c. ImageFrame::bitmap() API and the bitmap's immutable flag is already used on > other places in blink. [4] That's mostly for historical reasons. Blink has been converted to the (then) new SkImage APIs about a year ago, but we stopped at the decoder boundary. There' no fundamental reason for that, and it would make sense for decoders to use SkSurface and produce SkImage frames (since that's what the rest of the pipeline uses anyway). > Often, these bitmaps end up being used in SkBaseDevice::drawImageRect - would > using SkSurface instead of SkBitmap bring some improvement there? (you would use SkImage tear-offs, rather that SkSurface directly) All Blink images are drawn using SkImage APIs (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph..., etc). We always wrap decoder SkBitmaps as SkImages when passing downstream, so producing SkImages would remove an unnecessary step while consolidating on the modern/preferred Skia API.
Thanks. I'll try SkSurface approach then. On 2016/07/28 16:34:00, f(malita) wrote: > On 2016/07/28 14:03:44, aleksandar.stojiljkovic wrote: > > Problem with immutable pixel ref (we set it when decoding is complete) - it > > should be fine to reuse the bitmap's pixelref for other frame bitmap, avoid > copy > > and make pixelref mutable again only if it SkPixelRef>>fRefCnt is one - that > way > > we ensure that there is no other reference to it using ImageFrame::bitmap. > > Can you try to instrument the code and evaluate how effective this is going to > be in practice? > > Between display item recordings and ImageDecodeController caching (unless > they're purged super-aggressively), I wouldn't be surprised if the refcount is > > 1 most of the time. I wasn't clear in original message - referring here to SkBitmaps in ImageDecoder::m_frameBufferCache which should have refcount = 1 for the target use case from ImageFrameGenerator decode.
Description was changed from
==========
Save a copy when advancing to dependent GIF animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895, 425474
==========
to
==========
Save a copy when advancing to dependent GIF and WebP animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895, 425474
==========
Patch Set 4 : +webp. fix test failures. I decided to not to do SkBitmap->SkSurface&SkImage porting in this patch as the fix implemented here should be the same with SkSurface (we would want to avoid copy, even it is copy-on-write). https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:136: const SkBitmap& ImageFrame::bitmap(bool setImmutableIfDone) pkasting@ The fix for the failing tests is simpler than anticipated: - ImageFrameGenerator is the only customer of ImageFrame::bitmap() in cases referred from the bugs listed here. ImageFrameGenerator doesn't increase ref count (it just calls getPixels) and it is not interested if bitmap is mutable or not... When decoder and ImageFrame are used by ImageFrameGenerator, there is no need to get the bitmap immutable flag set (to call setImmutable). The other clients of ImageFrame::bitmap() are kept the same - setImmutable is deferred and set when client makes a request to access bitmap via ImageFrame::bitmap(bool setImmutableIfDone = true).
https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:104: if (other->m_bitmap.isImmutable()) Nit: Just combine these conditionals https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: // decoding animations. See also takeBitmapDataIfWritable. As a reader, I'm wondering why we would ever want to mark the bitmap immutable. Why not just always leave it mutable? Perhaps this comment should motivate why we want to make it immutable when possible. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:140: const SkBitmap& bitmap(bool setImmutableIfDone = true); Nit: This is named like a simple accessor, but it can also have a side effect. This seems confusing. I feel like it would be better to separate these into an accessor and a function that makes the bitmap immutable, and have the latter called from the relevant places we want that to happen. Then it's clear who really needs this and why. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:362: // We try to reuse |prevBuffer| as starting state and avoid copy. See comments in WebP decoder, they also apply to this file https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:259: // to reuse |prevBuffer| as starting state and avoid copy. Nit: and avoid copy -> to avoid copying https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:263: if (!buffer.copyBitmapData(prevBuffer)) Nit: I'd just combine these two conditionals https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:132: ImageFrame* frame = decoder->frameBufferAtIndex(i); Why remove the consts here and on similar lines below? https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:208: void verifyFramesMatch(const char* webpFile, ImageFrame* a, ImageFrame* b) Why remove the first const here? (The other two are fine to remove)
https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:104: if (other->m_bitmap.isImmutable()) On 2016/08/26 19:04:04, Peter Kasting wrote: > Nit: Just combine these conditionals Done. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: // decoding animations. See also takeBitmapDataIfWritable. On 2016/08/26 19:04:04, Peter Kasting wrote: > As a reader, I'm wondering why we would ever want to mark the bitmap immutable. > Why not just always leave it mutable? Perhaps this comment should motivate why > we want to make it immutable when possible. It is convenient that bitmap carry information about "is decoding done" through the layers of API, to help on optimizing caching and render quality. Originally added as replacement (patch [1]) of "is Complete" API now we are taking it a step back for one of the clients using isComplete through ImageFrame::getStatus(). Related patch (from 2012) [1] https://chromium.googlesource.com/chromium/src/+/5943ea57ef97970aa46651e033c9... Also, as SkImage is used around the blink, and SkImage is immutable snapshot, creating SkImage from SkBitmap when SkBitmap isn't immutable creates a copy: https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkImage_Raste... https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:140: const SkBitmap& bitmap(bool setImmutableIfDone = true); On 2016/08/26 19:04:04, Peter Kasting wrote: > Nit: This is named like a simple accessor, but it can also have a side effect. > This seems confusing. > > I feel like it would be better to separate these into an accessor and a function > that makes the bitmap immutable, It would be clear if we have additional accessor that returns SkImage. It gets called in places where immutable SkBitmap is expected and conversion to SkImage follows. SkImage is expected to be immutable snapshot so we choose to set ImageFrame::skbitmap to const internally before creating SkImage to avoid copy within SkImage::MakeFromBitmap. So, something like: const SkBitmap& bitmap() const // doesnt set immutable const SkImage& image() // sets to immutable before creating SkImage from SkBitmap. I'll add it in next patch. > and have the latter called from the relevant > places we want that to happen. Then it's clear who really needs this and why. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:362: // We try to reuse |prevBuffer| as starting state and avoid copy. On 2016/08/26 19:04:04, Peter Kasting wrote: > See comments in WebP decoder, they also apply to this file Done. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:259: // to reuse |prevBuffer| as starting state and avoid copy. On 2016/08/26 19:04:04, Peter Kasting wrote: > Nit: and avoid copy -> to avoid copying Done. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:263: if (!buffer.copyBitmapData(prevBuffer)) On 2016/08/26 19:04:04, Peter Kasting wrote: > Nit: I'd just combine these two conditionals Done. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:132: ImageFrame* frame = decoder->frameBufferAtIndex(i); On 2016/08/26 19:04:05, Peter Kasting wrote: > Why remove the consts here and on similar lines below? To make it consistent with other places (GIFImageDecoderTest and ImageDecoderTestHelpers).
https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: // decoding animations. See also takeBitmapDataIfWritable. On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > On 2016/08/26 19:04:04, Peter Kasting wrote: > > As a reader, I'm wondering why we would ever want to mark the bitmap > immutable. > > Why not just always leave it mutable? Perhaps this comment should motivate > why > > we want to make it immutable when possible. > > It is convenient that bitmap carry information about "is decoding done" through > the layers of API, to help on optimizing caching and render quality. > Originally added as replacement (patch [1]) of "is Complete" API now we are > taking it a step back for one of the clients using isComplete through > ImageFrame::getStatus(). What does "taking it a step back for one of the clients using isComplete through ImageFrame::getStatus()" mean? I don't understand that sentence. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:140: const SkBitmap& bitmap(bool setImmutableIfDone = true); On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > On 2016/08/26 19:04:04, Peter Kasting wrote: > > Nit: This is named like a simple accessor, but it can also have a side effect. > > > This seems confusing. > > > > I feel like it would be better to separate these into an accessor and a > function > > that makes the bitmap immutable, > > It would be clear if we have additional accessor that returns SkImage. It gets > called in places where immutable SkBitmap is expected and conversion to SkImage > follows. SkImage is expected to be immutable snapshot so we choose to set > ImageFrame::skbitmap to const internally before creating SkImage to avoid copy > within SkImage::MakeFromBitmap. > > So, something like: > const SkBitmap& bitmap() const // doesnt set immutable > const SkImage& image() // sets to immutable before creating SkImage from > SkBitmap. > > I'll add it in next patch. That second function still looks like a cheap accessor but actually isn't. At the very least, I'd name the second function something like "finalizePixelsAndGetImage()" and check in its implementation that the frame status is already complete. But I still think it might be clearer to just do something like: const SkBitmap& bitmap() const; void SetImmutable(); ...and have callers that want both things to happen call both. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:132: ImageFrame* frame = decoder->frameBufferAtIndex(i); On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > On 2016/08/26 19:04:05, Peter Kasting wrote: > > Why remove the consts here and on similar lines below? > > To make it consistent with other places (GIFImageDecoderTest and > ImageDecoderTestHelpers). Don't worry about that kind of consistency, especially when it's achieved by removing perfectly valid consts. If we want to make any change, it should probably be to add const those other places.
https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: // decoding animations. See also takeBitmapDataIfWritable. On 2016/08/27 04:19:58, Peter Kasting wrote: > On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > > On 2016/08/26 19:04:04, Peter Kasting wrote: > > > As a reader, I'm wondering why we would ever want to mark the bitmap > > immutable. > > > Why not just always leave it mutable? Perhaps this comment should motivate > > why > > > we want to make it immutable when possible. > > > > It is convenient that bitmap carry information about "is decoding done" > through > > the layers of API, to help on optimizing caching and render quality. > > Originally added as replacement (patch [1]) of "is Complete" API now we are > > taking it a step back for one of the clients using isComplete through > > ImageFrame::getStatus(). > > What does "taking it a step back for one of the clients using isComplete through > ImageFrame::getStatus()" mean? I don't understand that sentence. That sentence could be ignored - it just means that both getStatus() == FrameComplete and isImmutable are used for the same thing and we can use only getStatus() == FrameComplete. https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:140: const SkBitmap& bitmap(bool setImmutableIfDone = true); On 2016/08/27 04:19:58, Peter Kasting wrote: > On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > > On 2016/08/26 19:04:04, Peter Kasting wrote: > > > Nit: This is named like a simple accessor, but it can also have a side > effect. > > > > > This seems confusing. > > > > > > I feel like it would be better to separate these into an accessor and a > > function > > > that makes the bitmap immutable, > > > > It would be clear if we have additional accessor that returns SkImage. It gets > > called in places where immutable SkBitmap is expected and conversion to > SkImage > > follows. SkImage is expected to be immutable snapshot so we choose to set > > ImageFrame::skbitmap to const internally before creating SkImage to avoid copy > > within SkImage::MakeFromBitmap. > > > > So, something like: > > const SkBitmap& bitmap() const // doesnt set immutable > > const SkImage& image() // sets to immutable before creating SkImage from > > SkBitmap. > > > > I'll add it in next patch. > > That second function still looks like a cheap accessor but actually isn't. > > At the very least, I'd name the second function something like > "finalizePixelsAndGetImage()" and check in its implementation that the frame > status is already complete. > > But I still think it might be clearer to just do something like: > > const SkBitmap& bitmap() const; > void SetImmutable(); > > ...and have callers that want both things to happen call both. Done. bitmap() is back to original "cheap accessor" constness. Added finalizePixelsAndGetImage. Didn't add setImmutable since it can be done simply by calling: SkBitmap bitmap = frame->getBitmap(); bitmap.setImmutable(); https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/2155973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:132: ImageFrame* frame = decoder->frameBufferAtIndex(i); On 2016/08/27 04:19:58, Peter Kasting wrote: > On 2016/08/26 21:53:51, aleksandar.stojiljkovic wrote: > > On 2016/08/26 19:04:05, Peter Kasting wrote: > > > Why remove the consts here and on similar lines below? > > > > To make it consistent with other places (GIFImageDecoderTest and > > ImageDecoderTestHelpers). > > Don't worry about that kind of consistency, especially when it's achieved by > removing perfectly valid consts. If we want to make any change, it should > probably be to add const those other places. Done, bitmap() const reverted back .
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:177: DCHECK_EQ(frame->bitmap().colorType(), kN32_SkColorType); The old code handled this conditionally, so this is a behavior change. Is this safe? Nit: (expected, actual) https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:98: DCHECK(decoder->failed() || imageFrame->getStatus() == ImageFrame::FrameComplete); This check isn't safe if somehow failed() is false but |imageFrame| is null. If that should never happen, I'd change this to: if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) { ... } else { DCHECK(decoder->failed()); } https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101: bitmap.setImmutable(); Why is setting the bitmap as immutable here important? At the very least this needs a comment. This question basically applies to everywhere in this CL that does this. I was under the impression only people getting an SkImage needed to do this (to avoid a copy), but we seem to be doing it many other places. If this is because we're passing the bitmap to places that will turn it into an image, can we change to passing an SkImage now? https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2148: // allow ImageFrame::finalizePixelsAndGetImage to make a copy. Isn't it already allowed to make a copy? Does this comment mean something like "We could potentially relax this if we decided it was OK for the conversion from bitmap to image to make a copy in this case."? https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2154: DCHECK_EQ(bitmap.colorType(), kN32_SkColorType); Same comment as in ImageBitmap.cpp. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if (other->m_bitmap.isImmutable() || this == other) Tiny efficiency nit: Reverse order of checks https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:139: // next animation frame by calling takeBitmapDataIfWritable. Nit: The header comments should already justify any functional difference between these two, so I'd remove this comment and just inline the method into the header. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:103: // marked as done (immutable). Returns whether the move succeeded. Nit: Maybe better second sentence: "This fails if the provided frame's bitmap is marked immutable, since moving its data out from under it destructively modifies it." https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:141: PassRefPtr<SkImage> finalizePixelsAndGetImage(); Given that this doesn't always finalize the pixels (if decoding isn't complete), maybe combining these two pieces of functionality isn't a good idea. It's convenient, but it's less clear than I thought it would be since I had thought we were only calling this when the frame was complete, and would thus finalize unconditionally. It looks like all but one caller knows the frame is complete already. So we could make these do something like: frame->finalizePixels(); // or ->markBitmapImmutable(), if you want to be more literal frame->image(); And that one put the finalizePixels() call under a conditional. Then it's clear when callers are finalizing vs. not. finalizePixels() could DCHECK that the frame is complete, so people don't accidentally call it for incomplete frames, which would be bad. It would be useful for the places that are currently doing this manually (by getting the bitmap and calling setImmutable() on it) as well, assuming we want to keep those, because it's a bit briefer and clearer than that code (and it has the DCHECK() guard). Finally we could put a comment on the declaration of finalizePixels() that this results in saving a copy when image() is used, but disables passing this frame to takeBitmapDataIfWriteable(), so callers need to decide carefully what will result in best performance. (It would be nice to know, in practice, what sorts of sequences of calls can result in these optimization cases overlapping.) Then we could put a comment on image() that calling it when things are not finalized incurs a copy. If you wanted, this could even be split into two functions, e.g. finalizedImage() and nonFinalizedImage() (or image() and copyImage(), or something), each of which would simply DCHECK whether or not the bitmap was immutable. This would force callers to be explicit about which one they thought they were doing, so as to avoid unintentional copies.
Patch Set 6 : review #34 fixes. pkasting@, thanks and sorry for the delay on this. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:177: DCHECK_EQ(frame->bitmap().colorType(), kN32_SkColorType); On 2016/08/30 06:59:40, Peter Kasting wrote: > The old code handled this conditionally, so this is a behavior change. Is this > safe? > > Nit: (expected, actual) Done. Didn't see how it could be different from kN32 but agree - no need to introduce behavior change. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:98: DCHECK(decoder->failed() || imageFrame->getStatus() == ImageFrame::FrameComplete); On 2016/08/30 06:59:40, Peter Kasting wrote: > This check isn't safe if somehow failed() is false but |imageFrame| is null. > > If that should never happen, I'd change this to: > > if (imageFrame && imageFrame->getStatus() == ImageFrame::FrameComplete) { > ... > } else { > DCHECK(decoder->failed()); > } Done. Reverted. As allDataReceived is set to true it is expected that either it is complete or failed. Didn't see in code how it could be null. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101: bitmap.setImmutable(); On 2016/08/30 06:59:40, Peter Kasting wrote: > Why is setting the bitmap as immutable here important? At the very least this > needs a comment. > > This question basically applies to everywhere in this CL that does this. I was > under the impression only people getting an SkImage needed to do this (to avoid > a copy), but we seem to be doing it many other places. > > If this is because we're passing the bitmap to places that will turn it into an > image, can we change to passing an SkImage now? Reverted this to original implementation - in ImageSkiaRep::ImageSkiaRep(const SkBitmap& src, float scale) where this code leads to, bitmap is set to immutable. [1] https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744... https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101: bitmap.setImmutable(); On 2016/08/30 06:59:40, Peter Kasting wrote: > Why is setting the bitmap as immutable here important? At the very least this > needs a comment. > > This question basically applies to everywhere in this CL that does this. I was > under the impression only people getting an SkImage needed to do this (to avoid > a copy), but we seem to be doing it many other places. > > If this is because we're passing the bitmap to places that will turn it into an > image, can we change to passing an SkImage now? That was my plan too, with all the cases where SkBitmap was ported to SkImage. After this patch, only remaining case where we do this is with WebImage(SkBitmap) construction: https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIm... and there is WebImage::getSkBitmap API that would need to be replaced with SkImage variant. The reason I left the SkBitmap/setImmutable used here is that it is WebKit public/platform API and don't know the deprecation policy here / if anybody else outside chromium codebase is using it. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:74: *result = frame->bitmap(); Original code seems to be used only by devtools. After this patch, it should be fine to remove that skia legacy code under: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPicture.cpp?... #ifdef SK_SUPPORT_LEGACY_PICTUREINSTALLPIXELREF class InstallProcImageDeserializer : public SkImageDeserializer { ... Drawback: now we have unnecessary path here only - toSkSp(fromSkSp()) - this is the price for not having fromSkSp(frame->finalizePixelsAndGetImage()) everywhere else. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2148: // allow ImageFrame::finalizePixelsAndGetImage to make a copy. On 2016/08/30 06:59:40, Peter Kasting wrote: > Isn't it already allowed to make a copy? Does this comment mean something like > "We could potentially relax this if we decided it was OK for the conversion from > bitmap to image to make a copy in this case."? By early return if "frame->getStatus() != ImageFrame::FrameComplete" we ensure that there is no copy done by finalizePixelsAndGetImage. This means that if don't want to prevent copy by finalizePixelsAndGetImage, we can relax the check to e.g.: if (!frame) return; https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2154: DCHECK_EQ(bitmap.colorType(), kN32_SkColorType); On 2016/08/30 06:59:40, Peter Kasting wrote: > Same comment as in ImageBitmap.cpp. Done. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if (other->m_bitmap.isImmutable() || this == other) On 2016/08/30 06:59:40, Peter Kasting wrote: > Tiny efficiency nit: Reverse order of checks It isn't obvious when/if this could happen (this == other) so while debugging most common use cases I found that the current order is leaving the check earlier. "this == other" I copied from copyBitmapData. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:139: // next animation frame by calling takeBitmapDataIfWritable. On 2016/08/30 06:59:40, Peter Kasting wrote: > Nit: The header comments should already justify any functional difference > between these two, so I'd remove this comment and just inline the method into > the header. Done. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:103: // marked as done (immutable). Returns whether the move succeeded. On 2016/08/30 06:59:40, Peter Kasting wrote: > Nit: Maybe better second sentence: "This fails if the provided frame's bitmap is > marked immutable, since moving its data out from under it destructively modifies > it." The issue here is not in destructive modification (we don't need that frame data) but if it is done we cannot write to it when decoding the next frame. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:141: PassRefPtr<SkImage> finalizePixelsAndGetImage(); On 2016/08/30 06:59:40, Peter Kasting wrote: > Given that this doesn't always finalize the pixels (if decoding isn't complete), > maybe combining these two pieces of functionality isn't a good idea. It's > convenient, but it's less clear than I thought it would be since I had thought > we were only calling this when the frame was complete, and would thus finalize > unconditionally. > > It looks like all but one caller knows the frame is complete already. So we > could make these do something like: > > frame->finalizePixels(); // or ->markBitmapImmutable(), if you want to be > more literal > frame->image(); > > And that one put the finalizePixels() call under a conditional. Then it's clear > when callers are finalizing vs. not. > > finalizePixels() could DCHECK that the frame is complete, so people don't > accidentally call it for incomplete frames, which would be bad. It would be > useful for the places that are currently doing this manually (by getting the > bitmap and calling setImmutable() on it) as well, assuming we want to keep > those, because it's a bit briefer and clearer than that code (and it has the > DCHECK() guard). Finally we could put a comment on the declaration of > finalizePixels() that this results in saving a copy when image() is used, but > disables passing this frame to takeBitmapDataIfWriteable(), so callers need to > decide carefully what will result in best performance. (It would be nice to > know, in practice, what sorts of sequences of calls can result in these > optimization cases overlapping.) > > Then we could put a comment on image() that calling it when things are not > finalized incurs a copy. If you wanted, this could even be split into two > functions, e.g. finalizedImage() and nonFinalizedImage() (or image() and > copyImage(), or something), each of which would simply DCHECK whether or not the > bitmap was immutable. This would force callers to be explicit about which one > they thought they were doing, so as to avoid unintentional copies. I like finalizePixelsAndGetImage as it explains what it is doing (and as you said, the only problem is the condition inside). Handled that one caller separately, updated the comment here and added DCHECK as you suggested.
Have not re-reviewed https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101: bitmap.setImmutable(); On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote: > The reason I left the SkBitmap/setImmutable used here is that it is WebKit > public/platform API and don't know the deprecation policy here / if anybody else > outside chromium codebase is using it. Blink is Chrome's fork, so we assume that we only want things Chrome is using. So you should always go ahead and rip out things that you see Chromium code no longer needing. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if (other->m_bitmap.isImmutable() || this == other) On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote: > On 2016/08/30 06:59:40, Peter Kasting wrote: > > Tiny efficiency nit: Reverse order of checks > > It isn't obvious when/if this could happen (this == other) so while debugging > most common use cases I found that the current order is leaving the check > earlier. > > "this == other" I copied from copyBitmapData. I suppose you could make this (and maybe that function also?) DCHECK that this != other, if it never makes sense for that to happen (which it probably doesn't).
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:101: bitmap.setImmutable(); On 2016/09/21 21:54:49, Peter Kasting wrote: > On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote: > > The reason I left the SkBitmap/setImmutable used here is that it is WebKit > > public/platform API and don't know the deprecation policy here / if anybody > else > > outside chromium codebase is using it. > > Blink is Chrome's fork, so we assume that we only want things Chrome is using. > > So you should always go ahead and rip out things that you see Chromium code no > longer needing. Checked in details - a lot of layout tests are using WebImage and the code needs SkBitmap for direct access to pixels - so I left SkBitmap but there in the WebImage::getSkBitmap API, and seems to be no need to call setImmutable - on rare places where it is converted to SkImage it is already happening on client side of the call through [1] https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=14744... https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:74: *result = frame->bitmap(); On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote: > Original code seems to be used only by devtools. > After this patch, it should be fine to remove that skia legacy code under: > > https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPicture.cpp?... > > #ifdef SK_SUPPORT_LEGACY_PICTUREINSTALLPIXELREF > class InstallProcImageDeserializer : public SkImageDeserializer { > ... > > Drawback: now we have unnecessary path here only - toSkSp(fromSkSp()) - this is > the price for not having fromSkSp(frame->finalizePixelsAndGetImage()) everywhere > else. Done. Removed the drawback. https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:103: if (other->m_bitmap.isImmutable() || this == other) On 2016/09/21 21:54:49, Peter Kasting wrote: > On 2016/09/21 20:56:57, aleksandar.stojiljkovic wrote: > > On 2016/08/30 06:59:40, Peter Kasting wrote: > > > Tiny efficiency nit: Reverse order of checks > > > > It isn't obvious when/if this could happen (this == other) so while debugging > > most common use cases I found that the current order is leaving the check > > earlier. > > > > "this == other" I copied from copyBitmapData. > > I suppose you could make this (and maybe that function also?) DCHECK that this > != other, if it never makes sense for that to happen (which it probably > doesn't). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
LGTM https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:178: if (frame->bitmap().colorType() != kN32_SkColorType) So, what I meant by "is this safe" was not so much "leave this as a conditional" but simply "can you justify that this does or does not happen"? It sounded like you don't think this can ever fail, so maybe we _should_ DCHECK it. An intentional behavior change is OK, I just didn't want an accidental one. If we don't DCHECK it, it deserves a comment about when it can fail. Another question to ask is "do we even care"? Why bother checking the color type at all? Can't SkImage handle other color types anyway? I guess I don't really know what the code is trying to do. Applies in the other place that checks the color type too. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:180: return fromSkSp(frame->finalizePixelsAndGetImage()); Nit: Shorter: return (frame->bitmap().colorType() == kN32_SkColorType) ? fromSkSp(frame->finalizePixelsAndGetImage()) : nullptr; https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebImage.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebImage.cpp:84: return WebImage(frame->bitmap()); Nit: Shorter: return (frame && !decoder->failed()) ? WebImage(frame->bitmap()) : WebImage(); https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebImage.cpp:118: if (!bitmap.isNull() && frame->getStatus() == ImageFrame::FrameComplete) { Nit: No {} https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122: return fromSkSp(SkImage::MakeFromBitmap(frame->bitmap())); Nit: Avoids duplicating fromSkSp() call: return fromSkSp((frame->getStatus() == ImageFrame::FrameComplete) ? frame->finalizePixelsAndGetImage() : SkImage::MakeFromBitmap(frame->bitmap())); https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:76: return frame->finalizePixelsAndGetImage(); Nit: Shorter: return (frame && !imageDecoder->failed()) ? frame->finalizePixelsAndGetImage() : nullptr; https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:136: DCHECK(m_status == FrameComplete); Nit: DCHECK_EQ(FrameComplete, m_status) https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:140: // avoid copying in SkImage::MakeFromBitmap. Nit: Second sentence is already in the header and can be eliminated. Not really sure what the first sentence is saying. Are you justifying why setStatus() does not mark the bitmap as immutable? If so, move this comment there and say something like this instead: An immutable bitmap would mean callers could construct an SkImage from this without a copy. However, setting the bitmap immutable here would defeat takeBitmapDataIfWritable(). Instead we let the bitmap stay mutable until someone calls finalizePixelsAndGetImage() to actually get the SkImage. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:141: if (!m_bitmap.isImmutable()) Nit: Conditional check unnecessary
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patch 9: Rebase. Path 10: Nits. Thanks pkasting@. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:178: if (frame->bitmap().colorType() != kN32_SkColorType) On 2016/09/22 21:44:08, Peter Kasting wrote: > So, what I meant by "is this safe" was not so much "leave this as a conditional" > but simply "can you justify that this does or does not happen"? > > It sounded like you don't think this can ever fail, so maybe we _should_ DCHECK > it. An intentional behavior change is OK, I just didn't want an accidental one. > > If we don't DCHECK it, it deserves a comment about when it can fail. > > Another question to ask is "do we even care"? Why bother checking the color > type at all? Can't SkImage handle other color types anyway? I guess I don't > really know what the code is trying to do. > > Applies in the other place that checks the color type too. I didn't see how it could happen with current set of decoders in public Chromium base. Reverted it back because, after reducing workload I plan to resume work on Index8 and RGB565 (for Android when target surface is 565) and then to handle this constraints. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:180: return fromSkSp(frame->finalizePixelsAndGetImage()); On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Shorter: > > return (frame->bitmap().colorType() == kN32_SkColorType) ? > fromSkSp(frame->finalizePixelsAndGetImage()) : nullptr; Done. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebImage.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebImage.cpp:84: return WebImage(frame->bitmap()); On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Shorter: > > return (frame && !decoder->failed()) ? WebImage(frame->bitmap()) : WebImage(); Done. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebImage.cpp:118: if (!bitmap.isNull() && frame->getStatus() == ImageFrame::FrameComplete) { On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122: return fromSkSp(SkImage::MakeFromBitmap(frame->bitmap())); On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Avoids duplicating fromSkSp() call: > > return fromSkSp((frame->getStatus() == ImageFrame::FrameComplete) > ? frame->finalizePixelsAndGetImage() > : SkImage::MakeFromBitmap(frame->bitmap())); Done. The most recent refactoring removed the need for fromSkSp meanwhile. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:76: return frame->finalizePixelsAndGetImage(); On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Shorter: > > return (frame && !imageDecoder->failed()) ? > frame->finalizePixelsAndGetImage() : nullptr; Done. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:136: DCHECK(m_status == FrameComplete); On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: DCHECK_EQ(FrameComplete, m_status) Done. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:140: // avoid copying in SkImage::MakeFromBitmap. On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Second sentence is already in the header and can be eliminated. Not really > sure what the first sentence is saying. Are you justifying why setStatus() does > not mark the bitmap as immutable? If so, move this comment there and say > something like this instead: > > An immutable bitmap would mean callers could construct an SkImage from this > without a copy. However, setting the bitmap immutable here would defeat > takeBitmapDataIfWritable(). Instead we let the bitmap stay mutable until > someone calls finalizePixelsAndGetImage() to actually get the SkImage. Done. Rephrased it, removed the note about copy as it is in the header and joined it with the comment in setStatus. https://codereview.chromium.org/2155973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:141: if (!m_bitmap.isImmutable()) On 2016/09/22 21:44:08, Peter Kasting wrote: > Nit: Conditional check unnecessary Done.
Further owner's review needed. fmalita@ (changes in platform/graphics and platform/exported) and dcheng@ (change in core/frame/ImageBitmap.cpp), could you please review? Thanks,
aleksandar.stojiljkovic@intel.com changed reviewers: + dcheng@chromium.org
+dcheng@ - reviewer for core/frame/ImageBitmap.cpp change. Thanks.
https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return (frame->bitmap().colorType() == kN32_SkColorType) ? frame->finalizePixelsAndGetImage() : nullptr; Just curious: what does that mean if we get here and the color type doesn't match what we expect? Does that mean an error of some sort occurred earlier in the decoding pipeline?
https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return (frame->bitmap().colorType() == kN32_SkColorType) ? frame->finalizePixelsAndGetImage() : nullptr; On 2016/09/27 22:44:15, dcheng wrote: > Just curious: what does that mean if we get here and the color type doesn't > match what we expect? Does that mean an error of some sort occurred earlier in > the decoding pipeline? kUnknown_SkColorType: In initial state, SkBitmap has kUnknown_SkColorType. This happens on error but also when file is partially received, the frame data is not yet received and the frame not initialized. In this case ASSERT/DCHECK(!isNull && !isEmpty) is covering the same as the condition. some other bitmap type: Not an error but someone implements a decoder producing different bitmap type. Don't think there is such case now - I'm working on a patch of such kind for chromium [1]. Don't know if there are decoders in different chromium flavors doing it (probably no need to care about it). [1] https://codereview.chromium.org/1460523002/ Otherwise, the change in this file is refactoring only. We're not using isImmutable but it had the same meaning as getStatus() == ImageFrame::FrameComplete.
https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return (frame->bitmap().colorType() == kN32_SkColorType) ? frame->finalizePixelsAndGetImage() : nullptr; On 2016/09/28 18:51:24, aleksandar.stojiljkovic wrote: > On 2016/09/27 22:44:15, dcheng wrote: > > Just curious: what does that mean if we get here and the color type doesn't > > match what we expect? Does that mean an error of some sort occurred earlier in > > the decoding pipeline? > > kUnknown_SkColorType: > In initial state, SkBitmap has kUnknown_SkColorType. This happens on error but > also when file is partially received, the frame data is not yet received and the > frame not initialized. > In this case ASSERT/DCHECK(!isNull && !isEmpty) is covering the same as the > condition. > > some other bitmap type: > Not an error but someone implements a decoder producing different bitmap type. > Don't think there is such case now - I'm working on a patch of such kind for > chromium [1]. Don't know if there are decoders in different chromium flavors > doing it (probably no need to care about it). > [1] > https://codereview.chromium.org/1460523002/ > > > Otherwise, the change in this file is refactoring only. We're not using > isImmutable but it had the same meaning as getStatus() == > ImageFrame::FrameComplete. I understand that this is a refactoring only change, but I don't really understand it either. Is it possible to add a comment about why we'd only return an SkImage if the color type matches exactly? How is it handled if it doesn't match? Do we just not return an image at all? That doesn't seem completely right either? (I'm sure this makes more sense to someone who's already familiar with the decoding pipeline, but I'm not, and so more comments seem like they'd be useful here)
Patch Set 11 : Remove unecessary color check in ImageBitmap. Thanks dcheng@. https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2155973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:213: return (frame->bitmap().colorType() == kN32_SkColorType) ? frame->finalizePixelsAndGetImage() : nullptr; On 2016/09/28 21:16:52, dcheng wrote: > On 2016/09/28 18:51:24, aleksandar.stojiljkovic wrote: > > On 2016/09/27 22:44:15, dcheng wrote: > > > Just curious: what does that mean if we get here and the color type doesn't > > > match what we expect? Does that mean an error of some sort occurred earlier > in > > > the decoding pipeline? > > > > kUnknown_SkColorType: > > In initial state, SkBitmap has kUnknown_SkColorType. This happens on error but > > also when file is partially received, the frame data is not yet received and > the > > frame not initialized. > > In this case ASSERT/DCHECK(!isNull && !isEmpty) is covering the same as the > > condition. > > > > some other bitmap type: > > Not an error but someone implements a decoder producing different bitmap type. > > Don't think there is such case now - I'm working on a patch of such kind for > > chromium [1]. Don't know if there are decoders in different chromium flavors > > doing it (probably no need to care about it). > > [1] > > https://codereview.chromium.org/1460523002/ > > > > > > Otherwise, the change in this file is refactoring only. We're not using > > isImmutable but it had the same meaning as getStatus() == > > ImageFrame::FrameComplete. > > I understand that this is a refactoring only change, but I don't really > understand it either. Is it possible to add a comment about why we'd only return > an SkImage if the color type matches exactly? How is it handled if it doesn't > match? Do we just not return an image at all? That doesn't seem completely right > either? > > (I'm sure this makes more sense to someone who's already familiar with the > decoding pipeline, but I'm not, and so more comments seem like they'd be useful > here) Yes, given the explanation I provided above, there is really no sense in doing the color check. Removed it. There are no decoders that produce other color types than N32. If the frame's bitmap is empty (colortype kUnknown_SkColorType) but frame marked as complete which cannot happen now, but let's open possibility for future, it is still perfectly fine to call SkImage::MakeFromBitmap(m_bitmap) on empty m_bitmap - it returns null. So, no need to check color type - all cases are handled. Related to your other concern, returning nullptr from this method is correct and expected in case of error. All the clients of the call are handling it.
LGTM
aleksandar.stojiljkovic@intel.com changed reviewers: + kbr@chromium.org
The patch still needs platform/graphics OWNER's review. CC-ing kbr@ in case fmalita@ is busy/away. Thanks.
A few questions. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:107: other->m_status = FrameEmpty; What about all of the other fields of this ImageFrame, like m_status? Are you sure this ImageFrame object will be in a consistent state after swapping m_bitmap with other->m_bitmap? Or, if the invariant is that m_status on both sides must be FrameComplete for this to run, what about either DCHECKs for that, or explicit tests and returning false if not? https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:366: if ((buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious || !buffer->takeBitmapDataIfWritable(prevBuffer)) && !buffer->copyBitmapData(*prevBuffer)) This is a lot of logic for a single if-statement. It's also not clear to me that if buffer->takeBitmapDataIfWritable() succeeds, that we should run the logic below, since presumably prevBuffer is already emptied out. Same comment in the WEBP image decoder. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:374: buffer->zeroFillFrameRect(prevRect); I don't understand why this preexisting code is calling this against buffer instead of prevBuffer. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:262: if ((buffer.getAlphaBlendSource() == ImageFrame::BlendAtopPreviousFrame || !buffer.takeBitmapDataIfWritable(&prevBuffer)) && !buffer.copyBitmapData(prevBuffer)) Same comment. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:270: buffer.zeroFillFrameRect(prevRect); Same comment.
Also: the description for this CL is very long. Could you put most of the text (in particular, the patch used for measurement, and maybe more of it) into one of the associated bugs and link to it instead? Thanks.
Description was changed from
==========
Save a copy when advancing to dependent GIF and WebP animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements: For GIF file in the crbug.com/490895
- Average per frame decoding time was 19.9ms.
- After this patch, average frame decoding is 17.6ms.
Measured using this patch and then out\Release_x64\chrome --enable-logging --v=1
on Windows 10, Intel (R) Core(TM) i7-4770HQ CPU @ 2.20GHz 2.20 GHz
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -30,6 +30,10 @@
#include "wtf/PtrUtil.h"
#include <limits>
+ // TODO(astojilj): Remove this - just for measurement.
+#include "base/time/time.h"
+#include "base/timer/elapsed_timer.h"
+
namespace blink {
GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
@@ -312,11 +316,20 @@ void GIFImageDecoder::decode(size_t index)
} while (frameToDecode != kNotFound &&
m_frameBufferCache[frameToDecode].getStatus() != ImageFrame::FrameComplete);
for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ base::ElapsedTimer timer;
if (!m_reader->decode(*i)) {
setFailed();
return;
}
+ // TODO(astojilj) Temporary code is used to measure elapsed time.
+ // printf bellow is used only to avoid getting microseconds optimized
away in
+ // Release_x64 build - set a breakpoint to the line with printf to get
the
+ // value.
+ uint64_t microseconds = timer.Elapsed().InMicroseconds();
+ LOG(WARNING) << "decoding index:" << index << " decodes:" << *i << " :
" << microseconds;
+
if (m_purgeAggressively)
clearCacheExceptFrame(*i);
Disclaimer: Same could be applied to WebP only after refactoring - blending
implementation in WebPImageDecoder::applyPostProcessing asumes that both
previous and current buffer exist.
BUG=490895, 425474
==========
to
==========
Save a copy when advancing to dependent GIF and WebP animation frames
As clearCacheExceptFrame anyhow clears dependent frame, this is causing no
change in cache content: instead of create new, copy and clear old, we do:
swap content of old and empty new.
Measurements for GIF file are available in the crbug.com/490895
BUG=490895, 425474
==========
Patch Set 12 : DCHECK m_status in ImageFrame. Shortened commit message. Thanks kbr@. On 2016/09/29 00:41:38, Ken Russell wrote: > Also: the description for this CL is very long. Could you put most of the text > (in particular, the patch used for measurement, and maybe more of it) into one > of the associated bugs and link to it instead? Thanks. Done. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right): https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:107: other->m_status = FrameEmpty; On 2016/09/29 00:40:04, Ken Russell wrote: > What about all of the other fields of this ImageFrame, like m_status? Are you > sure this ImageFrame object will be in a consistent state after swapping > m_bitmap with other->m_bitmap? For both clients, GIF and WebP right after this call (or copyBitmapData variant) m_status is set to FramePartial (m_status before this call is FrameEmpty). > Or, if the invariant is that m_status on both sides must be FrameComplete for > this to run, what about either DCHECKs for that, or explicit tests and returning > false if not? Added DCHECKs: other is always in FrameComplete status, this frame m_status is FrameEmpty - both of the conditions are ensured on client side before the call but adding them here too should help clarifying and avoid misuse later. Thanks. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:366: if ((buffer->getDisposalMethod() == ImageFrame::DisposeOverwritePrevious || !buffer->takeBitmapDataIfWritable(prevBuffer)) && !buffer->copyBitmapData(*prevBuffer)) On 2016/09/29 00:40:04, Ken Russell wrote: > This is a lot of logic for a single if-statement. It is a nit suggested by pkasting@ and it takes less space this way than with original: https://codereview.chromium.org/2155973002/diff2/20001:200001/third_party/Web... > It's also not clear to me that > if buffer->takeBitmapDataIfWritable() succeeds, that we should run the logic > below, since presumably prevBuffer is already emptied out. Same comment in the > WEBP image decoder. Regardless of the way the new frame got the data from previous frame (via take or via copy), if disposal method is DisposeOverwriteBgcolor, new frame |buffer| decoding starts from zeroing (making it fully transparent) previous frame subframe. It keeps the pixels outside subframe it got from previous frame (which again previous frame potentially gets from a long dependency chain) but it needs to erase only the subframe before decoding and writing new values to |buffer|. https://codereview.chromium.org/2155973002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:374: buffer->zeroFillFrameRect(prevRect); On 2016/09/29 00:40:04, Ken Russell wrote: > I don't understand why this preexisting code is calling this against buffer > instead of prevBuffer. Partly covered in the comment above. At this point decoding advances from prevBuffer to buffer, prevBuffer's content is copyed/moved to |buffer| and prev buffer disposal method specifies what to do in |buffer|. The comment states "clear the previous frame to transparent" and "frame" has a meaning of subrectangle/subframe of the full buffer. It can be rephrased as "clear the previous buffer's frame in the new buffer". Don't know if I managed to clarify it.
Thanks for the updates. LGTM
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2155973002/#ps220001 (title: "DCHECK m_status in ImageFrame. Shortened commit message. Thanks kbr@.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Save a copy when advancing to dependent GIF and WebP animation frames As clearCacheExceptFrame anyhow clears dependent frame, this is causing no change in cache content: instead of create new, copy and clear old, we do: swap content of old and empty new. Measurements for GIF file are available in the crbug.com/490895 BUG=490895, 425474 ========== to ========== Save a copy when advancing to dependent GIF and WebP animation frames As clearCacheExceptFrame anyhow clears dependent frame, this is causing no change in cache content: instead of create new, copy and clear old, we do: swap content of old and empty new. Measurements for GIF file are available in the crbug.com/490895 BUG=490895, 425474 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Save a copy when advancing to dependent GIF and WebP animation frames As clearCacheExceptFrame anyhow clears dependent frame, this is causing no change in cache content: instead of create new, copy and clear old, we do: swap content of old and empty new. Measurements for GIF file are available in the crbug.com/490895 BUG=490895, 425474 ========== to ========== Save a copy when advancing to dependent GIF and WebP animation frames As clearCacheExceptFrame anyhow clears dependent frame, this is causing no change in cache content: instead of create new, copy and clear old, we do: swap content of old and empty new. Measurements for GIF file are available in the crbug.com/490895 BUG=490895, 425474 Committed: https://crrev.com/3a38c33c398189620281ad19a0f8e5b7f6ac3e69 Cr-Commit-Position: refs/heads/master@{#422125} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3a38c33c398189620281ad19a0f8e5b7f6ac3e69 Cr-Commit-Position: refs/heads/master@{#422125} |
