gpu-raster: Check for NULL sk_surface in GpuRasterizer::RasterizeSource.

gpu-raster: Check for NULL sk_surface in GpuRasterizer::RasterizeSource.

When the GL Context has been lost, GrContext::wrapBackendTexture returns NULL and ScopedWriteLockGr::InitSkSurface fails to initialize the SkSurface.

In this case, skip trying to draw to the SkSurface's canvas.

BUG=464933
Committed: https://crrev.com/4cf8d357f19f4f5c4a8628ef43d352f8e003226a
Cr-Commit-Position: refs/heads/master@{#320153}

[vmiura, 2015-03-07 00:33:04]

vmiura@chromium.org changed reviewers:
+ hendrikw@chromium.org, vmpstr@chromium.org

[vmiura, 2015-03-07 00:33:04]

PTAL

[vmiura, 2015-03-07 00:34:35]

Patchset #1 (id:1) has been deleted

[danakj, 2015-03-07 00:35:50]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-03-07 00:35:50]

What can make it be null? Do we want to just show white/black but pretend we
rastered in that case?

[vmiura, 2015-03-07 00:36:11]

Patchset #1 (id:20001) has been deleted

[vmiura, 2015-03-07 00:39:14]

On 2015/03/07 00:35:50, danakj wrote:
> What can make it be null? Do we want to just show white/black but pretend we
> rastered in that case?

The case I'm seeing is after context lost.

We are wrapping an existing resource so don't expect it to fail normally..?

[vmiura, 2015-03-07 04:05:30]

On 2015/03/07 00:39:14, vmiura wrote:
> On 2015/03/07 00:35:50, danakj wrote:
> > What can make it be null? Do we want to just show white/black but pretend we
> > rastered in that case?
> 
> The case I'm seeing is after context lost.
> 
> We are wrapping an existing resource so don't expect it to fail normally..?

This matches what we did before I think.  We pretend we rasterized the tile.

[hendrikw, 2015-03-09 15:53:27]

On 2015/03/07 04:05:30, vmiura wrote:
> On 2015/03/07 00:39:14, vmiura wrote:
> > On 2015/03/07 00:35:50, danakj wrote:
> > > What can make it be null? Do we want to just show white/black but pretend
we
> > > rastered in that case?
> > 
> > The case I'm seeing is after context lost.
> > 
> > We are wrapping an existing resource so don't expect it to fail normally..?
> 
> This matches what we did before I think.  We pretend we rasterized the tile.

I tested out this change with the failure on windows.  LGTM.

[danakj, 2015-03-09 16:38:04]

https://codereview.chromium.org/988953002/diff/40001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/988953002/diff/40001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:125: if (!sk_surface)
Can you add a comment explaining this is for lost context case since it's not
obvious?

[vmiura, 2015-03-09 18:01:26]

PTAL

https://codereview.chromium.org/988953002/diff/40001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/988953002/diff/40001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:125: if (!sk_surface)
On 2015/03/09 16:38:03, danakj wrote:
> Can you add a comment explaining this is for lost context case since it's not
> obvious?

Done.

[danakj, 2015-03-09 18:04:46]

LGTM thanks

[danakj, 2015-03-09 18:04:59]

btw is this something we can unit test? should be possible

[vmiura, 2015-03-09 21:08:03]

Patchset #3 (id:80001) has been deleted

[vmiura, 2015-03-09 21:16:41]

vmiura@chromium.org changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[vmiura, 2015-03-09 21:16:42]

piman@ / sievers@: Please take a look at context_provider_command_buffer.cc


Added unit test for context lost case.  Also noticed an
inconsistency with passing lost context state to GrContext.

GrContext is initialized lazily by the ContextProvider, and
it's possible that it was initialized after context was
already lost.  In that case we did not abandon GrContext.

In theory, GrContext will still work after lost context,
but for consistency I think it's better to abandon it.

[no sievers, 2015-03-09 21:25:48]

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:139:
gr_context_->OnLostContext();
Can abandonContext() safely be called more than once?

Can we maybe call it directly here?
The OnLostContext() callbacks are usually only called once.

[vmiura, 2015-03-09 21:42:52]

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

[vmiura, 2015-03-09 21:42:52]

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:139:
gr_context_->OnLostContext();
On 2015/03/09 21:25:48, sievers wrote:
> Can abandonContext() safely be called more than once?
> 
> Can we maybe call it directly here?
> The OnLostContext() callbacks are usually only called once.

