Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1777403002:
Allow const& for SkImages and SkPictures in draw methods. (Closed)
DescriptionAllow const& for SkImages and SkPictures in draw methods.
This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter).
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1777403002
Committed: https://skia.googlesource.com/skia/+/f8053da25981c6b3152b637c1c91f43cff194c25
Patch Set 1 #
Total comments: 3
Patch Set 2 : use const& instead of value #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : const& for pictures as well #
Total comments: 1
Patch Set 5 : change to const sk_sp<Foo>&, to match the signature of all of our factories and setters #Messages
Total messages: 56 (19 generated)
Description was changed from ========== take sk_sp version of SkImage parameter directly in SkCanvas is there a more straight-forward way to do this? should we *not* offer it, and force callers to pass *? BUG=skia: ========== to ========== take sk_sp version of SkImage parameter directly in SkCanvas is there a more straight-forward way to do this? should we *not* offer it, and force callers to pass *? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + bsalomon@google.com, bungeman@google.com, fmalita@chromium.org, mtklein@google.com
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/1
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h#new... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage> image, SkScalar left, SkScalar top, Taking the sk_sp by value here means you're taking ownership (creating or moving a ref). When the parameter goes out of scope then it will be released, causing churn. I don't think that was intended. I think you want another signature that takes one of these by reference? Maybe it's clearer to just pass the pointer only to indicate that this doesn't take ownership?
On 2016/03/10 22:14:02, bungeman1 wrote: > https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h > File include/core/SkCanvas.h (right): > > https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h#new... > include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage> image, SkScalar > left, SkScalar top, > Taking the sk_sp by value here means you're taking ownership (creating or moving > a ref). When the parameter goes out of scope then it will be released, causing > churn. I don't think that was intended. I think you want another signature that > takes one of these by reference? Maybe it's clearer to just pass the pointer > only to indicate that this doesn't take ownership? That's what the Windows bots are complaining about, because this has a ref, it may need to release that ref, resulting in the destructor needing to be called (which requires that the wrapped type be complete).
https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h#new... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage> image, SkScalar left, SkScalar top, On 2016/03/10 22:14:01, bungeman1 wrote: > Taking the sk_sp by value here means you're taking ownership (creating or moving > a ref). When the parameter goes out of scope then it will be released, causing > churn. I don't think that was intended. I think you want another signature that > takes one of these by reference? Maybe it's clearer to just pass the pointer > only to indicate that this doesn't take ownership? Is there a downside to a const sk_sp<const>& param? Seems like life will get annoying if we have to say drawImage(image.get()) all the time. Until the transition is complete we will have methods that take a ptr and do take ownership so I'm not sure it has an unambiguous meaning.
Doh! I actually meant to use const sp& for all of those. Will amend. So there's no magical incantation that allows either with only one signature? :(
https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h#new... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage> image, SkScalar left, SkScalar top, On 2016/03/10 22:22:16, bsalomon wrote: > On 2016/03/10 22:14:01, bungeman1 wrote: > > Taking the sk_sp by value here means you're taking ownership (creating or > moving > > a ref). When the parameter goes out of scope then it will be released, causing > > churn. I don't think that was intended. I think you want another signature > that > > takes one of these by reference? Maybe it's clearer to just pass the pointer > > only to indicate that this doesn't take ownership? > > Is there a downside to a const sk_sp<const>& param? Seems like life will get > annoying if we have to say drawImage(image.get()) all the time. > > Until the transition is complete we will have methods that take a ptr and do > take ownership so I'm not sure it has an unambiguous meaning. We would need two signatures, one for "const sk_sp<const>& param" and one for "sk_sp<const>&& param". One could do this with templates and universal references like template <typename T> SK_WHEN(is_same<T, sk_sp<const SkImage>>::value, void) drawImage(T&& image) or something like that.
On 2016/03/10 23:06:14, bungeman1 wrote: > https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h > File include/core/SkCanvas.h (right): > > https://codereview.chromium.org/1777403002/diff/1/include/core/SkCanvas.h#new... > include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage> image, SkScalar > left, SkScalar top, > On 2016/03/10 22:22:16, bsalomon wrote: > > On 2016/03/10 22:14:01, bungeman1 wrote: > > > Taking the sk_sp by value here means you're taking ownership (creating or > > moving > > > a ref). When the parameter goes out of scope then it will be released, > causing > > > churn. I don't think that was intended. I think you want another signature > > that > > > takes one of these by reference? Maybe it's clearer to just pass the pointer > > > only to indicate that this doesn't take ownership? > > > > Is there a downside to a const sk_sp<const>& param? Seems like life will get > > annoying if we have to say drawImage(image.get()) all the time. > > > > Until the transition is complete we will have methods that take a ptr and do > > take ownership so I'm not sure it has an unambiguous meaning. > > We would need two signatures, one for "const sk_sp<const>& param" and one for > "sk_sp<const>&& param". One could do this with templates and universal > references like > > template <typename T> SK_WHEN(is_same<T, sk_sp<const SkImage>>::value, void) > drawImage(T&& image) > > or something like that. I'm an idiot, the rvalue with bind to the const sk_sp<const>&, so you only need that one. Forget I said that.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/20001
now passing by const&
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1777403002/#ps20001 (title: "use const& instead of value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/20001
The CQ bit was unchecked by reed@google.com
On 2016/03/10 23:15:52, bungeman1 wrote: > the rvalue with bind to the const sk_sp<const>&, so you only need > that one. To make sure I get this straight in my head: it will bind, but it will also preclude move optimizations (cannot move out of const&), right? This is likely OK for the canvas API (although say a recording canvas may benefit from no-churn ownership transfer), but for setters we really want to optimize the ownership transfer case - hence the current pass-by-val+move direction.
Do we think move scenarios are common for images?
Clearly (looking at our code and chrome's) moving effects like shaders is very
common:
setShader(MakeShader())
Drawing images is not so clear to me, as many clients seem to also hold on to
the image afterwards (in which case const& is as efficient as anything).
Other considerations?
On 2016/03/11 14:40:32, reed1 wrote: > Do we think move scenarios are common for images? About the only move scenario I can think of involves recording (where say Blink records some temp image/subpicture and then caches the resulting skp). Not very common, I don't think, so it makes sense to optimize the SkCanvas API for the no-transfer case. I just brought it up to make sure *my* understanding of the trade-offs is accurate :)
Here's what we can do if we restrict ourselves to one function, passing by const
T&, T, or T&&:
1) If we pass by const T&, we can copy or borrow, but not move. This is
familiar C++98 behavior.
2) If we pass by T, we can copy or move (if the caller uses std::move or
passes an rvalue), but not borrow. This what we've gotten used to lately with
C++11.
3) If we pass by T&&, we can actually copy, move, or borrow as we like, but it
requires that the caller is willing to let us move if we want to (i.e. they must
have called std::move, passed an rvalue, etc., and can't really assume anything
about the state of the object after the call).
3) is the most powerful, but it's rather burdensome on the client if it's the
only interface we offer.
What do we get if we offer pairs of these?
1+2) is a non-starter. Methods that only differ by whether the parameter is
constT& or T are ambiguous.
2+3) is not a good option. Technically we could copy, borrow, or move with
that combination, but we'd only be able to borrow under the same odd API as 3).
And this combination makes passing an rvalue ambiguous, which makes using the
API pointlessly awkward and verbose.
1+3) is a good option. We can copy or borrow via 1, and move (or whatever we
want) via 3.
void foo(const T&);
void foo(T&&);
Here's a little test program I used to remind myself what happens when:
#include <utility>
#include <stdio.h>
struct Bar{
Bar() { printf("create %p\n", this); }
~Bar() { printf("destroy %p\n", this); }
Bar(const Bar& that) { printf("copy from %p to %p\n", &that, this); }
Bar(Bar&& that) { printf("move from %p to %p\n", &that, this); }
};
//void bar(Bar b) { printf("bar(Bar), %p\n", &b); }
void bar(const Bar& b) { printf("bar(const Bar&), %p\n", &b); }
void bar(Bar&& b) { printf("bar(Bar&&), %p\n", &b); }
int main() {
Bar x;
bar(x);
bar(std::move(x));
bar(Bar());
return 0;
}
https://codereview.chromium.org/1777403002/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/20001/include/core/SkCanvas.h... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage>& image, SkScalar left, SkScalar top, First off, this has to be 'const sk_sp<const SkImage>&' or it won't work with rvalues (for example canvas->drawImage(CreateImage(...)) won't work). Even with this change, I don't think this works well with the common case of sk_sp<SkImage> foo = ...; canvas->drawImage(foo, ...); The issue is that sk_sp<SkImage> and sk_sp<const SkImage> are distinct types, so a conversion is required. Since foo is an lvalue, the compiler uses the 'is_convertible' copy-like constructor to create a temporary sk_sp<const SkImage>. This means at the call site there will be a ref-unref pair around making this call.
On 2016/03/11 15:28:00, mtklein wrote:
> Here's what we can do if we restrict ourselves to one function, passing by
const
> T&, T, or T&&:
>
> 1) If we pass by const T&, we can copy or borrow, but not move. This is
> familiar C++98 behavior.
>
> 2) If we pass by T, we can copy or move (if the caller uses std::move or
> passes an rvalue), but not borrow. This what we've gotten used to lately with
> C++11.
>
> 3) If we pass by T&&, we can actually copy, move, or borrow as we like, but
it
> requires that the caller is willing to let us move if we want to (i.e. they
must
> have called std::move, passed an rvalue, etc., and can't really assume
anything
> about the state of the object after the call).
>
>
> 3) is the most powerful, but it's rather burdensome on the client if it's the
> only interface we offer.
>
> What do we get if we offer pairs of these?
> 1+2) is a non-starter. Methods that only differ by whether the parameter
is
> constT& or T are ambiguous.
>
> 2+3) is not a good option. Technically we could copy, borrow, or move with
> that combination, but we'd only be able to borrow under the same odd API as
3).
> And this combination makes passing an rvalue ambiguous, which makes using the
> API pointlessly awkward and verbose.
>
> 1+3) is a good option. We can copy or borrow via 1, and move (or whatever
we
> want) via 3.
>
> void foo(const T&);
> void foo(T&&);
>
> Here's a little test program I used to remind myself what happens when:
>
> #include <utility>
> #include <stdio.h>
>
> struct Bar{
> Bar() { printf("create %p\n", this); }
> ~Bar() { printf("destroy %p\n", this); }
>
> Bar(const Bar& that) { printf("copy from %p to %p\n", &that, this); }
> Bar(Bar&& that) { printf("move from %p to %p\n", &that, this); }
> };
>
> //void bar(Bar b) { printf("bar(Bar), %p\n", &b); }
> void bar(const Bar& b) { printf("bar(const Bar&), %p\n", &b); }
> void bar(Bar&& b) { printf("bar(Bar&&), %p\n", &b); }
>
> int main() {
>
> Bar x;
>
> bar(x);
> bar(std::move(x));
>
> bar(Bar());
>
> return 0;
> }
Facts over, now my opinion. I don't see any need yet to add methods that pass
sk_sp<SkImage>&&. const sk_sp<SkImage>& seems just fine until we hit some new
recording-cost crisis. I agree that passing sk_sp<SkImage> all the time would
be an unnecessary pessimization.
https://codereview.chromium.org/1777403002/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/20001/include/core/SkCanvas.h... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage>& image, SkScalar left, SkScalar top, On 2016/03/11 15:31:11, bungeman1 wrote: > First off, this has to be 'const sk_sp<const SkImage>&' or it won't work with > rvalues (for example canvas->drawImage(CreateImage(...)) won't work). > > > Even with this change, I don't think this works well with the common case of > > sk_sp<SkImage> foo = ...; > canvas->drawImage(foo, ...); > > The issue is that sk_sp<SkImage> and sk_sp<const SkImage> are distinct types, so > a conversion is required. Since foo is an lvalue, the compiler uses the > 'is_convertible' copy-like constructor to create a temporary sk_sp<const > SkImage>. This means at the call site there will be a ref-unref pair around > making this call. Writing this declaration as template <typename T> skstd::enable_if_t<std::is_convertible<T, sk_sp<const SkImage>>::value, void> drawImage(...) does work around the whole making a copy thing, though I'm not sure that's the kind of solution you want. This essentially says that if you could have created a temporary sk_sp from what you had, just pass me what you had instead. I was just thinking that with generics like in Java this concept would be written something like drawImage(const sk_sp<? extends const SkImage>&, ...) but that only works in Java because they don't have templates or template specialization.
Glad this is sparking discussion. I am not in a hurry to land this (or a variant)
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/40001
Description was changed from ========== take sk_sp version of SkImage parameter directly in SkCanvas is there a more straight-forward way to do this? should we *not* offer it, and force callers to pass *? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Allow const* or const& for SkImages in draw methods. This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
ptal
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/60001
Description was changed from ========== Allow const* or const& for SkImages in draw methods. This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Allow const& for SkImages and SkPictures in draw methods. This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1777403002/diff/60001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1777403002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:775: void drawImage(sk_sp<const SkImage>& image, SkScalar left, SkScalar top, These all need to be const references 'const sk_sp<const SkImage>&', or I don't think they're going to do what you think they're going to do. Unfortunately, due to the limitations of the type system and whatnot, we may be better off with a 'borrowed_sk_sp' type to represent this sort of borrowing. Otherwise we'll run into invisible inefficiencies in various use cases.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/80001
agreed. Changed to const sk_sp<Foo>&, to match our factories and setters.
On 2016/03/17 14:46:22, bungeman1 wrote: > Unfortunately, due to the limitations of the type system and whatnot, we may be > better off with a 'borrowed_sk_sp' type to represent this sort of borrowing. PassRefPtr? :) On 2016/03/17 14:55:34, reed1 wrote: > agreed. Changed to const sk_sp<Foo>&, to match our factories and setters. const sk_sp<Foo>& lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1777403002/#ps80001 (title: "change to const sk_sp<Foo>&, to match the signature of all of our factories and setters")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1777403002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1777403002/80001
Message was sent while issue was closed.
Description was changed from ========== Allow const& for SkImages and SkPictures in draw methods. This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Allow const& for SkImages and SkPictures in draw methods. This is just a convenience for callers, since we are (strongly) encouraging them to use sk_sp<> for managing image objects (and pictures btw). No change under the hood, as we are keeping our agnostic approach to these for the protected virtuals (since often we don't want to ref the parameter). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f8053da25981c6b3152b637c1c91f43cff194c25 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/f8053da25981c6b3152b637c1c91f43cff194c25
Message was sent while issue was closed.
On 2016/03/17 14:58:10, f(malita) wrote: > On 2016/03/17 14:46:22, bungeman1 wrote: > > Unfortunately, due to the limitations of the type system and whatnot, we may > be > > better off with a 'borrowed_sk_sp' type to represent this sort of borrowing. > > PassRefPtr? :) > > > On 2016/03/17 14:55:34, reed1 wrote: > > agreed. Changed to const sk_sp<Foo>&, to match our factories and setters. > > const sk_sp<Foo>& lgtm So, we decided against simple "const SkImage&" because sometimes we will take a ref when drawing?
Message was sent while issue was closed.
On 2016/04/06 22:16:24, Ben Wagner wrote: > On 2016/03/17 14:58:10, f(malita) wrote: > > On 2016/03/17 14:46:22, bungeman1 wrote: > > > Unfortunately, due to the limitations of the type system and whatnot, we may > > be > > > better off with a 'borrowed_sk_sp' type to represent this sort of borrowing. > > > > PassRefPtr? :) > > > > > > On 2016/03/17 14:55:34, reed1 wrote: > > > agreed. Changed to const sk_sp<Foo>&, to match our factories and setters. > > > > const sk_sp<Foo>& lgtm > > So, we decided against simple "const SkImage&" because sometimes we will take a > ref when drawing? That's one reason: SkCanvas is an interface, and we have several impls which don't just draw - e.g. SkRecorder, SkDebugCanvas, CC's ImageHijackCanvas. Another reason might be that ref-counted objects are (now) normally managed/stored using sk_sp, so it's natural to stay in this domain. MikeR ran into one drawback: if clients have a sk_sp<FooDerived> rather than a sk_sp<Foo>, then conversion to const sk_sp<Foo>& adds churn.
Message was sent while issue was closed.
On 2016/04/06 22:16:24, Ben Wagner wrote: > On 2016/03/17 14:58:10, f(malita) wrote: > > On 2016/03/17 14:46:22, bungeman1 wrote: > > > Unfortunately, due to the limitations of the type system and whatnot, we may > > be > > > better off with a 'borrowed_sk_sp' type to represent this sort of borrowing. > > > > PassRefPtr? :) > > > > > > On 2016/03/17 14:55:34, reed1 wrote: > > > agreed. Changed to const sk_sp<Foo>&, to match our factories and setters. > > > > const sk_sp<Foo>& lgtm > > So, we decided against simple "const SkImage&" because sometimes we will take a > ref when drawing? We have always had simple "SkImage*", and we still do. The sp variants are just convenience helpers so callers don't have to say obj.get() each time.
Message was sent while issue was closed.
On 2016/04/07 13:40:12, reed1 wrote: > On 2016/04/06 22:16:24, Ben Wagner wrote: > > On 2016/03/17 14:58:10, f(malita) wrote: > > > On 2016/03/17 14:46:22, bungeman1 wrote: > > > > Unfortunately, due to the limitations of the type system and whatnot, we > may > > > be > > > > better off with a 'borrowed_sk_sp' type to represent this sort of > borrowing. > > > > > > PassRefPtr? :) > > > > > > > > > On 2016/03/17 14:55:34, reed1 wrote: > > > > agreed. Changed to const sk_sp<Foo>&, to match our factories and setters. > > > > > > const sk_sp<Foo>& lgtm > > > > So, we decided against simple "const SkImage&" because sometimes we will take > a > > ref when drawing? > > We have always had simple "SkImage*", and we still do. The sp variants are just > convenience helpers so callers don't have to say obj.get() each time. And if you take "const SkImage&" you can say '*obj' instead of obj.get(), which is much terser. Of course, this also states that you don't take nullptr, which isn't always the case. |
