cc: Let skia veto gpu rasterization.

cc: Let skia veto gpu rasterization.

Once gpu rasterization is disabled on a layer, it will stay disabled.

BUG=329722
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262994

[alokp, 2014-04-04 18:19:42]

ernstm, vangelis: FYI

bsalomon, humper: We need to discuss the usage of GrContext argument in
SkPicture::isSuitableForGpuRasterization. Please see my comments in
cc::Picture::IsSuitableForGpuRasterization.

[enne (OOO), 2014-04-04 18:34:19]

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:154: // This flag will be pushed with the next
commit.
I'm confused why the SetNeedsCommit went away.

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.h:64: enum TriBool { TRIBOOL_UNKNOWN, TRIBOOL_FALSE,
TRIBOOL_TRUE };
I think this would be clearer if you just added a second bool for
gpu_rasterization_has_ever_been_disabled_.  

Actually, rather than adding a second boolean, could you also use the
PicturePile's suitable boolean as the "sticky" disabled flag directly?

https://codereview.chromium.org/222903005/diff/70001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/222903005/diff/70001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:252: is_suitable_for_gpu_rasterization_ =
You could just shorten this to &=

[enne (OOO), 2014-04-04 18:40:44]

(Also, unrelated, but I think the "sticky flag on a cc::Layer" pattern isn't the
best, since layers get created and destroyed all the time from Blink based on
other changes, so we probably lose the stickiness quite often.  Perhaps we need
to think about pushing more of these flags [like LCD too] up the stack to Blink.
 I think it's not unreasonable to do it here as a first pass, but just a thought
for the future.)

[alokp, 2014-04-04 18:50:34]

I agree about moving the sticky state to blink. I will look into it separately.

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:154: // This flag will be pushed with the next
commit.
On 2014/04/04 18:34:19, enne wrote:
> I'm confused why the SetNeedsCommit went away.

If only the hint changes and the layer is still valid, there is no need to do
anything immediately. If there is an invalidation (current or future) we will
re-raster everything so that it is consistent across the layer. I will add more
comments if this sounds reasonable to you.

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.h:64: enum TriBool { TRIBOOL_UNKNOWN, TRIBOOL_FALSE,
TRIBOOL_TRUE };
On 2014/04/04 18:34:19, enne wrote:
> I think this would be clearer if you just added a second bool for
> gpu_rasterization_has_ever_been_disabled_.  
> 
> Actually, rather than adding a second boolean, could you also use the
> PicturePile's suitable boolean as the "sticky" disabled flag directly?

I do not understand the comment about PicturePile boolean. Can you elaborate?

https://codereview.chromium.org/222903005/diff/70001/cc/resources/picture_pil...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/222903005/diff/70001/cc/resources/picture_pil...
cc/resources/picture_pile.cc:252: is_suitable_for_gpu_rasterization_ =
On 2014/04/04 18:34:19, enne wrote:
> You could just shorten this to &=

Done.

[enne (OOO), 2014-04-04 19:40:07]

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:154: // This flag will be pushed with the next
commit.
On 2014/04/04 18:50:34, Alok Priyadarshi wrote:
> On 2014/04/04 18:34:19, enne wrote:
> > I'm confused why the SetNeedsCommit went away.
> 
> If only the hint changes and the layer is still valid, there is no need to do
> anything immediately. If there is an invalidation (current or future) we will
> re-raster everything so that it is consistent across the layer. I will add
more
> comments if this sounds reasonable to you.

Yeah, this just seems a little dodgy to me, because it seems like you should at
least be setting "needs push properties".  I guess you could argue that if
there's any invalidation then properties will need to be pushed, so this will
get covered, but it's subtle.