bsalomon@: Is it safe to call abandonContext() more than once?

sievers@: I guess we don't expect ContextProviderCommandBuffer::OnLostCallback()
to be called again after this point.  However I see that
ContextProviderCommandBuffer::VerifyContexts() calls OnLostContext() as well so
if anything was using that, it could happen multiple times.

[piman, 2015-03-09 22:22:05]

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:138: if
(gr_context_ && IsContextLost())
nit: no need to test for gr_contex_, it's !NULL here (new asserts rather than
returning NULL).

Should we return NULL rather than an unusable GrContext?

[vmiura, 2015-03-09 22:39:02]

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:138: if
(gr_context_ && IsContextLost())
On 2015/03/09 22:22:05, piman (Very slow to review) wrote:
> nit: no need to test for gr_contex_, it's !NULL here (new asserts rather than
> returning NULL).
> 
> Should we return NULL rather than an unusable GrContext?

Ok, I'll remove the check.

I think lost context should not cause us to return NULL GrContext as lots of our
code can not handle that case.

[bsalomon, 2015-03-09 22:58:02]

On 2015/03/09 21:42:52, vmiura wrote:
>
https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
> File content/common/gpu/client/context_provider_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
> content/common/gpu/client/context_provider_command_buffer.cc:139:
> gr_context_->OnLostContext();
> On 2015/03/09 21:25:48, sievers wrote:
> > Can abandonContext() safely be called more than once?
> > 
> > Can we maybe call it directly here?
> > The OnLostContext() callbacks are usually only called once.
> 
> bsalomon@: Is it safe to call abandonContext() more than once?

Yup, should be safe.

> 
> sievers@: I guess we don't expect
ContextProviderCommandBuffer::OnLostCallback()
> to be called again after this point.  However I see that
> ContextProviderCommandBuffer::VerifyContexts() calls OnLostContext() as well
so
> if anything was using that, it could happen multiple times.

[vmiura, 2015-03-09 23:10:01]

PTAL

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:138: if
(gr_context_ && IsContextLost())
On 2015/03/09 22:39:01, vmiura wrote:
> On 2015/03/09 22:22:05, piman (Very slow to review) wrote:
> > nit: no need to test for gr_contex_, it's !NULL here (new asserts rather
than
> > returning NULL).
> > 
> > Should we return NULL rather than an unusable GrContext?
> 
> Ok, I'll remove the check.
> 
> I think lost context should not cause us to return NULL GrContext as lots of
our
> code can not handle that case.

Extra NULL check remove Done.

https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:139:
gr_context_->OnLostContext();
Calling abandonContext() directly Done.

[vmiura, 2015-03-11 17:49:41]

On 2015/03/09 23:10:01, vmiura wrote:
> PTAL
> 
>
https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
> File content/common/gpu/client/context_provider_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
> content/common/gpu/client/context_provider_command_buffer.cc:138: if
> (gr_context_ && IsContextLost())
> On 2015/03/09 22:39:01, vmiura wrote:
> > On 2015/03/09 22:22:05, piman (Very slow to review) wrote:
> > > nit: no need to test for gr_contex_, it's !NULL here (new asserts rather
> than
> > > returning NULL).
> > > 
> > > Should we return NULL rather than an unusable GrContext?
> > 
> > Ok, I'll remove the check.
> > 
> > I think lost context should not cause us to return NULL GrContext as lots of
> our
> > code can not handle that case.
> 
> Extra NULL check remove Done.
> 
>
https://codereview.chromium.org/988953002/diff/100001/content/common/gpu/clie...
> content/common/gpu/client/context_provider_command_buffer.cc:139:
> gr_context_->OnLostContext();
> Calling abandonContext() directly Done.

Gentle poke for review of
content/common/gpu/client/context_provider_command_buffer.cc.

We need to get a fix for the crash merged to M42 branch.

[piman, 2015-03-11 18:20:16]

lgtm

[vmiura, 2015-03-11 19:16:52]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2015-03-11 19:16:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
hendrikw@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/988953002/#ps120001
(title: "Fix suggestions.")

[commit-bot: I haz the power, 2015-03-11 19:38:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988953002/120001

[commit-bot: I haz the power, 2015-03-11 21:50:12]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-03-11 21:51:29]

Patchset 4 (id:??) landed as
https://crrev.com/4cf8d357f19f4f5c4a8628ef43d352f8e003226a
Cr-Commit-Position: refs/heads/master@{#320153}