Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
688423004:
Remove multi-frame image decoder when the image is completely decoded (Closed)
|
Created:
6 years, 1 month ago by Zhenyu Shan Modified:
6 years, 1 month ago CC:
blink-reviews, krit, Rik, jbroman, mkwst+moarreviews_chromium.org, danakj, pdr+graphicswatchlist_chromium.org, Stephen Chennney, rwlbuis, nduca Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionRemove multi-frame image decoder when the image is completely decoded
Count decoded frames in ImageFrameGenerator, and remove remove multi-frame image decoders when the image is completely decoded
This saves memory on pages with a lot of multi-frame images. data and discussion please see https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KZ8EDtEZ9-c
BUG=429614
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185311
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : Add a test #Patch Set 5 : fix DeferredImageDecoderTest crash #
Messages
Total messages: 28 (6 generated)
zhenyu.shan@intel.com changed reviewers: + eae@chromium.org, hclam@chromium.org
Please take a look, thanks!
On 2014/10/31 06:57:01, Zhenyu Shan wrote: > Please take a look, thanks! Please file a bug to capture some of the interesting discussion that happened on the blink-dev@ mailing list; the Skia team is interested in this kind of change, as well. You can then change the commit message here to automatically link to it.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:93: , m_framesCount(0) No need to have this variable. https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:239: const bool removeDecoder = m_framesCount && (completeFrameCount >= m_framesCount); It's better to have all the states to compute |removeDecoder| done here: bool removeDecoder; if (m_isMultiFrame) { int decodedFrameCount = /* .. for loop here to count decoded frames .. */; removeDecoder = decoder->isAllDataReceived() && decodedFrameCount == decoder->frameCount(); } else { removeDecoder = complete; } https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:243: ImageDecodingStore::instance()->removeDecoder(this, decoder); Need to clear m_frameComplete() here. https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:296: if (allDataReceived) { I prefer you move this block to "tryToResumeDecode". https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.h:114: size_t m_framesCount; I don't think you need this member variable. This is available in ImageDecoder already.
https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:296: if (allDataReceived) { On 2014/10/31 17:17:48, Alpha wrote: > I prefer you move this block to "tryToResumeDecode". There is an issue here: decoder->frameCount() and decoder->isAllDataAvailable() returns correct result only here, between the two decoder->setData(). For frameCount(), GIF decoder needs parse the raw data every time. If (m_data == null) it will crash. For isAllDataAvailable(), decoder->setData(0, false) set it to false. So if we call it in tryToResumeDecode() it will always be false. We can workaround the frameCount() issue by checking if (m_data == null) in GIFImageDecoder::parse, but we still need to know if |isAllDataRecieved| to get the final total frameCount() for an image. I agree that moving everything needed to calculate |removeDecoder| to tryToResumeDecode() is reasonable, but this is impossible for now because of these issues. So in patchset 2 I leave |m_framesCount| not removed. If there is any better possibility please tell me. (e.g. Can we change (*decoder)->setData(0, false) to (*decoder)->setData(0, allDataReceived)?)
https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/688423004/diff/1/Source/platform/graphics/Ima... Source/platform/graphics/ImageFrameGenerator.cpp:296: if (allDataReceived) { On 2014/11/03 08:19:17, Zhenyu Shan wrote: > On 2014/10/31 17:17:48, Alpha wrote: > > I prefer you move this block to "tryToResumeDecode". > > There is an issue here: decoder->frameCount() and decoder->isAllDataAvailable() > returns correct result only here, between the two decoder->setData(). > > For frameCount(), GIF decoder needs parse the raw data every time. If (m_data == > null) it will crash. > > For isAllDataAvailable(), decoder->setData(0, false) set it to false. So if we > call it in tryToResumeDecode() it will always be false. > > We can workaround the frameCount() issue by checking if (m_data == null) in > GIFImageDecoder::parse, but we still need to know if |isAllDataRecieved| to get > the final total frameCount() for an image. > > I agree that moving everything needed to calculate |removeDecoder| to > tryToResumeDecode() is reasonable, but this is impossible for now because of > these issues. So in patchset 2 I leave |m_framesCount| not removed. If there is > any better possibility please tell me. > > (e.g. Can we change (*decoder)->setData(0, false) to (*decoder)->setData(0, > allDataReceived)?) Okay that makes sense. Please see latest comments. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:230: // the decoder. The exception is multi-frame decoder which can generate This comment needs to be updated. Please doucment that we remove multi-frame decoder after all frames are decoded. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:303: m_framesCount = (*decoder)->frameCount(); Please add a comment to explain why. The reason of doing this is because m_frameCount is reliable only if all data is received, particularly with GIF. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:304: m_frameComplete.resize(m_framesCount); Don't do this. You already have a resize in tryToResumeDecode. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:114: size_t m_framesCount; This should be named m_frameCount to be consistent.
https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:230: // the decoder. The exception is multi-frame decoder which can generate On 2014/11/05 00:31:00, Alpha wrote: > This comment needs to be updated. Please doucment that we remove multi-frame > decoder after all frames are decoded. Done. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:303: m_framesCount = (*decoder)->frameCount(); On 2014/11/05 00:31:00, Alpha wrote: > Please add a comment to explain why. The reason of doing this is because > m_frameCount is reliable only if all data is received, particularly with GIF. Done. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:304: m_frameComplete.resize(m_framesCount); On 2014/11/05 00:31:00, Alpha wrote: > Don't do this. You already have a resize in tryToResumeDecode. Done. https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/688423004/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:114: size_t m_framesCount; On 2014/11/05 00:31:00, Alpha wrote: > This should be named m_frameCount to be consistent. Done.
Thanks. I think this is really close. Do you mind adding a small unit test for this behavior?
This behavior of deleting the decoder after all frames are decoded is good for the current caching policy, i.e. cache all frames in Skia. We can land this after test is ready. We're going to change the caching behavior of Skia such that frames are discarded after use. When that lands we'll not want to remove decoder so actively.
On 2014/11/05 07:13:09, Alpha wrote:
> Thanks. I think this is really close. Do you mind adding a small unit test for
> this behavior?
Unittest done. This unit test needs even more effort than the patch itself
because the original MockImageDecoder didn't consider MultiFrame situations... I
hope I've done things right.
Also fixed a crash in |frameHasAlpha|, all the indexes should be 0.
|incompleteDecodeBecomesCompleteMultiThreaded| still crashes, the same happens
without my patch so I think that should be fixed in another cl.
But it seems this file is not built in blink_platform_unittests by
default("legacy" as I saw in blink_platform.gypi)... Should I change that or
leave it?
On 2014/11/05 17:18:32, Alpha wrote: > This behavior of deleting the decoder after all frames are decoded is good for > the current caching policy, i.e. cache all frames in Skia. We can land this > after test is ready. > > We're going to change the caching behavior of Skia such that frames are > discarded after use. When that lands we'll not want to remove decoder so > actively. Good to hear that. Seems it will save a lot of memory. When will it happen and how can I help?
Ping for your review. Thanks!
On 2014/11/11 02:36:43, Zhenyu Shan wrote: > Ping for your review. Thanks! Change LGTM.
On 2014/11/06 09:27:55, Zhenyu Shan wrote:
> On 2014/11/05 07:13:09, Alpha wrote:
> > Thanks. I think this is really close. Do you mind adding a small unit test
for
> > this behavior?
>
> Unittest done. This unit test needs even more effort than the patch itself
> because the original MockImageDecoder didn't consider MultiFrame situations...
I
> hope I've done things right.
>
> Also fixed a crash in |frameHasAlpha|, all the indexes should be 0.
> |incompleteDecodeBecomesCompleteMultiThreaded| still crashes, the same happens
> without my patch so I think that should be fixed in another cl.
Hm.. The test should be running in bots and I'm not seeing a crash.
>
> But it seems this file is not built in blink_platform_unittests by
> default("legacy" as I saw in blink_platform.gypi)... Should I change that or
> leave it?
Please just leave it as is. The test should be included in webkit_unit_tests.
On 2014/11/06 09:30:03, Zhenyu Shan wrote: > On 2014/11/05 17:18:32, Alpha wrote: > > This behavior of deleting the decoder after all frames are decoded is good for > > the current caching policy, i.e. cache all frames in Skia. We can land this > > after test is ready. > > > > We're going to change the caching behavior of Skia such that frames are > > discarded after use. When that lands we'll not want to remove decoder so > > actively. > > Good to hear that. Seems it will save a lot of memory. When will it happen and > how can I help? Some background information: http://build.chromium.org/p/chromium/waterfall?builder=Chromium%20Mac https://code.google.com/p/skia/issues/detail?id=3099 This requires changes to both blink and skia. The change in Blink is simple. We'll keep only the last SkBitmap requested in DeferredImageDecoder. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Right now we keep the array of SkBitmaps that corresponds all frames known. Each SkBitmap will maintain a reference to the cache object. If we keep only the last SkBitmap known then Skia is free to evict cached objects.
The CQ bit was checked by zhenyu.shan@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/688423004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
zhenyu.shan@intel.com changed reviewers: + abarth@chromium.org, dominik.rottsches@intel.com, senorblanco@chromium.org
Fixed crash in DeferredImageDecoder tests & added file owners. Please take a look, Thanks!
rsLGTM. As Nat mentioned in the blink-dev thread, it would be really great to have a Telemetry test for this to ensure we don't regress on performance or memory savings.
LGTM
The CQ bit was checked by zhenyu.shan@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/688423004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 185311 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