A comment would help a lot, since normally most code sets a property and then
sets needs commit / needs push properties, so it looks missing.

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/222903005/diff/70001/cc/layers/picture_layer....
cc/layers/picture_layer.h:64: enum TriBool { TRIBOOL_UNKNOWN, TRIBOOL_FALSE,
TRIBOOL_TRUE };
On 2014/04/04 18:50:34, Alok Priyadarshi wrote:
> On 2014/04/04 18:34:19, enne wrote:
> > I think this would be clearer if you just added a second bool for
> > gpu_rasterization_has_ever_been_disabled_.  
> > 
> > Actually, rather than adding a second boolean, could you also use the
> > PicturePile's suitable boolean as the "sticky" disabled flag directly?
> 
> I do not understand the comment about PicturePile boolean. Can you elaborate?

I'm saying that pile_->is_suitable_for_gpu_rasterization() is already sticky
across frames, and that rather than using a tri-state enum (which I find
confusing in logic), that you can just check that function as a "has ever been
disabled" flag.

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:144: has_gpu_rasterization_hint_ = has_hint ?
TRIBOOL_TRUE : TRIBOOL_FALSE;
In other words, keep this function the same as before...

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:162: return has_gpu_rasterization_hint_ ==
TRIBOOL_TRUE &&
...and just say has_gpu_rasterization_hint_ &&
pile_->is_suitable_for_gpu_rasterization().

It just seems like you're maintaining redundant state (the "has ever been
disabled" state) in the TRIBOOL and the pile suitability flag.

[alokp, 2014-04-04 21:53:45]

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer....
cc/layers/picture_layer.cc:162: return has_gpu_rasterization_hint_ ==
TRIBOOL_TRUE &&
I do not think there is any redundancy. Both the flags - has-hint and
is-suitable - are sticky but controlled by two different independent signals -
will-change and skia-veto.

If I understand your proposal, it may not work if the content is always
gpu-suitable:
1. The default value of has-hint is false, so we raster with cpu.
2. SetHasGpuRasterizationHint(true)
3. Invalidation - contents is gpu-suitable
4. Raster with gpu
5. SetHasGpuRasterizationHint(false)
6. Invalidation - contents still gpu-suitable
7. Raster with cpu
8. SetHasGpuRasterizationHint(true)
9. Invalidation - contents still gpu-suitable
10. Raster with gpu

In short, we can keep switch between cpu and gpu if the content continues to be
gpu-suitable. Please let me know if you had something else in mind.

I have used boost::tribool in the past to represent unknown boolean state and it
seems appropriate in this case. A two-state boolean does not work because the
default state is the same as the sticky state. I can add another bool if it
makes it less confusing.

[enne (OOO), 2014-04-04 22:33:29]

Oh, I see.  I missed the point that there's two independently sticky settings. 
Sorry for my confusion!

I find tribools a little confusing (and can lead to weirdness where
bool(TRIBOOL_FALSE) evaluates to true), but don't feel at all strongly about it.
 So, this is fine by me, lgtm.

[bsalomon_chromium, 2014-04-07 13:24:39]

The reason we would need the GrContext is that it has a set of capabilities that
we've collected from the GL context (mainly what extensions are present). If we
can't actually have the GrContext we could probably figure out an alternative.
The minimum would be some of the GL strings (extensions, renderer, version).

[alokp, 2014-04-07 16:43:29]

On 2014/04/07 13:24:39, bsalomon1 wrote:
> The reason we would need the GrContext is that it has a set of capabilities
that
> we've collected from the GL context (mainly what extensions are present). If
we
> can't actually have the GrContext we could probably figure out an alternative.
> The minimum would be some of the GL strings (extensions, renderer, version).

I am curious how SkPicture::suitableForGpuRasterization would work. Would the
analysis be done when this function is called or during record? If the former,
it may be better to combine this analysis with an existing analysis step on the
chrome side so that we do not need to playback the SkPicture multiple times. If
the latter, wouldn't you need this GrContext when recording starts?

[alokp, 2014-04-07 16:44:29]

Ali: Can you take a look. I moved around a few of the tests you added.

[humper, 2014-04-07 16:46:21]

