Use SkSurface and SkImage instead of SkGpuDevice and SkBitmap in cc.

Use SkSurface and SkImage instead of SkGpuDevice and SkBitmap in cc.

Committed: https://crrev.com/55ebb77d934328f027c2dc5c5cd2f7739eb3cd50
Cr-Commit-Position: refs/heads/master@{#293553}

[bsalomon, 2014-08-29 18:22:08]

bsalomon@google.com changed reviewers:
+ enne@chromium.org, senorblanco@chromium.org

[bsalomon, 2014-08-29 18:22:09]

I want to make SkGpuDevice a private subclass in Skia. These are the last
remaining users of it in Chromium/Blink.

We also have a long term goal of removing texture-backed SkBitmaps. This removes
a couple uses of them.

[enne (OOO), 2014-08-29 19:05:40]

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:802: SkImage* image = surface->newImageSnapshot();
After fixing numerous memory leaks, I'm a little allergic to raw pointers from
Skia in Chromium.  Can you wrap all the SkImage* in this patch in skia::RefPtr
via AdoptRef or SharePtr?

[bsalomon, 2014-08-29 20:47:33]

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:802: SkImage* image = surface->newImageSnapshot();
On 2014/08/29 19:05:40, enne wrote:
> After fixing numerous memory leaks, I'm a little allergic to raw pointers from
> Skia in Chromium.  Can you wrap all the SkImage* in this patch in skia::RefPtr
> via AdoptRef or SharePtr?

I tried in the new patch. I've never used these before so lemme know if I'm
messing it up.

[enne (OOO), 2014-08-29 21:20:21]

lgtm, although senorblanco should definitely take a look too.

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/520793002/diff/20001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:802: SkImage* image = surface->newImageSnapshot();
On 2014/08/29 20:47:33, bsalomon wrote:
> On 2014/08/29 19:05:40, enne wrote:
> > After fixing numerous memory leaks, I'm a little allergic to raw pointers
from
> > Skia in Chromium.  Can you wrap all the SkImage* in this patch in
skia::RefPtr
> > via AdoptRef or SharePtr?
> 
> I tried in the new patch. I've never used these before so lemme know if I'm
> messing it up.

Thanks.  Those usages look right to me, based on what I'm guessing the Skia
functions are doing.

[bsalomon, 2014-09-02 21:17:28]

bsalomon@google.com changed reviewers:
+ sugoi@chromium.org

[bsalomon, 2014-09-02 21:17:29]

Alexis, do you feel comfortable reviewing this in Stephen's absence or know if
there is anyone familiar with the image filter code who would?

[sugoi1, 2014-09-03 12:11:43]

On 2014/09/02 21:17:29, bsalomon wrote:
> Alexis, do you feel comfortable reviewing this in Stephen's absence or know if
> there is anyone familiar with the image filter code who would?

I only have 1 question: how does the existence of an SkImage object guarantees
that it has a texture? All the NULL pointer checks for textures have been
removed and changed to simply check for the SkImage object. Looking at the code,
it's not obvious to me that no error can possibly occur that would make the
texture NULL somehow. Or maybe we just want to crash if that's the case. In any
case, could you just explain this quickly?

[bsalomon, 2014-09-03 17:33:58]

On 2014/09/03 12:11:43, sugoi1 wrote:
> On 2014/09/02 21:17:29, bsalomon wrote:
> > Alexis, do you feel comfortable reviewing this in Stephen's absence or know
if
> > there is anyone familiar with the image filter code who would?
> 
> I only have 1 question: how does the existence of an SkImage object guarantees
> that it has a texture? All the NULL pointer checks for textures have been
> removed and changed to simply check for the SkImage object. Looking at the
code,
> it's not obvious to me that no error can possibly occur that would make the
> texture NULL somehow. Or maybe we just want to crash if that's the case. In
any
> case, could you just explain this quickly?

I *think* the textures could never be NULL the way the surfaces and images are
used as the code is, but when I started to explain it I started thinking it was
a fragile assumption. So I added checks for NULL getTexture()s.

[sugoi1, 2014-09-03 18:10:46]

On 2014/09/03 17:33:58, bsalomon wrote:
> On 2014/09/03 12:11:43, sugoi1 wrote:
> > On 2014/09/02 21:17:29, bsalomon wrote:
> > > Alexis, do you feel comfortable reviewing this in Stephen's absence or
know
> if
> > > there is anyone familiar with the image filter code who would?
> > 
> > I only have 1 question: how does the existence of an SkImage object
guarantees
> > that it has a texture? All the NULL pointer checks for textures have been
> > removed and changed to simply check for the SkImage object. Looking at the
> code,
> > it's not obvious to me that no error can possibly occur that would make the
> > texture NULL somehow. Or maybe we just want to crash if that's the case. In
> any
> > case, could you just explain this quickly?
> 
> I *think* the textures could never be NULL the way the surfaces and images are
> used as the code is, but when I started to explain it I started thinking it
was
> a fragile assumption. So I added checks for NULL getTexture()s.

LGTM, I think this is safe enough for now.

[bsalomon, 2014-09-03 18:11:47]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-09-03 18:12:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bsalomon@google.com/520793002/60001

[danakj, 2014-09-03 19:11:00]

On 2014/09/03 17:33:58, bsalomon wrote:
> On 2014/09/03 12:11:43, sugoi1 wrote:
> > On 2014/09/02 21:17:29, bsalomon wrote:
> > > Alexis, do you feel comfortable reviewing this in Stephen's absence or
know
> if
> > > there is anyone familiar with the image filter code who would?
> > 
> > I only have 1 question: how does the existence of an SkImage object
guarantees
> > that it has a texture? All the NULL pointer checks for textures have been
> > removed and changed to simply check for the SkImage object. Looking at the
> code,
> > it's not obvious to me that no error can possibly occur that would make the
> > texture NULL somehow. Or maybe we just want to crash if that's the case. In
> any
> > case, could you just explain this quickly?
> 
> I *think* the textures could never be NULL the way the surfaces and images are
> used as the code is, but when I started to explain it I started thinking it
was
> a fragile assumption. So I added checks for NULL getTexture()s.

Will you revisit these null checks? I don't like having null checks when we're
not sure what's going on. Can you add TODO for them if so?

[danakj, 2014-09-03 19:11:09]

The CQ bit was unchecked by danakj@chromium.org

[bsalomon, 2014-09-03 19:24:11]

On 2014/09/03 19:11:00, danakj wrote:
> On 2014/09/03 17:33:58, bsalomon wrote:
> > On 2014/09/03 12:11:43, sugoi1 wrote:
> > > On 2014/09/02 21:17:29, bsalomon wrote:
> > > > Alexis, do you feel comfortable reviewing this in Stephen's absence or
> know
> > if
> > > > there is anyone familiar with the image filter code who would?
> > > 
> > > I only have 1 question: how does the existence of an SkImage object
> guarantees
> > > that it has a texture? All the NULL pointer checks for textures have been
> > > removed and changed to simply check for the SkImage object. Looking at the
> > code,
> > > it's not obvious to me that no error can possibly occur that would make
the
> > > texture NULL somehow. Or maybe we just want to crash if that's the case.
In
> > any
> > > case, could you just explain this quickly?
> > 
> > I *think* the textures could never be NULL the way the surfaces and images
are
> > used as the code is, but when I started to explain it I started thinking it
> was
> > a fragile assumption. So I added checks for NULL getTexture()s.
> 
> Will you revisit these null checks? I don't like having null checks when we're
> not sure what's going on. Can you add TODO for them if so?

I wasn't planning to. In general it is possible for SkImage to have a NULL
texture. So I don't think it is fundamentally wrong to check for NULL. I might
be able to convince myself that they're unnecessary here based on how the
SkSurfaces and SkImags are used, but even if so I think it's a fragile
assumption that could be invalidated by a Skia-only change.

[danakj, 2014-09-03 19:34:36]

On Wed, Sep 3, 2014 at 3:24 PM, bsalomon via cc-bugs <cc-bugs@chromium.org>
wrote:

> On 2014/09/03 19:11:00, danakj wrote:
>
>> On 2014/09/03 17:33:58, bsalomon wrote:
>> > On 2014/09/03 12:11:43, sugoi1 wrote:
>> > > On 2014/09/02 21:17:29, bsalomon wrote:
>> > > > Alexis, do you feel comfortable reviewing this in Stephen's absence
>> or
>> know
>> > if
>> > > > there is anyone familiar with the image filter code who would?
>> > >
>> > > I only have 1 question: how does the existence of an SkImage object
>> guarantees
>> > > that it has a texture? All the NULL pointer checks for textures have
>> been
>> > > removed and changed to simply check for the SkImage object. Looking
>> at the
>> > code,
>> > > it's not obvious to me that no error can possibly occur that would
>> make
>>
> the
>
>> > > texture NULL somehow. Or maybe we just want to crash if that's the
>> case.
>>
> In
>
>> > any
>> > > case, could you just explain this quickly?
>> >
>> > I *think* the textures could never be NULL the way the surfaces and
>> images
>>
> are
>
>> > used as the code is, but when I started to explain it I started
>> thinking it
>> was
>> > a fragile assumption. So I added checks for NULL getTexture()s.
>>
>
>  Will you revisit these null checks? I don't like having null checks when
>> we're
>> not sure what's going on. Can you add TODO for them if so?
>>
>
> I wasn't planning to. In general it is possible for SkImage to have a NULL
> texture. So I don't think it is fundamentally wrong to check for NULL. I
> might
> be able to convince myself that they're unnecessary here based on how the
> SkSurfaces and SkImags are used, but even if so I think it's a fragile
> assumption that could be invalidated by a Skia-only change.
>

Hm, but it seems like cc would be able to know if the SkImage has a texture
earlier, when it decided to use the SkImage at all?

For instance in
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
, ApplyImageFilter will return an empty SkBitmap in cases where it had an
error. When it returns a non-empty SkBitmap, it will have done SkGpuDevice
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...>
::Create
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...,
and drawn things into it. Can it still have a null texture at that point?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bsalomon, 2014-09-04 21:27:02]

