cc: Add null checks for GrContext created by ContextProviderCommandBuffer.

cc: Add null checks for GrContext created by ContextProviderCommandBuffer.

GrContext::Create may return NULL if the base GL context was lost.

 - Check for the NULL case in GlRenderer.
 - For the GPU rasterizer, don't allow GPU rasterization to be initialized
   with a NULL GrContext.

BUG=464892
Committed: https://crrev.com/4e7e199cb73c95ee81c0804cf14a3d6ab015c75c
Cr-Commit-Position: refs/heads/master@{#330104}

[vmiura, 2015-05-12 20:35:23]

vmiura@chromium.org changed reviewers:
+ bsalomon@chromium.org, danakj@chromium.org

[vmiura, 2015-05-12 20:35:24]

PTAL.

Dana, do we have a GLRenderer ApplyImageFilter test where I could add a context
lost case?

[vmiura, 2015-05-12 21:13:52]

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

[vmiura, 2015-05-12 21:13:53]

sievers@chromium.org: Please review changes in 
 content/common/gpu/client/context_provider_command_buffer.cc

[bsalomon, 2015-05-12 21:18:37]

On 2015/05/12 20:35:24, vmiura wrote:
> PTAL.
> 
> Dana, do we have a GLRenderer ApplyImageFilter test where I could add a
context
> lost case?

FWIW, this lgtm.

[danakj, 2015-05-12 21:19:13]

So may if statements everywhere, this is making me sad. We're sure to miss
places in the future where we used a GrContext and shoulda checked if it is
valid first. D:

Can we do this in one true place earlier somehow? Can we make the GrContext not
be null and fail more gracefully when context is lost?

[vmiura, 2015-05-12 21:57:56]

On 2015/05/12 21:19:13, danakj wrote:
> So may if statements everywhere, this is making me sad. We're sure to miss
> places in the future where we used a GrContext and shoulda checked if it is
> valid first. D:
> 
> Can we do this in one true place earlier somehow? Can we make the GrContext
not
> be null and fail more gracefully when context is lost?

It seems to me it would have to be at the Skia level, for example instead of
GrContext::Create() returning null, returning an abandoned GrContext.

[danakj, 2015-05-12 22:00:22]

Can we do that?

On Tue, May 12, 2015 at 2:57 PM, <vmiura@chromium.org> wrote:

> On 2015/05/12 21:19:13, danakj wrote:
>
>> So may if statements everywhere, this is making me sad. We're sure to miss
>> places in the future where we used a GrContext and shoulda checked if it
>> is
>> valid first. D:
>>
>
>  Can we do this in one true place earlier somehow? Can we make the
>> GrContext
>>
> not
>
>> be null and fail more gracefully when context is lost?
>>
>
> It seems to me it would have to be at the Skia level, for example instead
> of
> GrContext::Create() returning null, returning an abandoned GrContext.
>
>
>
> https://codereview.chromium.org/1135743004/
>

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

[piman, 2015-05-12 22:06:10]

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

[piman, 2015-05-12 22:06:11]

https://codereview.chromium.org/1135743004/diff/1/content/common/gpu/client/c...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1135743004/diff/1/content/common/gpu/client/c...
content/common/gpu/client/context_provider_command_buffer.cc:143: // GrContext
creation should not fail with a valid context.
I'm not sure that's true. IsContextLost() is fundamentally racy, the context
might be lost right after it returned false.

A lost context is still a usable c++ object with all methods being functional
(if only return errors and failing GL calls). Couldn't we do the same with
GrContext?

[vmiura, 2015-05-12 23:09:40]

GrContext can work as a usable C++ object after it's created, and
it's able to become mostly a no-op after we call ::abandonContext().

The difficulty seems to be with the GrContext::Create, and speaking
with bsalomon@ it sounds non-trivial and he would like to make it
a goal for m45.

After discussing with piman@, I've changed things so for GPU raster
we create and check the GrContext in LTHI::InitializeRenderer() which
removes additional NULL checks elsewhere, however GlRenderer uses
GrContext for image filters and rather than always creating the
GrContext for GlRenderer, I've kept the check inside ScopedUseGrContext.

PTAL

[piman, 2015-05-12 23:33:51]

LGTM + nits

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2173: if (!context_provider)
nit: you can probably DCHECK this.

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

https://codereview.chromium.org/1135743004/diff/20001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:140: return
nullptr;
I'd skip this part. No use trying to optimize for this case.

[danakj, 2015-05-12 23:40:43]

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

https://codereview.chromium.org/1135743004/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:166: // GrContext for filters is created lazily, and
may fail.
Can you file a bug about making GrContext::Create not return null and point to
it here?

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
This can be changed by CreateAndSetRenderer below and suddenly become true I
think (I was hoping it was a one-way trip from true to false but the code
doesn't look like that to me.)

But maybe you can make UpdateGpuRasterizationStatus() before this. It's using
renderercaps but only as a proxy for the contextprovider caps. It's also
recreating layer resources and stuff which would maybe need to be split into
another function to run at the time when it currently is.

[danakj, 2015-05-12 23:40:58]

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

[danakj, 2015-05-12 23:40:59]

+senorblanco

[vmiura, 2015-05-13 02:53:00]

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
On 2015/05/12 23:40:43, danakj wrote:
> This can be changed by CreateAndSetRenderer below and suddenly become true I
> think (I was hoping it was a one-way trip from true to false but the code
> doesn't look like that to me.)
> 
> But maybe you can make UpdateGpuRasterizationStatus() before this. It's using
> renderercaps but only as a proxy for the contextprovider caps. It's also
> recreating layer resources and stuff which would maybe need to be split into
> another function to run at the time when it currently is.

Thanks for spotting this.

Hmm, I think it could also flip from false to true in
UpdateGpuRasterizationStatus()
via CommitComplete().

This doesn't look good to me anymore; we can handle failure in
InitializeRenderer() but after that I think the code assumes that
UpdateGpuRasterizationStatus() succeeds.

I'm thinking it's safer to back to "ifs" in the several places where GrContext()
is used, with big TODOs to remove them when GrContext can handle failure.

[piman, 2015-05-13 03:28:36]

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
On 2015/05/13 02:53:00, vmiura wrote:
> On 2015/05/12 23:40:43, danakj wrote:
> > This can be changed by CreateAndSetRenderer below and suddenly become true I
> > think (I was hoping it was a one-way trip from true to false but the code
> > doesn't look like that to me.)
> > 
> > But maybe you can make UpdateGpuRasterizationStatus() before this. It's
using
> > renderercaps but only as a proxy for the contextprovider caps. It's also
> > recreating layer resources and stuff which would maybe need to be split into
> > another function to run at the time when it currently is.
> 
> Thanks for spotting this.
> 
> Hmm, I think it could also flip from false to true in
> UpdateGpuRasterizationStatus()
> via CommitComplete().
> 
> This doesn't look good to me anymore; we can handle failure in
> InitializeRenderer() but after that I think the code assumes that
> UpdateGpuRasterizationStatus() succeeds.
> 
> I'm thinking it's safer to back to "ifs" in the several places where
GrContext()
> is used, with big TODOs to remove them when GrContext can handle failure.

We can simply rely on settings_.gpu_rasterization_enabled instead. If we /may/
use gpu rasterization, we should check the GrContext. Otherwise, we don't care.

[Stephen White, 2015-05-13 10:10:14]

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
On 2015/05/13 03:28:36, piman (Very slow to review) wrote:
> On 2015/05/13 02:53:00, vmiura wrote:
> > On 2015/05/12 23:40:43, danakj wrote:
> > > This can be changed by CreateAndSetRenderer below and suddenly become true
I
> > > think (I was hoping it was a one-way trip from true to false but the code
> > > doesn't look like that to me.)
> > > 
> > > But maybe you can make UpdateGpuRasterizationStatus() before this. It's
> using
> > > renderercaps but only as a proxy for the contextprovider caps. It's also
> > > recreating layer resources and stuff which would maybe need to be split
into
> > > another function to run at the time when it currently is.
> > 
> > Thanks for spotting this.
> > 
> > Hmm, I think it could also flip from false to true in
> > UpdateGpuRasterizationStatus()
> > via CommitComplete().
> > 
> > This doesn't look good to me anymore; we can handle failure in
> > InitializeRenderer() but after that I think the code assumes that
> > UpdateGpuRasterizationStatus() succeeds.
> > 
> > I'm thinking it's safer to back to "ifs" in the several places where
> GrContext()
> > is used, with big TODOs to remove them when GrContext can handle failure.
> 
> We can simply rely on settings_.gpu_rasterization_enabled instead. If we /may/
> use gpu rasterization, we should check the GrContext. Otherwise, we don't
care.

In that case, you should probably check settings_.gpu_rasterization_forced as
well.

[danakj, 2015-05-13 16:53:17]

On Tue, May 12, 2015 at 8:28 PM, <piman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> On 2015/05/13 02:53:00, vmiura wrote:
>
>> On 2015/05/12 23:40:43, danakj wrote:
>> > This can be changed by CreateAndSetRenderer below and suddenly
>>
> become true I
>
>> > think (I was hoping it was a one-way trip from true to false but the
>>
> code
>
>> > doesn't look like that to me.)
>> >
>> > But maybe you can make UpdateGpuRasterizationStatus() before this.
>>
> It's using
>
>> > renderercaps but only as a proxy for the contextprovider caps. It's
>>
> also
>
>> > recreating layer resources and stuff which would maybe need to be
>>
> split into
>
>> > another function to run at the time when it currently is.
>>
>
>  Thanks for spotting this.
>>
>
>  Hmm, I think it could also flip from false to true in
>> UpdateGpuRasterizationStatus()
>> via CommitComplete().
>>
>
>  This doesn't look good to me anymore; we can handle failure in
>> InitializeRenderer() but after that I think the code assumes that
>> UpdateGpuRasterizationStatus() succeeds.
>>
>
>  I'm thinking it's safer to back to "ifs" in the several places where
>>
> GrContext()
>
>> is used, with big TODOs to remove them when GrContext can handle
>>
> failure.
>
> We can simply rely on settings_.gpu_rasterization_enabled instead. If we
> /may/ use gpu rasterization, we should check the GrContext. Otherwise,
> we don't care.
>

SGTM

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

[vmiura, 2015-05-13 17:19:30]

On 2015/05/13 16:53:17, danakj wrote:
> On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > File cc/trees/layer_tree_host_impl.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> > On 2015/05/13 02:53:00, vmiura wrote:
> >
> >> On 2015/05/12 23:40:43, danakj wrote:
> >> > This can be changed by CreateAndSetRenderer below and suddenly
> >>
> > become true I
> >
> >> > think (I was hoping it was a one-way trip from true to false but the
> >>
> > code
> >
> >> > doesn't look like that to me.)
> >> >
> >> > But maybe you can make UpdateGpuRasterizationStatus() before this.
> >>
> > It's using
> >
> >> > renderercaps but only as a proxy for the contextprovider caps. It's
> >>
> > also
> >
> >> > recreating layer resources and stuff which would maybe need to be
> >>
> > split into
> >
> >> > another function to run at the time when it currently is.
> >>
> >
> >  Thanks for spotting this.
> >>
> >
> >  Hmm, I think it could also flip from false to true in
> >> UpdateGpuRasterizationStatus()
> >> via CommitComplete().
> >>
> >
> >  This doesn't look good to me anymore; we can handle failure in
> >> InitializeRenderer() but after that I think the code assumes that
> >> UpdateGpuRasterizationStatus() succeeds.
> >>
> >
> >  I'm thinking it's safer to back to "ifs" in the several places where
> >>
> > GrContext()
> >
> >> is used, with big TODOs to remove them when GrContext can handle
> >>
> > failure.
> >
> > We can simply rely on settings_.gpu_rasterization_enabled instead. If we
> > /may/ use gpu rasterization, we should check the GrContext. Otherwise,
> > we don't care.
> >
> 
> SGTM

Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU on ~22%
of page views on supported hardware, but this would increase it to 100%.

We want to remove has_gpu_rasterization_trigger_ in the near term, so I think
that's fine, but slightly more concerned about potentially merging that to M43.

[Stephen White, 2015-05-13 17:44:24]

On 2015/05/13 17:19:30, vmiura wrote:
> On 2015/05/13 16:53:17, danakj wrote:
> > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
> > 
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > > File cc/trees/layer_tree_host_impl.cc (right):
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> > > On 2015/05/13 02:53:00, vmiura wrote:
> > >
> > >> On 2015/05/12 23:40:43, danakj wrote:
> > >> > This can be changed by CreateAndSetRenderer below and suddenly
> > >>
> > > become true I
> > >
> > >> > think (I was hoping it was a one-way trip from true to false but the
> > >>
> > > code
> > >
> > >> > doesn't look like that to me.)
> > >> >
> > >> > But maybe you can make UpdateGpuRasterizationStatus() before this.
> > >>
> > > It's using
> > >
> > >> > renderercaps but only as a proxy for the contextprovider caps. It's
> > >>
> > > also
> > >
> > >> > recreating layer resources and stuff which would maybe need to be
> > >>
> > > split into
> > >
> > >> > another function to run at the time when it currently is.
> > >>
> > >
> > >  Thanks for spotting this.
> > >>
> > >
> > >  Hmm, I think it could also flip from false to true in
> > >> UpdateGpuRasterizationStatus()
> > >> via CommitComplete().
> > >>
> > >
> > >  This doesn't look good to me anymore; we can handle failure in
> > >> InitializeRenderer() but after that I think the code assumes that
> > >> UpdateGpuRasterizationStatus() succeeds.
> > >>
> > >
> > >  I'm thinking it's safer to back to "ifs" in the several places where
> > >>
> > > GrContext()
> > >
> > >> is used, with big TODOs to remove them when GrContext can handle
> > >>
> > > failure.
> > >
> > > We can simply rely on settings_.gpu_rasterization_enabled instead. If we
> > > /may/ use gpu rasterization, we should check the GrContext. Otherwise,
> > > we don't care.
> > >
> > 
> > SGTM
> 
> Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU on
~22%
> of page views on supported hardware, but this would increase it to 100%.
> 
> We want to remove has_gpu_rasterization_trigger_ in the near term, so I think
> that's fine, but slightly more concerned about potentially merging that to
M43.

I think the idea of splitting UpdateGpuRasterizationStatus() into two would
work.
We could then do the first part (updating use_gpu_rasterization_, use_msaa_ and
status) automatically when the viewport/content triggers are set, and just set
a bool that the tree resources are dirty and need to be recreated on draw (or
maybe 
just go ahead and ReleaseTreeResources() etc, and use their absence as a signal
to 
recreate them on draw? I'm not familiar enough with that code to know if that
would 
be correct / safe).

Then you could safely check use_gpu_rasterization_ as a signal to check the
GrContext, 
as you were before.

Some unit tests would probably have to be changed, since they assume that 
UpdateGpuRasterizationStatus() updates the tree resources.

[vmiura, 2015-05-13 17:57:34]

On 2015/05/13 17:44:24, Stephen White wrote:
> On 2015/05/13 17:19:30, vmiura wrote:
> > On 2015/05/13 16:53:17, danakj wrote:
> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
> > > 
> > > >
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > > > File cc/trees/layer_tree_host_impl.cc (right):
> > > >
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> > > > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> > > > On 2015/05/13 02:53:00, vmiura wrote:
> > > >
> > > >> On 2015/05/12 23:40:43, danakj wrote:
> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
> > > >>
> > > > become true I
> > > >
> > > >> > think (I was hoping it was a one-way trip from true to false but the
> > > >>
> > > > code
> > > >
> > > >> > doesn't look like that to me.)
> > > >> >
> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before this.
> > > >>
> > > > It's using
> > > >
> > > >> > renderercaps but only as a proxy for the contextprovider caps. It's
> > > >>
> > > > also
> > > >
> > > >> > recreating layer resources and stuff which would maybe need to be
> > > >>
> > > > split into
> > > >
> > > >> > another function to run at the time when it currently is.
> > > >>
> > > >
> > > >  Thanks for spotting this.
> > > >>
> > > >
> > > >  Hmm, I think it could also flip from false to true in
> > > >> UpdateGpuRasterizationStatus()
> > > >> via CommitComplete().
> > > >>
> > > >
> > > >  This doesn't look good to me anymore; we can handle failure in
> > > >> InitializeRenderer() but after that I think the code assumes that
> > > >> UpdateGpuRasterizationStatus() succeeds.
> > > >>
> > > >
> > > >  I'm thinking it's safer to back to "ifs" in the several places where
> > > >>
> > > > GrContext()
> > > >
> > > >> is used, with big TODOs to remove them when GrContext can handle
> > > >>
> > > > failure.
> > > >
> > > > We can simply rely on settings_.gpu_rasterization_enabled instead. If we
> > > > /may/ use gpu rasterization, we should check the GrContext. Otherwise,
> > > > we don't care.
> > > >
> > > 
> > > SGTM
> > 
> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU on
> ~22%
> > of page views on supported hardware, but this would increase it to 100%.
> > 
> > We want to remove has_gpu_rasterization_trigger_ in the near term, so I
think
> > that's fine, but slightly more concerned about potentially merging that to
> M43.
> 
> I think the idea of splitting UpdateGpuRasterizationStatus() into two would
> work.
> We could then do the first part (updating use_gpu_rasterization_, use_msaa_
and
> status) automatically when the viewport/content triggers are set, and just set
> a bool that the tree resources are dirty and need to be recreated on draw (or
> maybe 
> just go ahead and ReleaseTreeResources() etc, and use their absence as a
signal
> to 
> recreate them on draw? I'm not familiar enough with that code to know if that
> would 
> be correct / safe).
> 
> Then you could safely check use_gpu_rasterization_ as a signal to check the
> GrContext, 
> as you were before.
> 
> Some unit tests would probably have to be changed, since they assume that 
> UpdateGpuRasterizationStatus() updates the tree resources.

We'd have to make it so UpdateGpuRasterizationStatus() can leave an
uninitialized TileManager, resource pool, etc.  I'm not familiar enough with the
overall code assumptions, if this would work.  Dana?

[danakj, 2015-05-13 18:05:26]

On Wed, May 13, 2015 at 10:57 AM, <vmiura@chromium.org> wrote:

> On 2015/05/13 17:44:24, Stephen White wrote:
>
>> On 2015/05/13 17:19:30, vmiura wrote:
>> > On 2015/05/13 16:53:17, danakj wrote:
>> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
>> > >
>> > > >
>> > > >
>> > > >
>> > >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> > > > File cc/trees/layer_tree_host_impl.cc (right):
>> > > >
>> > > >
>> > > >
>> > >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> > > > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
>> > > > On 2015/05/13 02:53:00, vmiura wrote:
>> > > >
>> > > >> On 2015/05/12 23:40:43, danakj wrote:
>> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
>> > > >>
>> > > > become true I
>> > > >
>> > > >> > think (I was hoping it was a one-way trip from true to false but
>> the
>> > > >>
>> > > > code
>> > > >
>> > > >> > doesn't look like that to me.)
>> > > >> >
>> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
>> this.
>> > > >>
>> > > > It's using
>> > > >
>> > > >> > renderercaps but only as a proxy for the contextprovider caps.
>> It's
>> > > >>
>> > > > also
>> > > >
>> > > >> > recreating layer resources and stuff which would maybe need to be
>> > > >>
>> > > > split into
>> > > >
>> > > >> > another function to run at the time when it currently is.
>> > > >>
>> > > >
>> > > >  Thanks for spotting this.
>> > > >>
>> > > >
>> > > >  Hmm, I think it could also flip from false to true in
>> > > >> UpdateGpuRasterizationStatus()
>> > > >> via CommitComplete().
>> > > >>
>> > > >
>> > > >  This doesn't look good to me anymore; we can handle failure in
>> > > >> InitializeRenderer() but after that I think the code assumes that
>> > > >> UpdateGpuRasterizationStatus() succeeds.
>> > > >>
>> > > >
>> > > >  I'm thinking it's safer to back to "ifs" in the several places
>> where
>> > > >>
>> > > > GrContext()
>> > > >
>> > > >> is used, with big TODOs to remove them when GrContext can handle
>> > > >>
>> > > > failure.
>> > > >
>> > > > We can simply rely on settings_.gpu_rasterization_enabled instead.
>> If we
>> > > > /may/ use gpu rasterization, we should check the GrContext.
>> Otherwise,
>> > > > we don't care.
>> > > >
>> > >
>> > > SGTM
>> >
>> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU
>> on
>> ~22%
>> > of page views on supported hardware, but this would increase it to 100%.
>> >
>> > We want to remove has_gpu_rasterization_trigger_ in the near term, so I
>>
> think
>
>> > that's fine, but slightly more concerned about potentially merging that
>> to
>> M43.
>>
>
>  I think the idea of splitting UpdateGpuRasterizationStatus() into two
>> would
>> work.
>> We could then do the first part (updating use_gpu_rasterization_,
>> use_msaa_
>>
> and
>
>> status) automatically when the viewport/content triggers are set, and
>> just set
>> a bool that the tree resources are dirty and need to be recreated on draw
>> (or
>> maybe
>> just go ahead and ReleaseTreeResources() etc, and use their absence as a
>>
> signal
>
>> to
>> recreate them on draw? I'm not familiar enough with that code to know if
>> that
>> would
>> be correct / safe).
>>
>
>  Then you could safely check use_gpu_rasterization_ as a signal to check
>> the
>> GrContext,
>> as you were before.
>>
>
>  Some unit tests would probably have to be changed, since they assume that
>> UpdateGpuRasterizationStatus() updates the tree resources.
>>
>
> We'd have to make it so UpdateGpuRasterizationStatus() can leave an
> uninitialized TileManager, resource pool, etc.


Why do we need to do that, and not just update the bools, do the
destroy/create as another function?


>   I'm not familiar enough with the
> overall code assumptions, if this would work.  Dana?
>

That doesn't sound good.


>
> https://codereview.chromium.org/1135743004/
>

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

[Stephen White, 2015-05-13 18:21:06]

On 2015/05/13 18:05:26, danakj wrote:
> On Wed, May 13, 2015 at 10:57 AM, <mailto:vmiura@chromium.org> wrote:
> 
> > On 2015/05/13 17:44:24, Stephen White wrote:
> >
> >> On 2015/05/13 17:19:30, vmiura wrote:
> >> > On 2015/05/13 16:53:17, danakj wrote:
> >> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
> >> > >
> >> > > >
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> >
> >> > > > File cc/trees/layer_tree_host_impl.cc (right):
> >> > > >
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> >
> >> > > > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> >> > > > On 2015/05/13 02:53:00, vmiura wrote:
> >> > > >
> >> > > >> On 2015/05/12 23:40:43, danakj wrote:
> >> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
> >> > > >>
> >> > > > become true I
> >> > > >
> >> > > >> > think (I was hoping it was a one-way trip from true to false but
> >> the
> >> > > >>
> >> > > > code
> >> > > >
> >> > > >> > doesn't look like that to me.)
> >> > > >> >
> >> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
> >> this.
> >> > > >>
> >> > > > It's using
> >> > > >
> >> > > >> > renderercaps but only as a proxy for the contextprovider caps.
> >> It's
> >> > > >>
> >> > > > also
> >> > > >
> >> > > >> > recreating layer resources and stuff which would maybe need to be
> >> > > >>
> >> > > > split into
> >> > > >
> >> > > >> > another function to run at the time when it currently is.
> >> > > >>
> >> > > >
> >> > > >  Thanks for spotting this.
> >> > > >>
> >> > > >
> >> > > >  Hmm, I think it could also flip from false to true in
> >> > > >> UpdateGpuRasterizationStatus()
> >> > > >> via CommitComplete().
> >> > > >>
> >> > > >
> >> > > >  This doesn't look good to me anymore; we can handle failure in
> >> > > >> InitializeRenderer() but after that I think the code assumes that
> >> > > >> UpdateGpuRasterizationStatus() succeeds.
> >> > > >>
> >> > > >
> >> > > >  I'm thinking it's safer to back to "ifs" in the several places
> >> where
> >> > > >>
> >> > > > GrContext()
> >> > > >
> >> > > >> is used, with big TODOs to remove them when GrContext can handle
> >> > > >>
> >> > > > failure.
> >> > > >
> >> > > > We can simply rely on settings_.gpu_rasterization_enabled instead.
> >> If we
> >> > > > /may/ use gpu rasterization, we should check the GrContext.
> >> Otherwise,
> >> > > > we don't care.
> >> > > >
> >> > >
> >> > > SGTM
> >> >
> >> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU
> >> on
> >> ~22%
> >> > of page views on supported hardware, but this would increase it to 100%.
> >> >
> >> > We want to remove has_gpu_rasterization_trigger_ in the near term, so I
> >>
> > think
> >
> >> > that's fine, but slightly more concerned about potentially merging that
> >> to
> >> M43.
> >>
> >
> >  I think the idea of splitting UpdateGpuRasterizationStatus() into two
> >> would
> >> work.
> >> We could then do the first part (updating use_gpu_rasterization_,
> >> use_msaa_
> >>
> > and
> >
> >> status) automatically when the viewport/content triggers are set, and
> >> just set
> >> a bool that the tree resources are dirty and need to be recreated on draw
> >> (or
> >> maybe
> >> just go ahead and ReleaseTreeResources() etc, and use their absence as a
> >>
> > signal
> >
> >> to
> >> recreate them on draw? I'm not familiar enough with that code to know if
> >> that
> >> would
> >> be correct / safe).
> >>
> >
> >  Then you could safely check use_gpu_rasterization_ as a signal to check
> >> the
> >> GrContext,
> >> as you were before.
> >>
> >
> >  Some unit tests would probably have to be changed, since they assume that
> >> UpdateGpuRasterizationStatus() updates the tree resources.
> >>
> >
> > We'd have to make it so UpdateGpuRasterizationStatus() can leave an
> > uninitialized TileManager, resource pool, etc.
> 
> 
> Why do we need to do that, and not just update the bools, do the
> destroy/create as another function?

No particular reason, except to avoid yet-another-bool for tree resource
dirtiness.

I assume the state of tree resources doesn't matter until draw time, or 
is it important that they're reset right away?

> 
> >   I'm not familiar enough with the
> > overall code assumptions, if this would work.  Dana?
> >
> 
> That doesn't sound good.
> 
> 
> >
> > https://codereview.chromium.org/1135743004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-13 18:22:47]

On Wed, May 13, 2015 at 11:21 AM, <senorblanco@chromium.org> wrote:

> On 2015/05/13 18:05:26, danakj wrote:
>
>  On Wed, May 13, 2015 at 10:57 AM, <mailto:vmiura@chromium.org> wrote:
>>
>
>  > On 2015/05/13 17:44:24, Stephen White wrote:
>> >
>> >> On 2015/05/13 17:19:30, vmiura wrote:
>> >> > On 2015/05/13 16:53:17, danakj wrote:
>> >> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org>
>> wrote:
>> >> > >
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> >
>> >> > > > File cc/trees/layer_tree_host_impl.cc (right):
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> >
>> >> > > > cc/trees/layer_tree_host_impl.cc:2169: if
>> (use_gpu_rasterization_) {
>> >> > > > On 2015/05/13 02:53:00, vmiura wrote:
>> >> > > >
>> >> > > >> On 2015/05/12 23:40:43, danakj wrote:
>> >> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
>> >> > > >>
>> >> > > > become true I
>> >> > > >
>> >> > > >> > think (I was hoping it was a one-way trip from true to false
>> but
>> >> the
>> >> > > >>
>> >> > > > code
>> >> > > >
>> >> > > >> > doesn't look like that to me.)
>> >> > > >> >
>> >> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
>> >> this.
>> >> > > >>
>> >> > > > It's using
>> >> > > >
>> >> > > >> > renderercaps but only as a proxy for the contextprovider caps.
>> >> It's
>> >> > > >>
>> >> > > > also
>> >> > > >
>> >> > > >> > recreating layer resources and stuff which would maybe need
>> to be
>> >> > > >>
>> >> > > > split into
>> >> > > >
>> >> > > >> > another function to run at the time when it currently is.
>> >> > > >>
>> >> > > >
>> >> > > >  Thanks for spotting this.
>> >> > > >>
>> >> > > >
>> >> > > >  Hmm, I think it could also flip from false to true in
>> >> > > >> UpdateGpuRasterizationStatus()
>> >> > > >> via CommitComplete().
>> >> > > >>
>> >> > > >
>> >> > > >  This doesn't look good to me anymore; we can handle failure in
>> >> > > >> InitializeRenderer() but after that I think the code assumes
>> that
>> >> > > >> UpdateGpuRasterizationStatus() succeeds.
>> >> > > >>
>> >> > > >
>> >> > > >  I'm thinking it's safer to back to "ifs" in the several places
>> >> where
>> >> > > >>
>> >> > > > GrContext()
>> >> > > >
>> >> > > >> is used, with big TODOs to remove them when GrContext can handle
>> >> > > >>
>> >> > > > failure.
>> >> > > >
>> >> > > > We can simply rely on settings_.gpu_rasterization_enabled
>> instead.
>> >> If we
>> >> > > > /may/ use gpu rasterization, we should check the GrContext.
>> >> Otherwise,
>> >> > > > we don't care.
>> >> > > >
>> >> > >
>> >> > > SGTM
>> >> >
>> >> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize
>> GPU
>> >> on
>> >> ~22%
>> >> > of page views on supported hardware, but this would increase it to
>> 100%.
>> >> >
>> >> > We want to remove has_gpu_rasterization_trigger_ in the near term,
>> so I
>> >>
>> > think
>> >
>> >> > that's fine, but slightly more concerned about potentially merging
>> that
>> >> to
>> >> M43.
>> >>
>> >
>> >  I think the idea of splitting UpdateGpuRasterizationStatus() into two
>> >> would
>> >> work.
>> >> We could then do the first part (updating use_gpu_rasterization_,
>> >> use_msaa_
>> >>
>> > and
>> >
>> >> status) automatically when the viewport/content triggers are set, and
>> >> just set
>> >> a bool that the tree resources are dirty and need to be recreated on
>> draw
>> >> (or
>> >> maybe
>> >> just go ahead and ReleaseTreeResources() etc, and use their absence as
>> a
>> >>
>> > signal
>> >
>> >> to
>> >> recreate them on draw? I'm not familiar enough with that code to know
>> if
>> >> that
>> >> would
>> >> be correct / safe).
>> >>
>> >
>> >  Then you could safely check use_gpu_rasterization_ as a signal to check
>> >> the
>> >> GrContext,
>> >> as you were before.
>> >>
>> >
>> >  Some unit tests would probably have to be changed, since they assume
>> that
>> >> UpdateGpuRasterizationStatus() updates the tree resources.
>> >>
>> >
>> > We'd have to make it so UpdateGpuRasterizationStatus() can leave an
>> > uninitialized TileManager, resource pool, etc.
>>
>
>
>  Why do we need to do that, and not just update the bools, do the
>> destroy/create as another function?
>>
>
> No particular reason, except to avoid yet-another-bool for tree resource
> dirtiness.
>
> I assume the state of tree resources doesn't matter until draw time, or
> is it important that they're reset right away?


I would much prefer that we maintain the invariant that LTHI has a Renderer
and a ResourceProvider, etc, and they are not null. Code may be using them
for various things (like the GpuRaster computation for eg) at various times
that may not be part of drawing.


>
>
>
>  >   I'm not familiar enough with the
>> > overall code assumptions, if this would work.  Dana?
>> >
>>
>
>  That doesn't sound good.
>>
>
>
>  >
>> > https://codereview.chromium.org/1135743004/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/1135743004/
>

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

[danakj, 2015-05-13 18:23:43]

On Wed, May 13, 2015 at 11:22 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, May 13, 2015 at 11:21 AM, <senorblanco@chromium.org> wrote:
>
>> On 2015/05/13 18:05:26, danakj wrote:
>>
>>  On Wed, May 13, 2015 at 10:57 AM, <mailto:vmiura@chromium.org> wrote:
>>>
>>
>>  > On 2015/05/13 17:44:24, Stephen White wrote:
>>> >
>>> >> On 2015/05/13 17:19:30, vmiura wrote:
>>> >> > On 2015/05/13 16:53:17, danakj wrote:
>>> >> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org>
>>> wrote:
>>> >> > >
>>> >> > > >
>>> >> > > >
>>> >> > > >
>>> >> > >
>>> >> >
>>> >>
>>> >
>>> >
>>> >
>>>
>>
>>
>>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>>
>>> >
>>> >> > > > File cc/trees/layer_tree_host_impl.cc (right):
>>> >> > > >
>>> >> > > >
>>> >> > > >
>>> >> > >
>>> >> >
>>> >>
>>> >
>>> >
>>> >
>>>
>>
>>
>>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>>
>>> >
>>> >> > > > cc/trees/layer_tree_host_impl.cc:2169: if
>>> (use_gpu_rasterization_) {
>>> >> > > > On 2015/05/13 02:53:00, vmiura wrote:
>>> >> > > >
>>> >> > > >> On 2015/05/12 23:40:43, danakj wrote:
>>> >> > > >> > This can be changed by CreateAndSetRenderer below and
>>> suddenly
>>> >> > > >>
>>> >> > > > become true I
>>> >> > > >
>>> >> > > >> > think (I was hoping it was a one-way trip from true to false
>>> but
>>> >> the
>>> >> > > >>
>>> >> > > > code
>>> >> > > >
>>> >> > > >> > doesn't look like that to me.)
>>> >> > > >> >
>>> >> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
>>> >> this.
>>> >> > > >>
>>> >> > > > It's using
>>> >> > > >
>>> >> > > >> > renderercaps but only as a proxy for the contextprovider
>>> caps.
>>> >> It's
>>> >> > > >>
>>> >> > > > also
>>> >> > > >
>>> >> > > >> > recreating layer resources and stuff which would maybe need
>>> to be
>>> >> > > >>
>>> >> > > > split into
>>> >> > > >
>>> >> > > >> > another function to run at the time when it currently is.
>>> >> > > >>
>>> >> > > >
>>> >> > > >  Thanks for spotting this.
>>> >> > > >>
>>> >> > > >
>>> >> > > >  Hmm, I think it could also flip from false to true in
>>> >> > > >> UpdateGpuRasterizationStatus()
>>> >> > > >> via CommitComplete().
>>> >> > > >>
>>> >> > > >
>>> >> > > >  This doesn't look good to me anymore; we can handle failure in
>>> >> > > >> InitializeRenderer() but after that I think the code assumes
>>> that
>>> >> > > >> UpdateGpuRasterizationStatus() succeeds.
>>> >> > > >>
>>> >> > > >
>>> >> > > >  I'm thinking it's safer to back to "ifs" in the several places
>>> >> where
>>> >> > > >>
>>> >> > > > GrContext()
>>> >> > > >
>>> >> > > >> is used, with big TODOs to remove them when GrContext can
>>> handle
>>> >> > > >>
>>> >> > > > failure.
>>> >> > > >
>>> >> > > > We can simply rely on settings_.gpu_rasterization_enabled
>>> instead.
>>> >> If we
>>> >> > > > /may/ use gpu rasterization, we should check the GrContext.
>>> >> Otherwise,
>>> >> > > > we don't care.
>>> >> > > >
>>> >> > >
>>> >> > > SGTM
>>> >> >
>>> >> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize
>>> GPU
>>> >> on
>>> >> ~22%
>>> >> > of page views on supported hardware, but this would increase it to
>>> 100%.
>>> >> >
>>> >> > We want to remove has_gpu_rasterization_trigger_ in the near term,
>>> so I
>>> >>
>>> > think
>>> >
>>> >> > that's fine, but slightly more concerned about potentially merging
>>> that
>>> >> to
>>> >> M43.
>>> >>
>>> >
>>> >  I think the idea of splitting UpdateGpuRasterizationStatus() into two
>>> >> would
>>> >> work.
>>> >> We could then do the first part (updating use_gpu_rasterization_,
>>> >> use_msaa_
>>> >>
>>> > and
>>> >
>>> >> status) automatically when the viewport/content triggers are set, and
>>> >> just set
>>> >> a bool that the tree resources are dirty and need to be recreated on
>>> draw
>>> >> (or
>>> >> maybe
>>> >> just go ahead and ReleaseTreeResources() etc, and use their absence
>>> as a
>>> >>
>>> > signal
>>> >
>>> >> to
>>> >> recreate them on draw? I'm not familiar enough with that code to know
>>> if
>>> >> that
>>> >> would
>>> >> be correct / safe).
>>> >>
>>> >
>>> >  Then you could safely check use_gpu_rasterization_ as a signal to
>>> check
>>> >> the
>>> >> GrContext,
>>> >> as you were before.
>>> >>
>>> >
>>> >  Some unit tests would probably have to be changed, since they assume
>>> that
>>> >> UpdateGpuRasterizationStatus() updates the tree resources.
>>> >>
>>> >
>>> > We'd have to make it so UpdateGpuRasterizationStatus() can leave an
>>> > uninitialized TileManager, resource pool, etc.
>>>
>>
>>
>>  Why do we need to do that, and not just update the bools, do the
>>> destroy/create as another function?
>>>
>>
>> No particular reason, except to avoid yet-another-bool for tree resource
>> dirtiness.
>>
>> I assume the state of tree resources doesn't matter until draw time, or
>> is it important that they're reset right away?
>
>
> I would much prefer that we maintain the invariant that LTHI has a
> Renderer and a ResourceProvider, etc, and they are not null. Code may be
> using them for various things (like the GpuRaster computation for eg) at
> various times that may not be part of drawing.
>

And TileManager/ResourcePool are used to create the resources used for
drawing and signal that we should draw. They def have to exist outside of
drawing or we will never draw.


>
>
>>
>>
>>
>>  >   I'm not familiar enough with the
>>> > overall code assumptions, if this would work.  Dana?
>>> >
>>>
>>
>>  That doesn't sound good.
>>>
>>
>>
>>  >
>>> > https://codereview.chromium.org/1135743004/
>>> >
>>>
>>
>>  To unsubscribe from this group and stop receiving emails from it, send an
>>>
>> email
>>
>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>
>>
>>
>>
>> https://codereview.chromium.org/1135743004/
>>
>
>

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

[Stephen White, 2015-05-13 19:19:01]

On 2015/05/13 18:23:43, danakj wrote:

> > I would much prefer that we maintain the invariant that LTHI has a
> > Renderer and a ResourceProvider, etc, and they are not null. Code may be
> > using them for various things (like the GpuRaster computation for eg) at
> > various times that may not be part of drawing.

OK -- you know better than me about this code.

I'm a little unclear as to when we can safely update them, though.

Let me sketch out some code, since it's probably faster than trying to explain
it in a comment.

> And TileManager/ResourcePool are used to create the resources used for
> drawing and signal that we should draw. They def have to exist outside of
> drawing or we will never draw.

OK, good to know.

[Stephen White, 2015-05-13 19:38:03]

On 2015/05/13 19:19:01, Stephen White wrote:
> On 2015/05/13 18:23:43, danakj wrote:
> 
> > > I would much prefer that we maintain the invariant that LTHI has a
> > > Renderer and a ResourceProvider, etc, and they are not null. Code may be
> > > using them for various things (like the GpuRaster computation for eg) at
> > > various times that may not be part of drawing.
> 
> OK -- you know better than me about this code.
> 
> I'm a little unclear as to when we can safely update them, though.
> 
> Let me sketch out some code, since it's probably faster than trying to explain
> it in a comment.
> 
> > And TileManager/ResourcePool are used to create the resources used for
> > drawing and signal that we should draw. They def have to exist outside of
> > drawing or we will never draw.
> 
> OK, good to know.

I was thinking something like this: https://codereview.chromium.org/1134123005/

[vmiura, 2015-05-13 20:36:08]

On 2015/05/13 18:05:26, danakj wrote:
> On Wed, May 13, 2015 at 10:57 AM, <mailto:vmiura@chromium.org> wrote:
> 
> > On 2015/05/13 17:44:24, Stephen White wrote:
> >
> >> On 2015/05/13 17:19:30, vmiura wrote:
> >> > On 2015/05/13 16:53:17, danakj wrote:
> >> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org> wrote:
> >> > >
> >> > > >
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> >
> >> > > > File cc/trees/layer_tree_host_impl.cc (right):
> >> > > >
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
> >
> >> > > > cc/trees/layer_tree_host_impl.cc:2169: if (use_gpu_rasterization_) {
> >> > > > On 2015/05/13 02:53:00, vmiura wrote:
> >> > > >
> >> > > >> On 2015/05/12 23:40:43, danakj wrote:
> >> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
> >> > > >>
> >> > > > become true I
> >> > > >
> >> > > >> > think (I was hoping it was a one-way trip from true to false but
> >> the
> >> > > >>
> >> > > > code
> >> > > >
> >> > > >> > doesn't look like that to me.)
> >> > > >> >
> >> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
> >> this.
> >> > > >>
> >> > > > It's using
> >> > > >
> >> > > >> > renderercaps but only as a proxy for the contextprovider caps.
> >> It's
> >> > > >>
> >> > > > also
> >> > > >
> >> > > >> > recreating layer resources and stuff which would maybe need to be
> >> > > >>
> >> > > > split into
> >> > > >
> >> > > >> > another function to run at the time when it currently is.
> >> > > >>
> >> > > >
> >> > > >  Thanks for spotting this.
> >> > > >>
> >> > > >
> >> > > >  Hmm, I think it could also flip from false to true in
> >> > > >> UpdateGpuRasterizationStatus()
> >> > > >> via CommitComplete().
> >> > > >>
> >> > > >
> >> > > >  This doesn't look good to me anymore; we can handle failure in
> >> > > >> InitializeRenderer() but after that I think the code assumes that
> >> > > >> UpdateGpuRasterizationStatus() succeeds.
> >> > > >>
> >> > > >
> >> > > >  I'm thinking it's safer to back to "ifs" in the several places
> >> where
> >> > > >>
> >> > > > GrContext()
> >> > > >
> >> > > >> is used, with big TODOs to remove them when GrContext can handle
> >> > > >>
> >> > > > failure.
> >> > > >
> >> > > > We can simply rely on settings_.gpu_rasterization_enabled instead.
> >> If we
> >> > > > /may/ use gpu rasterization, we should check the GrContext.
> >> Otherwise,
> >> > > > we don't care.
> >> > > >
> >> > >
> >> > > SGTM
> >> >
> >> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize GPU
> >> on
> >> ~22%
> >> > of page views on supported hardware, but this would increase it to 100%.
> >> >
> >> > We want to remove has_gpu_rasterization_trigger_ in the near term, so I
> >>
> > think
> >
> >> > that's fine, but slightly more concerned about potentially merging that
> >> to
> >> M43.
> >>
> >
> >  I think the idea of splitting UpdateGpuRasterizationStatus() into two
> >> would
> >> work.
> >> We could then do the first part (updating use_gpu_rasterization_,
> >> use_msaa_
> >>
> > and
> >
> >> status) automatically when the viewport/content triggers are set, and
> >> just set
> >> a bool that the tree resources are dirty and need to be recreated on draw
> >> (or
> >> maybe
> >> just go ahead and ReleaseTreeResources() etc, and use their absence as a
> >>
> > signal
> >
> >> to
> >> recreate them on draw? I'm not familiar enough with that code to know if
> >> that
> >> would
> >> be correct / safe).
> >>
> >
> >  Then you could safely check use_gpu_rasterization_ as a signal to check
> >> the
> >> GrContext,
> >> as you were before.
> >>
> >
> >  Some unit tests would probably have to be changed, since they assume that
> >> UpdateGpuRasterizationStatus() updates the tree resources.
> >>
> >
> > We'd have to make it so UpdateGpuRasterizationStatus() can leave an
> > uninitialized TileManager, resource pool, etc.
> 
> 
> Why do we need to do that, and not just update the bools, do the
> destroy/create as another function?

I mean, as you mentioned it would be better to maintain the invariant that LTHI
has a Renderer, ResourceProvider, etc. regardless of where we destroy/create
them.  If GrContext creation fails then we can't create a GPU rasterizer.

Let me add a variation to cosnider: https://codereview.chromium.org/1139463004

In this we just don't allow a GPU rasterizer to be created if we don't have a
valid GrContext.  This also catches a case where currently
use_gpu_rasterization_ == true but we actually create a software rasterizer
because of lacking a context_provider.  I hit this in one of the unittests.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

[danakj, 2015-05-13 20:44:40]

On Wed, May 13, 2015 at 1:36 PM, <vmiura@chromium.org> wrote:

> On 2015/05/13 18:05:26, danakj wrote:
>
>> On Wed, May 13, 2015 at 10:57 AM, <mailto:vmiura@chromium.org> wrote:
>>
>
>  > On 2015/05/13 17:44:24, Stephen White wrote:
>> >
>> >> On 2015/05/13 17:19:30, vmiura wrote:
>> >> > On 2015/05/13 16:53:17, danakj wrote:
>> >> > > On Tue, May 12, 2015 at 8:28 PM, <mailto:piman@chromium.org>
>> wrote:
>> >> > >
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> >
>> >> > > > File cc/trees/layer_tree_host_impl.cc (right):
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1135743004/diff/20001/cc/trees/layer_tree_hos...
>
>> >
>> >> > > > cc/trees/layer_tree_host_impl.cc:2169: if
>> (use_gpu_rasterization_) {
>> >> > > > On 2015/05/13 02:53:00, vmiura wrote:
>> >> > > >
>> >> > > >> On 2015/05/12 23:40:43, danakj wrote:
>> >> > > >> > This can be changed by CreateAndSetRenderer below and suddenly
>> >> > > >>
>> >> > > > become true I
>> >> > > >
>> >> > > >> > think (I was hoping it was a one-way trip from true to false
>> but
>> >> the
>> >> > > >>
>> >> > > > code
>> >> > > >
>> >> > > >> > doesn't look like that to me.)
>> >> > > >> >
>> >> > > >> > But maybe you can make UpdateGpuRasterizationStatus() before
>> >> this.
>> >> > > >>
>> >> > > > It's using
>> >> > > >
>> >> > > >> > renderercaps but only as a proxy for the contextprovider caps.
>> >> It's
>> >> > > >>
>> >> > > > also
>> >> > > >
>> >> > > >> > recreating layer resources and stuff which would maybe need
>> to be
>> >> > > >>
>> >> > > > split into
>> >> > > >
>> >> > > >> > another function to run at the time when it currently is.
>> >> > > >>
>> >> > > >
>> >> > > >  Thanks for spotting this.
>> >> > > >>
>> >> > > >
>> >> > > >  Hmm, I think it could also flip from false to true in
>> >> > > >> UpdateGpuRasterizationStatus()
>> >> > > >> via CommitComplete().
>> >> > > >>
>> >> > > >
>> >> > > >  This doesn't look good to me anymore; we can handle failure in
>> >> > > >> InitializeRenderer() but after that I think the code assumes
>> that
>> >> > > >> UpdateGpuRasterizationStatus() succeeds.
>> >> > > >>
>> >> > > >
>> >> > > >  I'm thinking it's safer to back to "ifs" in the several places
>> >> where
>> >> > > >>
>> >> > > > GrContext()
>> >> > > >
>> >> > > >> is used, with big TODOs to remove them when GrContext can handle
>> >> > > >>
>> >> > > > failure.
>> >> > > >
>> >> > > > We can simply rely on settings_.gpu_rasterization_enabled
>> instead.
>> >> If we
>> >> > > > /may/ use gpu rasterization, we should check the GrContext.
>> >> Otherwise,
>> >> > > > we don't care.
>> >> > > >
>> >> > >
>> >> > > SGTM
>> >> >
>> >> > Commonly has_gpu_rasterization_trigger_ is false, and we initialize
>> GPU
>> >> on
>> >> ~22%
>> >> > of page views on supported hardware, but this would increase it to
>> 100%.
>> >> >
>> >> > We want to remove has_gpu_rasterization_trigger_ in the near term,
>> so I
>> >>
>> > think
>> >
>> >> > that's fine, but slightly more concerned about potentially merging
>> that
>> >> to
>> >> M43.
>> >>
>> >
>> >  I think the idea of splitting UpdateGpuRasterizationStatus() into two
>> >> would
>> >> work.
>> >> We could then do the first part (updating use_gpu_rasterization_,
>> >> use_msaa_
>> >>
>> > and
>> >
>> >> status) automatically when the viewport/content triggers are set, and
>> >> just set
>> >> a bool that the tree resources are dirty and need to be recreated on
>> draw
>> >> (or
>> >> maybe
>> >> just go ahead and ReleaseTreeResources() etc, and use their absence as
>> a
>> >>
>> > signal
>> >
>> >> to
>> >> recreate them on draw? I'm not familiar enough with that code to know
>> if
>> >> that
>> >> would
>> >> be correct / safe).
>> >>
>> >
>> >  Then you could safely check use_gpu_rasterization_ as a signal to check
>> >> the
>> >> GrContext,
>> >> as you were before.
>> >>
>> >
>> >  Some unit tests would probably have to be changed, since they assume
>> that
>> >> UpdateGpuRasterizationStatus() updates the tree resources.
>> >>
>> >
>> > We'd have to make it so UpdateGpuRasterizationStatus() can leave an
>> > uninitialized TileManager, resource pool, etc.
>>
>
>
>  Why do we need to do that, and not just update the bools, do the
>> destroy/create as another function?
>>
>
> I mean, as you mentioned it would be better to maintain the invariant that
> LTHI
> has a Renderer, ResourceProvider, etc. regardless of where we
> destroy/create
> them.  If GrContext creation fails then we can't create a GPU rasterizer.
>

We can create one, it won't do anything useful. Then we'll be notified
about the context being lost/invalid, and drop everything and start over?


>
> Let me add a variation to cosnider:
> https://codereview.chromium.org/1139463004
>
> In this we just don't allow a GPU rasterizer to be created if we don't
> have a
> valid GrContext.  This also catches a case where currently
> use_gpu_rasterization_ == true but we actually create a software rasterizer
> because of lacking a context_provider.  I hit this in one of the unittests.
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
>
>
> https://codereview.chromium.org/1135743004/
>

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

[vmiura, 2015-05-13 23:26:13]

PT Another Look.

I've merged https://codereview.chromium.org/1139463004 here,
so instead of bailing in InitializeRenderer(), we block GPU raster in
UpdateGpuRasterizationStatus(), and so fallback to software.

[piman, 2015-05-13 23:28:06]

lgtm

[danakj, 2015-05-13 23:46:29]

LGTM

[vmiura, 2015-05-14 00:07:59]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2015-05-14 00:07:59]

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/1135743004/#ps40001
(title: "Block GPU rasterization mode if ContextProviders or GrContext are
missing.")

[commit-bot: I haz the power, 2015-05-14 00:09:10]

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

[commit-bot: I haz the power, 2015-05-14 02:24:46]

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

[commit-bot: I haz the power, 2015-05-14 02:24:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmiura, 2015-05-14 20:00:08]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2015-05-14 20:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1135743004/#ps60001
(title: "Only check the GrContext when GPU raster is transitioning to On.")

[commit-bot: I haz the power, 2015-05-14 20:02:41]

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

[commit-bot: I haz the power, 2015-05-14 22:11:28]

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

[commit-bot: I haz the power, 2015-05-14 22:11:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmiura, 2015-05-15 01:15:42]

I missed that we must hold the worker context's lock while GrContext() is
touched and may issue GL commands.

Refactored this into a LayerTreeHostImpl::CanUseGpuRasterization() method, as it
makes the conditionals simpler.

[vmiura, 2015-05-15 16:08:14]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2015-05-15 16:08:14]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1135743004/#ps100001
(title: "Worker context provider's lock must be held during GrContext
creation.")

[commit-bot: I haz the power, 2015-05-15 16:09:35]

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

[commit-bot: I haz the power, 2015-05-15 16:16:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-15 16:17:27]

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