Bug 172325 - [REGRESSION](r216901): Delete ImageDecoder if BitmapImage::destroyDecodedData() was called to destroy all the decoded frames
Summary: [REGRESSION](r216901): Delete ImageDecoder if BitmapImage::destroyDecodedData...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-18 17:42 PDT by Said Abou-Hallawa
Modified: 2017-05-18 21:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2017-05-18 18:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-05-18 17:42:26 PDT
In the change <http://trac.webkit.org/changeset/216901>, the function BitmapImage::destroyDecodedData() was changed such that if destroyAll was true but BitmapImage::destroyDecodedData() returns false, destroyAll was set to false which would lead to not calling ImageSource::clear(). ImageSource::clear() deletes the current ImageDecoder and creates a new one if the Image::data() is not null. Not calling ImageSource::clear() from BitmapImage::destroyDecodedData() when the passed destroyAll is true can cause the following problems:

1) CachedImage::didReplaceSharedBufferContents() calls m_image->destroyDecodedData(true) when the data SharedBuffer is switched and it assumes the current ImageDecoder will be deleted and a new one will be created with the new ShareBuffer.
2) For large images, the ImageDecoder may keep large buffers for raster data. Under memory pressure, the MemoryCache will request all the images to release their decoded frames. Because of https://bugs.webkit.org/show_bug.cgi?id=170640, we can't delete the current decoded frame. But deleting the ImageDecoder itself will release the raster data which will not be needed as long the current decoded frame is still cached.

However for animated images, it is okay not to call ImageSource::clear(). Animating an image happens after receiving all its data. So problem (1) is not a concern here. But deleting the ImageDecoder while animating an image will cause the animation to jitter because the new ImageDecoder has to decode all the frames from 0..currentFrame to be able to decode the nextFrame if it's equal to (currentFrame + 1).
Comment 1 Said Abou-Hallawa 2017-05-18 18:37:05 PDT
Created attachment 310589 [details]
Patch
Comment 2 Simon Fraser (smfr) 2017-05-18 18:49:46 PDT
Comment on attachment 310589 [details]
Patch

r=me but this code is becoming really hard to reason about.
Comment 3 Said Abou-Hallawa 2017-05-18 20:29:30 PDT
<rdar://problem/32243153>
Comment 4 WebKit Commit Bot 2017-05-18 21:09:48 PDT
Comment on attachment 310589 [details]
Patch

Clearing flags on attachment: 310589

Committed r217096: <http://trac.webkit.org/changeset/217096>
Comment 5 WebKit Commit Bot 2017-05-18 21:09:50 PDT
All reviewed patches have been landed.  Closing bug.