On 2014/09/03 19:34:36, danakj wrote:
> On Wed, Sep 3, 2014 at 3:24 PM, bsalomon via cc-bugs
<mailto:cc-bugs@chromium.org>
> wrote:
> 
> > On 2014/09/03 19:11:00, danakj wrote:
> >
> >> On 2014/09/03 17:33:58, bsalomon wrote:
> >> > On 2014/09/03 12:11:43, sugoi1 wrote:
> >> > > On 2014/09/02 21:17:29, bsalomon wrote:
> >> > > > Alexis, do you feel comfortable reviewing this in Stephen's absence
> >> or
> >> know
> >> > if
> >> > > > there is anyone familiar with the image filter code who would?
> >> > >
> >> > > I only have 1 question: how does the existence of an SkImage object
> >> guarantees
> >> > > that it has a texture? All the NULL pointer checks for textures have
> >> been
> >> > > removed and changed to simply check for the SkImage object. Looking
> >> at the
> >> > code,
> >> > > it's not obvious to me that no error can possibly occur that would
> >> make
> >>
> > the
> >
> >> > > texture NULL somehow. Or maybe we just want to crash if that's the
> >> case.
> >>
> > In
> >
> >> > any
> >> > > case, could you just explain this quickly?
> >> >
> >> > I *think* the textures could never be NULL the way the surfaces and
> >> images
> >>
> > are
> >
> >> > used as the code is, but when I started to explain it I started
> >> thinking it
> >> was
> >> > a fragile assumption. So I added checks for NULL getTexture()s.
> >>
> >
> >  Will you revisit these null checks? I don't like having null checks when
> >> we're
> >> not sure what's going on. Can you add TODO for them if so?
> >>
> >
> > I wasn't planning to. In general it is possible for SkImage to have a NULL
> > texture. So I don't think it is fundamentally wrong to check for NULL. I
> > might
> > be able to convince myself that they're unnecessary here based on how the
> > SkSurfaces and SkImags are used, but even if so I think it's a fragile
> > assumption that could be invalidated by a Skia-only change.
> >
> 
> Hm, but it seems like cc would be able to know if the SkImage has a texture
> earlier, when it decided to use the SkImage at all?
> 
> For instance in
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
> , ApplyImageFilter will return an empty SkBitmap in cases where it had an
> error. When it returns a non-empty SkBitmap, it will have done SkGpuDevice
>
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...>
> ::Create
>
<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...,
> and drawn things into it. Can it still have a null texture at that point?

