pin as texture api

pin as texture api

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241353002

Committed: https://skia.googlesource.com/skia/+/2d5b7147032e3806b5895667a899440119707c2d

[reed1, 2016-08-15 17:22:20]

Description was changed from

==========
pin as texture api

BUG=skia:
==========

to

==========
pin as texture api

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241353002
==========

[reed1, 2016-08-15 17:23:28]

reed@google.com changed reviewers:
+ brianosman@google.com, bsalomon@google.com, djsollen@google.com,
robertphillips@google.com

[reed1, 2016-08-15 17:23:28]

Seems like I could move this pattern into SkImage_Base, and trivially have it
work for all image types (with an override for Image_Gpu which would be a
no-op)...

[reed1, 2016-08-15 18:51:06]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 18:51:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bsalomon, 2016-08-15 19:10:40]

https://codereview.chromium.org/2241353002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.h (right):

https://codereview.chromium.org/2241353002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.h:39: class GrImageTextureAdjuster : public
GrRawTextureAdjuster {
I wonder if we should make GrTextureAdjuster a final class now and not have any
subclasses. IIRC there used to be additional complexity with texture-backed
SkBitmaps that is now gone.

[commit-bot: I haz the power, 2016-08-15 19:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 19:24:49]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-16 14:21:34]

ptal -- will remove the SkCanvas patch (just for testing) before landing

[reed1, 2016-08-16 14:21:38]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 14:21:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bsalomon, 2016-08-16 14:40:56]

A couple things:

1) I think we should check that the GrContext* passed in matches the existing
texture (previous pin for raster if exists, and backing store for gpu). If they
don't match I guess fail?

2) After talking to mtklein@ yesterday I think we should require that the client
explicitly unpin the texture before the image's ref count reaches zero (and be
sure to unpin on safe thread) and assert that it is unpinned in ~SkImage.
Otherwise it is easy for the client to have the final unref occur in another
thread which would be unsafe.

[commit-bot: I haz the power, 2016-08-16 14:48:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 14:48:16]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-16 16:08:13]

On 2016/08/16 14:40:56, bsalomon wrote:
> A couple things:
> 
> 1) I think we should check that the GrContext* passed in matches the existing
> texture (previous pin for raster if exists, and backing store for gpu). If
they
> don't match I guess fail?
> 
> 2) After talking to mtklein@ yesterday I think we should require that the
client
> explicitly unpin the texture before the image's ref count reaches zero (and be
> sure to unpin on safe thread) and assert that it is unpinned in ~SkImage.
> Otherwise it is easy for the client to have the final unref occur in another
> thread which would be unsafe.

1. agreed. almost did it earlier, but will now.

2. I'm fine to add that additional constraint, but is this situation any
different from SkImage_Gpu today, where the client must perform the final unref
happens in the proper thread?

[mtklein, 2016-08-16 16:09:14]

> 2. I'm fine to add that additional constraint, but is this situation any
> different from SkImage_Gpu today, where the client must perform the final
unref
> happens in the proper thread?

If we unpin explicitly on the correct thread, the final unref can happy on any
thread, right?

[mtklein, 2016-08-16 16:13:24]

On 2016/08/16 16:09:14, mtklein wrote:
> > 2. I'm fine to add that additional constraint, but is this situation any
> > different from SkImage_Gpu today, where the client must perform the final
> unref
> > happens in the proper thread?
> 
> If we unpin explicitly on the correct thread, the final unref can happy on any
> thread, right?

*happen

[mtklein, 2016-08-16 16:13:24]

On 2016/08/16 16:09:14, mtklein wrote:
> > 2. I'm fine to add that additional constraint, but is this situation any
> > different from SkImage_Gpu today, where the client must perform the final
> unref
> > happens in the proper thread?
> 
> If we unpin explicitly on the correct thread, the final unref can happy on any
> thread, right?

*happen

[bsalomon, 2016-08-16 16:21:00]

On 2016/08/16 16:13:24, mtklein wrote:
> On 2016/08/16 16:09:14, mtklein wrote:
> > > 2. I'm fine to add that additional constraint, but is this situation any
> > > different from SkImage_Gpu today, where the client must perform the final
> > unref
> > > happens in the proper thread?
> > 
> > If we unpin explicitly on the correct thread, the final unref can happy on
any
> > thread, right?
> 
> *happen

For raster images, yes. For SkImage_Gpu images as reed@ points out it cannot
(either with or without this change).

[reed1, 2016-08-16 16:31:55]

On 2016/08/16 16:08:13, reed1 wrote:
> On 2016/08/16 14:40:56, bsalomon wrote:
> > A couple things:
> > 
> > 1) I think we should check that the GrContext* passed in matches the
existing
> > texture (previous pin for raster if exists, and backing store for gpu). If
> they
> > don't match I guess fail?
> > 
> > 2) After talking to mtklein@ yesterday I think we should require that the
> client
> > explicitly unpin the texture before the image's ref count reaches zero (and
be
> > sure to unpin on safe thread) and assert that it is unpinned in ~SkImage.
> > Otherwise it is easy for the client to have the final unref occur in another
> > thread which would be unsafe.
> 
> 1. agreed. almost did it earlier, but will now.
> 
> 2. I'm fine to add that additional constraint, but is this situation any
> different from SkImage_Gpu today, where the client must perform the final
unref
> happens in the proper thread?

