Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2326473005:
Use sk_sp<SkBitmap::Allocator> instead of raw pointers or WTF::RefPtr<T>. (Closed)
|
Created:
4 years, 3 months ago by Łukasz Anforowicz Modified:
4 years, 3 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis, scroggo_chromium, danakj, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllocate ExternalMemoryAllocator on the stack.
The change is desirable because:
- Ref-counting doesn't result in memory safety here, because
ExternalMemoryAllocator wraps a raw pointer to short-lived
memory. Ref-counting cannot extend the lifetime of the
memory this pointer points to.
- No consumers of SkBitmap::Allocator currently use ref-counting.
If SkBitmap::Allocator was designed today, it probably wouldn't
use SkRefCnt as a base class.
- Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF,
but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should
only be used with Blink/WTF types going forward (i.e. Skia types
should be wrapped in smart pointers provided by Skia).
More discussion is in an older code review:
https://codereview.chromium.org/1880993003/#msg10
BUG=641014
Committed: https://crrev.com/34971fa949b0957eb7c4feb575f372b2c0c6b98a
Cr-Commit-Position: refs/heads/master@{#418657}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Allocating ExternalMemoryAllocator on the stack. #Messages
Total messages: 32 (17 generated)
The CQ bit was checked by lukasza@chromium.org 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...
Description was changed from ========== Use sk_sp<SkBitmap::Allocator> instead of raw pointers or WTF::RefPtr<T>. The change is desirable because: - Smart pointers offer better memory safety. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). BUG=641014 ========== to ========== Use sk_sp<SkBitmap::Allocator> instead of raw pointers or WTF::RefPtr<T>. The change is desirable because: - Smart pointers offer better memory safety. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). BUG=641014 ==========
lukasza@chromium.org changed reviewers: + senorblanco@chromium.org
Stephen, could you please take a look? https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:281: (*decoder)->setMemoryAllocator(std::move(allocator)); |allocator| comes from |decodeAndScale|, which will destroy |allocator| via the stack-allocated smart pointer. I don't understand why in the old code it was safe to pass |allocator| (short-lived, owned by callstack frame of |decodeAndScale|) into |decoder| (potentially long-lived, right?). At any rate, I think the new code is safe in that regard.
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:281: (*decoder)->setMemoryAllocator(std::move(allocator)); On 2016/09/09 20:10:04, Łukasz Anforowicz wrote: > |allocator| comes from |decodeAndScale|, which will destroy |allocator| via the > stack-allocated smart pointer. I don't understand why in the old code it was > safe to pass |allocator| (short-lived, owned by callstack frame of > |decodeAndScale|) into |decoder| (potentially long-lived, right?). At any rate, > I think the new code is safe in that regard. It's safe (but arguably not safe from future changes) because we call setMemoryAllocator(0) down below (line 297 in this version). Before we had sk_sp, I started changing this to use RefPtr. It's probably worth looking at Florin's comments in that CL: https://codereview.chromium.org/1880993003/#msg10
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:281: (*decoder)->setMemoryAllocator(std::move(allocator)); On 2016/09/09 20:51:04, scroggo_chromium wrote: > On 2016/09/09 20:10:04, Łukasz Anforowicz wrote: > > |allocator| comes from |decodeAndScale|, which will destroy |allocator| via > the > > stack-allocated smart pointer. I don't understand why in the old code it was > > safe to pass |allocator| (short-lived, owned by callstack frame of > > |decodeAndScale|) into |decoder| (potentially long-lived, right?). At any > rate, > > I think the new code is safe in that regard. > > It's safe (but arguably not safe from future changes) because we call > setMemoryAllocator(0) down below (line 297 in this version). > > Before we had sk_sp, I started changing this to use RefPtr. It's probably worth > looking at Florin's comments in that CL: > > https://codereview.chromium.org/1880993003/#msg10 My first priority is to eradicate RefPtr<SkFoo> (because this blocks Chromium style migration). I am not sure how to get there: 1. Minimal CL (i.e. only 2 line change: RefPtr->sk_sp + adoptRef->sk_sp contructor). 2. #1 + also switch from raw pointer to smart pointer in decode and tryToResumeDecode methods. 3. #2 + also switch from raw pointer to smart pointer the ImageFrame::m_allocator field. 4. Long-term refactoring (i.e. remove setAllocator from ImageFrame and from ImageDecoder + pass a raw pointer to allocator through the whole method call chain between ImageDecoder::frameBufferAtIndex and ImageFrame::setSizeAndColorProfile). There are two difficulties I see with #4. - Size of the required refactoring. I tried to look into this today and it seems to be a massive refactoring (based on looking in Cr code search at the size of the call tree of ImageFrame::setSizeAndColorProfile). - Deriving from SkRefCnt - this cannot be changed without changing Skia and it would feel weird/fragile to be stack-allocating an object derived from SkRefCnt. So - WDYT? Should I be trying to do #1, #2, #3 or #4? :-) It sounds that #1 might be easiest to land, right?
https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:281: (*decoder)->setMemoryAllocator(std::move(allocator)); On 2016/09/12 20:50:09, Łukasz Anforowicz wrote: > On 2016/09/09 20:51:04, scroggo_chromium wrote: > > On 2016/09/09 20:10:04, Łukasz Anforowicz wrote: > > > |allocator| comes from |decodeAndScale|, which will destroy |allocator| via > > the > > > stack-allocated smart pointer. I don't understand why in the old code it > was > > > safe to pass |allocator| (short-lived, owned by callstack frame of > > > |decodeAndScale|) into |decoder| (potentially long-lived, right?). At any > > rate, > > > I think the new code is safe in that regard. > > > > It's safe (but arguably not safe from future changes) because we call > > setMemoryAllocator(0) down below (line 297 in this version). Another note: it would *not* be safe if we did not call setMemoryAllocator(0) down below, even with your changes to use sk_sp, because the ExternalMemoryAllocator has a pointer to memory it does not own. So this might be suggesting some safety that isn't really there. > > > > Before we had sk_sp, I started changing this to use RefPtr. It's probably > worth > > looking at Florin's comments in that CL: > > > > https://codereview.chromium.org/1880993003/#msg10 > > My first priority is to eradicate RefPtr<SkFoo> (because this blocks Chromium > style migration). I am not sure how to get there: > > 1. Minimal CL (i.e. only 2 line change: RefPtr->sk_sp + adoptRef->sk_sp > contructor). > > 2. #1 + also switch from raw pointer to smart pointer in decode and > tryToResumeDecode methods. > > 3. #2 + also switch from raw pointer to smart pointer the > ImageFrame::m_allocator field. > > 4. Long-term refactoring (i.e. remove setAllocator from ImageFrame and from > ImageDecoder + pass a raw pointer to allocator through the whole method call > chain between ImageDecoder::frameBufferAtIndex and > ImageFrame::setSizeAndColorProfile). > > > There are two difficulties I see with #4. > > - Size of the required refactoring. I tried to look into this today and it > seems to be a massive refactoring (based on looking in Cr code search at the > size of the call tree of ImageFrame::setSizeAndColorProfile). > > - Deriving from SkRefCnt - this cannot be changed without changing Skia and it > would feel weird/fragile to be stack-allocating an object derived from SkRefCnt. > > > So - WDYT? Should I be trying to do #1, #2, #3 or #4? :-) It sounds that #1 > might be easiest to land, right? In theory, I would vote for #4, but there's no reason for that to hold you up. To help you eradicate RefPtr, I think it makes more sense to stack allocate with some comments explaining why it's safe (as discussed in https://codereview.chromium.org/1880993003/#msg10). > it would feel weird/fragile to be stack-allocating an object derived from SkRefCnt. FWIW, we do this elsewhere, and I think it is safe so long as you know what you are doing. Either way, this code is already fragile, and I don't think switching to sk_sp as in this patch set (which I think corresponds to option #3) makes it less so. If you're dead set against stack allocation, I suppose you could do #1, although to be honest that is essentially the same (except it has a heap allocation!). https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:127: const sk_sp<SkBitmap::Allocator>& allocator() const { return m_allocator; } We went back and forth on how to handle this inside Skia. I think we decided to return a raw pointer in this case. I don't know whether that means we should do the same for sk_sp's in Chromium/Blink. (You may have already seen the document about sk_sp, but just in case, it's here: https://docs.google.com/document/d/1PZRnDBwhnB_Ug6DtlkphAvJ9Ga5U3ONbSn1wf6jXX...) https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: void setMemoryAllocator(sk_sp<SkBitmap::Allocator> allocator) { m_allocator = allocator; } Don't you want to std::move |allocator|?
The CQ bit was checked by lukasza@chromium.org 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...
Description was changed from ========== Use sk_sp<SkBitmap::Allocator> instead of raw pointers or WTF::RefPtr<T>. The change is desirable because: - Smart pointers offer better memory safety. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). BUG=641014 ========== to ========== Allocate ExternalMemoryAllocator on the stack. The change is desirable because: - Ref-counting doesn't result in memory safety here, because ExternalMemoryAllocator wraps a raw pointer to short-lived memory. Ref-counting cannot extend the lifetime of the memory this pointer points to. - No consumers of SkBitmap::Allocator currently use ref-counting. If SkBitmap::Allocator was designed today, it probably wouldn't use SkRefCnt as a base class. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). More discussion is in an older code review: https://codereview.chromium.org/1880993003/#msg10 BUG=641014 ==========
scroggo@, can you take another look please? (I've tried to also update the CL description so it reflects the different approach we are taking than in the initial patchset). https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:281: (*decoder)->setMemoryAllocator(std::move(allocator)); On 2016/09/13 14:54:42, scroggo_chromium wrote: > On 2016/09/12 20:50:09, Łukasz Anforowicz wrote: > > On 2016/09/09 20:51:04, scroggo_chromium wrote: > > > On 2016/09/09 20:10:04, Łukasz Anforowicz wrote: > > > > |allocator| comes from |decodeAndScale|, which will destroy |allocator| > via > > > the > > > > stack-allocated smart pointer. I don't understand why in the old code it > > was > > > > safe to pass |allocator| (short-lived, owned by callstack frame of > > > > |decodeAndScale|) into |decoder| (potentially long-lived, right?). At any > > > rate, > > > > I think the new code is safe in that regard. > > > > > > It's safe (but arguably not safe from future changes) because we call > > > setMemoryAllocator(0) down below (line 297 in this version). > > Another note: it would *not* be safe if we did not call setMemoryAllocator(0) > down below, even with your changes to use sk_sp, because the > ExternalMemoryAllocator has a pointer to memory it does not own. So this might > be suggesting some safety that isn't really there. Thanks for pointing this out - I agree that using sk_sp<T> would be wrong, because in this case would be giving an incorrect sense of safety. > > > > > > Before we had sk_sp, I started changing this to use RefPtr. It's probably > > worth > > > looking at Florin's comments in that CL: > > > > > > https://codereview.chromium.org/1880993003/#msg10 > > > > My first priority is to eradicate RefPtr<SkFoo> (because this blocks Chromium > > style migration). I am not sure how to get there: > > > > 1. Minimal CL (i.e. only 2 line change: RefPtr->sk_sp + adoptRef->sk_sp > > contructor). > > > > 2. #1 + also switch from raw pointer to smart pointer in decode and > > tryToResumeDecode methods. > > > > 3. #2 + also switch from raw pointer to smart pointer the > > ImageFrame::m_allocator field. > > > > 4. Long-term refactoring (i.e. remove setAllocator from ImageFrame and from > > ImageDecoder + pass a raw pointer to allocator through the whole method call > > chain between ImageDecoder::frameBufferAtIndex and > > ImageFrame::setSizeAndColorProfile). > > > > > > There are two difficulties I see with #4. > > > > - Size of the required refactoring. I tried to look into this today and it > > seems to be a massive refactoring (based on looking in Cr code search at the > > size of the call tree of ImageFrame::setSizeAndColorProfile). > > > > - Deriving from SkRefCnt - this cannot be changed without changing Skia and it > > would feel weird/fragile to be stack-allocating an object derived from > SkRefCnt. > > > > > > So - WDYT? Should I be trying to do #1, #2, #3 or #4? :-) It sounds that #1 > > might be easiest to land, right? > > In theory, I would vote for #4, but there's no reason for that to hold you up. > To help you eradicate RefPtr, I think it makes more sense to stack allocate with > some comments explaining why it's safe (as discussed in > https://codereview.chromium.org/1880993003/#msg10). Agreed - stack allocation makes sense to me now. I think we don't necessarily need the comments from https://codereview.chromium.org/1880993003/#msg10 that wonder whether ref is taken at any time. We only care that no extra ref is taken when the stack allocated object gets destroyed. Therefore, I think the comment + DCHECK I have in the latest patchset should be sufficient. > > > it would feel weird/fragile to be stack-allocating an object derived from > SkRefCnt. > > FWIW, we do this elsewhere, and I think it is safe so long as you know what you > are doing. Ack. The DCHECK(x.unique()) makes me happier about this idea. > > Either way, this code is already fragile, and I don't think switching to sk_sp > as in this patch set (which I think corresponds to option #3) makes it less so. > If you're dead set against stack allocation, I suppose you could do #1, although > to be honest that is essentially the same (except it has a heap allocation!). I see now how options #1 through #3 are wrong, because they don't offer any extra safety (because the allocator keeps wrapping a short-lived pointer). https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right): https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:127: const sk_sp<SkBitmap::Allocator>& allocator() const { return m_allocator; } On 2016/09/13 14:54:42, scroggo_chromium wrote: > We went back and forth on how to handle this inside Skia. I think we decided to > return a raw pointer in this case. I don't know whether that means we should do > the same for sk_sp's in Chromium/Blink. > I've reverted this part of the change and just focused on eradicating RefPtr<SkXxx> by allocating ExternalMemoryAllocator on the stack. > (You may have already seen the document about sk_sp, but just in case, it's > here: > https://docs.google.com/document/d/1PZRnDBwhnB_Ug6DtlkphAvJ9Ga5U3ONbSn1wf6jXX...) Thanks - I think I have not seen this document before. https://codereview.chromium.org/2326473005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:139: void setMemoryAllocator(sk_sp<SkBitmap::Allocator> allocator) { m_allocator = allocator; } On 2016/09/13 14:54:42, scroggo_chromium wrote: > Don't you want to std::move |allocator|? Ooops - yes, std::move would be appropriate here. At any rate, I've reverted this part of the change and just focused on eradicating RefPtr<SkXxx> by allocating ExternalMemoryAllocator on the stack.
LGTM. Thanks!
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
senorblanco@, can you do an OWNERS review please?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM
The CQ bit was checked by lukasza@chromium.org
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by lukasza@chromium.org
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Allocate ExternalMemoryAllocator on the stack. The change is desirable because: - Ref-counting doesn't result in memory safety here, because ExternalMemoryAllocator wraps a raw pointer to short-lived memory. Ref-counting cannot extend the lifetime of the memory this pointer points to. - No consumers of SkBitmap::Allocator currently use ref-counting. If SkBitmap::Allocator was designed today, it probably wouldn't use SkRefCnt as a base class. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). More discussion is in an older code review: https://codereview.chromium.org/1880993003/#msg10 BUG=641014 ========== to ========== Allocate ExternalMemoryAllocator on the stack. The change is desirable because: - Ref-counting doesn't result in memory safety here, because ExternalMemoryAllocator wraps a raw pointer to short-lived memory. Ref-counting cannot extend the lifetime of the memory this pointer points to. - No consumers of SkBitmap::Allocator currently use ref-counting. If SkBitmap::Allocator was designed today, it probably wouldn't use SkRefCnt as a base class. - Blink-to-Chromium style conversion will rename |ref| to |Ref| in WTF, but won't do it outside of Blink/WTF (e.g. in Skia), so RefPtr should only be used with Blink/WTF types going forward (i.e. Skia types should be wrapped in smart pointers provided by Skia). More discussion is in an older code review: https://codereview.chromium.org/1880993003/#msg10 BUG=641014 Committed: https://crrev.com/34971fa949b0957eb7c4feb575f372b2c0c6b98a Cr-Commit-Position: refs/heads/master@{#418657} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/34971fa949b0957eb7c4feb575f372b2c0c6b98a Cr-Commit-Position: refs/heads/master@{#418657} |
