First pass at GPU veto

First pass at GPU veto

As a short term solution this CL collects information during the recording process for use in suitableForGpuRasterization.

BUG=366495
Committed: http://code.google.com/p/skia/source/detail?r=14368

[robertphillips, 2014-04-24 18:04:07]

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:371: */
SkPictInfo is in src\core\SkPicturePlayback.h. Don't know if we want to make it
more visible.

[bsalomon, 2014-04-24 18:20:51]

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:371: */
On 2014/04/24 18:04:07, robertphillips wrote:
> SkPictInfo is in src\core\SkPicturePlayback.h. Don't know if we want to make
it
> more visible.

4 << 1?

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:377: kHasPathEffects_ContentFlag = kFirstBit,
Hm... It seems more flexible to count things rather than just flag them.

[robertphillips, 2014-04-24 18:27:26]

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/251533004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:371: */
New version should address this.

[robertphillips, 2014-04-24 19:38:59]

PTAL - now with counting.

[bsalomon, 2014-04-24 19:57:36]

https://codereview.chromium.org/251533004/diff/60001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/251533004/diff/60001/src/core/SkPicture.cpp#n...
src/core/SkPicture.cpp:612: static const int kNumAAConcavePaths = 1;
Can we start with this one a little larger? Maybe 5? I realize it's still a huge
hack.

I'm also realizing that we might want to know the surface we're going to be
asked to render to. If it has MSAA we don't really need to worry about concave
paths so much. Alok, is this possible?

Also, we need to know that they're not hairline stroked.

[bsalomon, 2014-04-24 20:46:56]

lgtm.


From in-person discussion with Rob: We probably don't need the actual SkSurface
or GrRenderTarget. The sample count should suffice. We could also have a named
special value for the case where the client knows they'll use the sample count
recommended by GrContext::getRecommendedSampleCount(). However, that function
requires the GrPixelConfig, so we'd want that, too.

[alokp, 2014-04-24 20:55:21]

https://codereview.chromium.org/251533004/diff/60001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/251533004/diff/60001/src/core/SkPicture.cpp#n...
src/core/SkPicture.cpp:612: static const int kNumAAConcavePaths = 1;
On 2014/04/24 19:57:37, bsalomon wrote:
> Can we start with this one a little larger? Maybe 5? I realize it's still a
huge
> hack.
> 
> I'm also realizing that we might want to know the surface we're going to be
> asked to render to. If it has MSAA we don't really need to worry about concave
> paths so much. Alok, is this possible?
> 
> Also, we need to know that they're not hairline stroked.

Would you have MSAA info in GrContext? I will need to move the veto call to
impl-thread to access GrContext. It is possible to do that with some work.

[robertphillips, 2014-04-24 21:01:06]

The CQ bit was checked by robertphillips@google.com

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

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/251533004/8...

[bsalomon, 2014-04-24 21:02:10]

On 2014/04/24 20:55:21, Alok Priyadarshi wrote:

> Would you have MSAA info in GrContext? I will need to move the veto call to
> impl-thread to access GrContext. It is possible to do that with some work.


The client tells Ganesh how many samples they want when the surface/render
target is created. So GrContext knows the max sample count, but the actual count
is up to the client. We also have a function on GrContext called
getRecommendedSampleCount(dpi, pixelConfig) that lets the client defer to us
about the sampleCount.

So we could have two versions of suitableForGpuRasterization():
bool suitableForGpuRasterization(GrCtx, msaaCount) {
  // do the real work
}

bool suitableForGpuRasterization(GrCtx, dpi, pixelConfig) {
  return this->suitableForGpuRasterization(GrCtx,
GrCtx->getRecommendedSampleCount(dpi, pixelConfig));
}

Chromium could call the second if it knows it will always using
getRecommendedSampleCount when creating its surfaces.

[commit-bot: I haz the power, 2014-04-24 21:53:36]

Change committed as 14368

[tomhudson, 2014-04-28 11:03:24]

Do we need a second implementation of this with SkRecord?

Why was this done via insertion into SkPictureRecord instead of subclassing on
top of it as we've done for AnalysisCanvas etc in the past?

[robertphillips, 2014-04-28 12:02:41]

When SkRecord is integrated into the rest of Skia we will want to reimplement
this capability. In that world we will probably implement it as a quick scan
over the recorded operations. The current version was implemented as part of
SkPictureReocrd (rather then another pass over the data) to keep it fast coupled
with the knowledge that it will be going away soon. It probably isn't worth
currently implementing this in SkRecord since it is likely to be changing
rapidly short-term.

[mtklein, 2014-04-28 13:22:36]

On 2014/04/28 12:02:41, robertphillips wrote:
> When SkRecord is integrated into the rest of Skia we will want to reimplement
> this capability. In that world we will probably implement it as a quick scan
> over the recorded operations. The current version was implemented as part of
> SkPictureReocrd (rather then another pass over the data) to keep it fast
coupled
> with the knowledge that it will be going away soon. It probably isn't worth
> currently implementing this in SkRecord since it is likely to be changing
> rapidly short-term.

Yup, for fun I sketched up a version of the current logic for SkRecord.  It was
easy.  The important part here is figuring out what we want to veto.  Even
starting from scratch, this will take ~no time to port to a scan over SkRecord.

[tomhudson, 2014-04-28 13:48:11]

On 2014/04/28 13:22:36, mtklein wrote:
> On 2014/04/28 12:02:41, robertphillips wrote:
> > When SkRecord is integrated into the rest of Skia we will want to
reimplement
> > this capability. In that world we will probably implement it as a quick scan
> > over the recorded operations. The current version was implemented as part of
> > SkPictureReocrd (rather then another pass over the data) to keep it fast
> coupled
> > with the knowledge that it will be going away soon. It probably isn't worth
> > currently implementing this in SkRecord since it is likely to be changing
> > rapidly short-term.
> 
> Yup, for fun I sketched up a version of the current logic for SkRecord.  It
was
> easy.  The important part here is figuring out what we want to veto.  Even
> starting from scratch, this will take ~no time to port to a scan over
SkRecord.

I'm not sure we understand any of Rob's reasons over here:

(1) We were hoping to turn on SkRecord in Chrome real soon now - perhaps even
this week? It's not clear to me/us what's blocking that.

(2) An algorithm like AnalysisCanvas didn't have to be a separate pass; the
reason for making it one was so that we could make a decision based on the
analysis results before we did the recording. If we don't need the results of
the analysis until rasterization time, it could be in a Canvas/Device subclass
rather than built into Skia.

(3) Making AnalysisCanvas part of the skia:: hierarchy was intended to make it
easier for Chrome to tune than is code in Skia; if we're tuning it based on
Chrome benchmark results rather than Skia microbenchmark results, shouldn't we
have the code in Chrome? If we're going to have to change it rapidly in the
short term, shouldn't we be aiming for that?

[Caveat: I've seen comment streams about getting rid of a lot of virtual
functions, but it looks like all the necessary ones are still there on SkDevice
and SkCanvas]

On the other hand, one of the ideas we've heard coming out of MTV is that they
want to use SkPictureRecord for software compositor, but SkRecord for GPU, on
the grounds of unspecified unsubstantiated performance fears. If that's the
course we choose going forward, this is making the decision between GPU and
not-GPU too late. (Or requiring supporting both playback methods for both
rasterization methods, which is also ugly. But we don't understand their current
approach to driving the GPU/software decision, either.)