In the current implementation of newImageSnapshot() it is not possible for valid
gpu-backed SkSurface to return either a NULL SkImage or an SkImage with a NULL
texture. But it is possible that that could change in the future. I modified the
code to do the NULL check before returning SkImage in Apply*(). So the caller
can know that if a non-NULL SkImage was returned then it contains a valid
texture. PTAL.

[danakj, 2014-09-04 21:35:16]

Thanks, that looks a lot better to me. LGTM

[danakj, 2014-09-04 21:35:28]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-04 21:37:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bsalomon@google.com/520793002/80001

[commit-bot: I haz the power, 2014-09-04 22:40:06]

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

[commit-bot: I haz the power, 2014-09-04 22:40:07]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bsalomon, 2014-09-05 17:58:29]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-09-05 18:00:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bsalomon@google.com/520793002/80001

[commit-bot: I haz the power, 2014-09-05 18:02:37]

Committed patchset #5 (id:80001) as 37ffe8f81510bbfb82b48640aa96fe7e51efc654

[bsalomon, 2014-09-05 19:52:58]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/546073002/ by bsalomon@google.com.

The reason for reverting is: Looks like it may have broken the layout tests.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7/builds/....

[Stephen White, 2014-09-09 14:10:31]

Some post-commit comments.

https://codereview.chromium.org/520793002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/520793002/diff/80001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:673: skia::RefPtr<SkCanvas> canvas =
skia::SharePtr(surface->getCanvas());
Nit: does this really need to be a RefPtr? Doesn't this canvas live at
least as long as the surface that returns it? If so, I'd actually prefer 
a bare ptr here, since SharePtr() is an uncommon idiom.

https://codereview.chromium.org/520793002/diff/80001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:796: skia::RefPtr<SkCanvas> canvas =
skia::SharePtr(surface->getCanvas());
Same here.

https://codereview.chromium.org/520793002/diff/80001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1000: skia::RefPtr<SkImage> filter_bitmap;
Naming nit: perhaps this should be renamed filter_image, now that it's 
no longer an SkBitmap.

[commit-bot: I haz the power, 2014-09-10 03:40:41]

Patchset 5 (id:??) landed as
https://crrev.com/55ebb77d934328f027c2dc5c5cd2f7739eb3cd50
Cr-Commit-Position: refs/heads/master@{#293553}