Actually, I think I already checked for #1

[bsalomon, 2016-08-16 16:36:47]

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Gpu.h
File src/image/SkImage_Gpu.h (right):

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Gpu.h...
src/image/SkImage_Gpu.h:48: return sk_ref_sp(fTexture.get());
Here do we know that this texture is in the right context?

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:205: sk_sp<GrTexture> tex =
this->refPinnedTexture(&uniqueID);
Doesn't #1 need to be checked here? i.e. that tex->getContext() == ctx?

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:234: if (fPinnedTexture) {
What if fPinnedTexture's context is not ctx?

[mtklein, 2016-08-16 16:39:53]

On 2016/08/16 16:21:00, bsalomon wrote:
> On 2016/08/16 16:13:24, mtklein wrote:
> > On 2016/08/16 16:09:14, mtklein wrote:
> > > > 2. I'm fine to add that additional constraint, but is this situation any
> > > > different from SkImage_Gpu today, where the client must perform the
final
> > > unref
> > > > happens in the proper thread?
> > > 
> > > If we unpin explicitly on the correct thread, the final unref can happy on
> any
> > > thread, right?
> > 
> > *happen
> 
> For raster images, yes. For SkImage_Gpu images as reed@ points out it cannot
> (either with or without this change).

I see.  I think Derek and I have a backup plan to make sure they're always
destroyed on the render thread.

[reed1, 2016-08-16 16:41:33]

https://codereview.chromium.org/2241353002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.h (right):

https://codereview.chromium.org/2241353002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.h:39: class GrImageTextureAdjuster : public
GrRawTextureAdjuster {
On 2016/08/15 19:10:39, bsalomon wrote:
> I wonder if we should make GrTextureAdjuster a final class now and not have
any
> subclasses. IIRC there used to be additional complexity with texture-backed
> SkBitmaps that is now gone.

Done.

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:205: sk_sp<GrTexture> tex =
this->refPinnedTexture(&uniqueID);
On 2016/08/16 16:36:47, bsalomon wrote:
> Doesn't #1 need to be checked here? i.e. that tex->getContext() == ctx?

Does this sort of check not already exist for all textures / texture-images?

https://codereview.chromium.org/2241353002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:234: if (fPinnedTexture) {
On 2016/08/16 16:36:47, bsalomon wrote:
> What if fPinnedTexture's context is not ctx?

Oops, missed it here. I was only looking at unpin. THanks.

[reed1, 2016-08-16 17:25:03]

ptal -- moved api to SkImagePriv.h, which derek should already have access to

[reed1, 2016-08-16 17:25:08]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 17:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 17:46:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 17:46:43]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-17 17:18:33]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-08-17 17:18:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-08-17 17:18:44]

ptal

[mtklein, 2016-08-17 17:28:10]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2016-08-17 17:28:11]

lgtm

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h
File src/core/SkImagePriv.h (right):

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
src/core/SkImagePriv.h:86: *  This must be balanced by an equal number of calls
to onUnpinAsTexture() -- calls can be
onUnpinAsTexture() -> SkImage_unpinAsTexture() ?

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
src/core/SkImagePriv.h:96: *  The balancing call to onPinAsTexture. When a
balanced number of calls have been made, then
ditto about on...()

[mtklein, 2016-08-17 17:28:52]

On 2016/08/17 17:28:11, mtklein wrote:
> lgtm
> 
> https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h
> File src/core/SkImagePriv.h (right):
> 
>
https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
> src/core/SkImagePriv.h:86: *  This must be balanced by an equal number of
calls
> to onUnpinAsTexture() -- calls can be
> onUnpinAsTexture() -> SkImage_unpinAsTexture() ?
> 
>
https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
> src/core/SkImagePriv.h:96: *  The balancing call to onPinAsTexture. When a
> balanced number of calls have been made, then
> ditto about on...()

That's lgtm with the caveat of most of the details looking right but unfamiliar.

[bsalomon, 2016-08-17 17:30:24]

lgtm

[reed1, 2016-08-17 17:31:02]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-08-17 17:35:34]

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h
File src/core/SkImagePriv.h (right):

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
src/core/SkImagePriv.h:86: *  This must be balanced by an equal number of calls
to onUnpinAsTexture() -- calls can be
On 2016/08/17 17:28:11, mtklein wrote:
> onUnpinAsTexture() -> SkImage_unpinAsTexture() ?

Done.

https://codereview.chromium.org/2241353002/diff/120001/src/core/SkImagePriv.h...
src/core/SkImagePriv.h:96: *  The balancing call to onPinAsTexture. When a
balanced number of calls have been made, then
On 2016/08/17 17:28:11, mtklein wrote:
> ditto about on...()

Done.

[reed1, 2016-08-17 17:35:40]

The CQ bit was checked by reed@google.com

[reed1, 2016-08-17 17:35:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com,
bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2241353002/#ps140001
(title: "fix comments")

[commit-bot: I haz the power, 2016-08-17 17:35:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 18:12:35]

Description was changed from

==========
pin as texture api

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241353002
==========

to

==========
pin as texture api

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241353002

Committed:
https://skia.googlesource.com/skia/+/2d5b7147032e3806b5895667a899440119707c2d
==========

[commit-bot: I haz the power, 2016-08-17 18:12:37]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/2d5b7147032e3806b5895667a899440119707c2d