Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064273003:
Fix a SkImage leak in ImageBitmap:cropImage() (Closed)
DescriptionFix a SkImage leak in ImageBitmap:cropImage()
We're currently using the raw pointer API for image subsetting, and not
adopting the result immediately. Then if we take the
unPremulSkImageToPremul() path, we leak.
Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking.
R=reed@google.com,xidachen@chromium.org
BUG=619893
Committed: https://crrev.com/9315dfe2897c1586d4d54bfae5fc876345bf1c8f
Cr-Commit-Position: refs/heads/master@{#399869}
Patch Set 1 #Patch Set 2 : use a RefPtr #
Total comments: 3
Messages
Total messages: 12 (5 generated)
Description was changed from ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 ========== to ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 ==========
fmalita@chromium.org changed reviewers: + junov@chromium.org
There's more ImageBitmap code that should be converted to sk_sp APIs, but I'm keeping this fix minimal for clarity. I'll make another sk_sp cleanup pass later. https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:155: croppedSkImage->preroll(); Unrelated to this CL: I think I understand why we want to preroll before resolving the promise (http://crbug.com/580202), but I'm a bit concerned about doing it in this place. cropImage is called from most ImageBitmap constructors - isn't this potentially clogging the main thread, when the ImageBitmap is unrelated to a promise? It seems a safer place to force the decode would be ImageBitmapLoader, and only when resolving a promise. WDYT?
lgtm Thank you for cleaning this up. https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:155: croppedSkImage->preroll(); On 2016/06/15 08:41:54, f(malita) wrote: > Unrelated to this CL: I think I understand why we want to preroll before > resolving the promise (http://crbug.com/580202), but I'm a bit concerned about > doing it in this place. cropImage is called from most ImageBitmap constructors > - isn't this potentially clogging the main thread, when the ImageBitmap is > unrelated to a promise? > > It seems a safer place to force the decode would be ImageBitmapLoader, and only > when resolving a promise. WDYT? The problem is that ImageBitmapLoader is not always used. For example, when createImageBitmap from an HTMLImageElement, it doesn't use the ImageBitmapLoader, but calls the constructor of ImageBitmap(HTMLImageElement) directly which jumps to cropImage. Is there a way to check whether the croppedSkImage is always decoded or not? So that we can call preroll() only in the case that it is not decoded yet?
https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:155: croppedSkImage->preroll(); On 2016/06/15 08:56:11, xidachen wrote: > On 2016/06/15 08:41:54, f(malita) wrote: > > Unrelated to this CL: I think I understand why we want to preroll before > > resolving the promise (http://crbug.com/580202), but I'm a bit concerned about > > doing it in this place. cropImage is called from most ImageBitmap > constructors > > - isn't this potentially clogging the main thread, when the ImageBitmap is > > unrelated to a promise? > > > > It seems a safer place to force the decode would be ImageBitmapLoader, and > only > > when resolving a promise. WDYT? > > The problem is that ImageBitmapLoader is not always used. For example, when > createImageBitmap from an HTMLImageElement, it doesn't use the > ImageBitmapLoader, but calls the constructor of ImageBitmap(HTMLImageElement) > directly which jumps to cropImage. > > Is there a way to check whether the croppedSkImage is always decoded or not? So > that we can call preroll() only in the case that it is not decoded yet? sorry, I meant is there a way to check whether the croppedSkImage is *already* decoded or not?
On 2016/06/15 08:57:29, xidachen wrote: > https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): > > https://codereview.chromium.org/2064273003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/ImageBitmap.cpp:155: > croppedSkImage->preroll(); > On 2016/06/15 08:56:11, xidachen wrote: > > On 2016/06/15 08:41:54, f(malita) wrote: > > > Unrelated to this CL: I think I understand why we want to preroll before > > > resolving the promise (http://crbug.com/580202), but I'm a bit concerned > about > > > doing it in this place. cropImage is called from most ImageBitmap > > constructors > > > - isn't this potentially clogging the main thread, when the ImageBitmap is > > > unrelated to a promise? > > > > > > It seems a safer place to force the decode would be ImageBitmapLoader, and > > only > > > when resolving a promise. WDYT? > > > > The problem is that ImageBitmapLoader is not always used. For example, when > > createImageBitmap from an HTMLImageElement, it doesn't use the > > ImageBitmapLoader, but calls the constructor of ImageBitmap(HTMLImageElement) > > directly which jumps to cropImage. > > > > Is there a way to check whether the croppedSkImage is always decoded or not? > So > > that we can call preroll() only in the case that it is not decoded yet? > > sorry, I meant is there a way to check whether the croppedSkImage is *already* > decoded or not? If the image is already decoded, I don't think there's any cost when calling preroll (maybe reed can confirm) - so there's no benefit to making the call conditional. I was concerned that we're forcing the decoding upfront even for ImageBitmaps which don't need it. But if ImageBitmaps are only used for C2D stuff and this is the expected behavior (decoding happens before the result is passed back to JS), then I guess there's no problem.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064273003/20001
Message was sent while issue was closed.
Description was changed from ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 ========== to ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 ========== to ========== Fix a SkImage leak in ImageBitmap:cropImage() We're currently using the raw pointer API for image subsetting, and not adopting the result immediately. Then if we take the unPremulSkImageToPremul() path, we leak. Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking. R=reed@google.com,xidachen@chromium.org BUG=619893 Committed: https://crrev.com/9315dfe2897c1586d4d54bfae5fc876345bf1c8f Cr-Commit-Position: refs/heads/master@{#399869} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9315dfe2897c1586d4d54bfae5fc876345bf1c8f Cr-Commit-Position: refs/heads/master@{#399869} |