On 2014/04/07 16:43:29, Alok Priyadarshi wrote:
> On 2014/04/07 13:24:39, bsalomon1 wrote:
> > The reason we would need the GrContext is that it has a set of capabilities
> that
> > we've collected from the GL context (mainly what extensions are present). If
> we
> > can't actually have the GrContext we could probably figure out an
alternative.
> > The minimum would be some of the GL strings (extensions, renderer, version).
> 
> I am curious how SkPicture::suitableForGpuRasterization would work. Would the
> analysis be done when this function is called or during record? If the former,
> it may be better to combine this analysis with an existing analysis step on
the
> chrome side so that we do not need to playback the SkPicture multiple times.
If
> the latter, wouldn't you need this GrContext when recording starts?

The idea is that at record time, we would flag the picture whenever an operation
occurs that is known to be slow across all current realizations of Ganesh.

If there are things that depend on the context, we will either be able to
determine them solely from the context, or we will make note of them during
record time (e.g., a flag saying "only ganesh if context has such and such
capability") and combine those flags with the context capabilities at query
time.

the suitability predicate function should be extremely lightweight.

[ajuma, 2014-04-07 17:38:25]

lgtm with a nit:

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (left):

https://codereview.chromium.org/222903005/diff/90001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:1632: class
HybridRasterizationPictureLayerImplTest : public PictureLayerImplTest {
We'll need to keep this class -- a couple more tests using this were added last
week.

[alokp, 2014-04-09 18:28:27]

https://codereview.chromium.org/222903005/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/222903005/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: is_suitable_for_gpu_rasterization_ &=
enne@: I need to move this flag to the impl thread because
SkPicture::suitableForGpuRasterization needs the GrContext. Is there a way to
detect whenever a PicturePileImpl is updated? Otherwise I may need to traverse
through all pictures in the pile after each commit.

[enne (OOO), 2014-04-09 18:32:53]

https://codereview.chromium.org/222903005/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/222903005/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:253: is_suitable_for_gpu_rasterization_ &=
On 2014/04/09 18:28:27, Alok Priyadarshi wrote:
> enne@: I need to move this flag to the impl thread because
> SkPicture::suitableForGpuRasterization needs the GrContext. Is there a way to
> detect whenever a PicturePileImpl is updated? Otherwise I may need to traverse
> through all pictures in the pile after each commit.

PicturePileImpls are never updated, only overwritten.  I think you could just
move this flag to the base class and copy it in each of the ctors and copy ctors
there.

[alokp, 2014-04-09 23:12:47]

I looked at moving the veto to the impl-thread so that we could pass GrContext
to SkPicture::suitableForGpuRasterization. There are two main issues:

1. I could not figure out how to avoid a post-commit analysis step that iterates
through all pictures in the picture pile. AFAICT there is no clear way to figure
out which pictures are new and which ones have already been analyzed. I may be
able to limit the analysis to invalidation region. This analysis may be done in
PictureLayerImpl::DoPostCommitInitialization. PictureLayerImpl can get the
GrContext from LayerTreeImpl, which leads to the second issue.

2. enne@ raised a valid concern about exposing PicturePile and Picture to
GrContext. We may be able to avoid that by having Picture expose a const
SkPicture.

Since the need for GrContext is still speculative, I am going to land this patch
for now and revisit this later. We may either move it to the impl-thread or
fetch whatever information skia needs on the main thread.

[alokp, 2014-04-09 23:22:02]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-04-09 23:22:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/222903005/140001

[commit-bot: I haz the power, 2014-04-10 00:33:06]

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

[commit-bot: I haz the power, 2014-04-10 00:33:07]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[alokp, 2014-04-10 00:40:50]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-04-10 00:41:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/222903005/140001

[commit-bot: I haz the power, 2014-04-10 01:04:21]

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

[commit-bot: I haz the power, 2014-04-10 01:04:22]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[alokp, 2014-04-10 06:44:50]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-04-10 06:44:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/222903005/140001

[commit-bot: I haz the power, 2014-04-10 07:17:47]

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

[commit-bot: I haz the power, 2014-04-10 07:17:47]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[alokp, 2014-04-10 14:10:12]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-04-10 14:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/222903005/140001

[commit-bot: I haz the power, 2014-04-10 15:12:22]

Change committed as 262994