Add flag for window rectangles to GrRenderTarget

Add flag for window rectangles to GrRenderTarget

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002

Committed: https://skia.googlesource.com/skia/+/f9635999a4aa8810d04e8ef04594a9fbcc061e3d

[csmartdalton, 2016-08-09 06:47:37]

Description was changed from

==========
Add flag that indicates if a render target is offscreen

Adds a flag to GrRenderTarget that indicates if it is offscreen. This
will be required by window rectangles, which can only be used with
offscreen framebuffers.

Also attempts to clean up some of the mixed samples API.

BUG=skia:
==========

to

==========
Add flag that indicates if a render target is offscreen

Adds a flag to GrRenderTarget that indicates if it is offscreen. This
will be required by window rectangles, which can only be used with
offscreen framebuffers.

Also attempts to clean up some of the mixed samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

[csmartdalton, 2016-08-09 06:52:57]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, robertphillips@google.comv

[csmartdalton, 2016-08-09 06:52:58]

Greg, Is there an easy way to tell if a vulkan buffer is offscreen?

https://codereview.chromium.org/2225303002/diff/1/src/gpu/vk/GrVkRenderTarget...
File src/gpu/vk/GrVkRenderTarget.cpp (right):

https://codereview.chromium.org/2225303002/diff/1/src/gpu/vk/GrVkRenderTarget...
src/gpu/vk/GrVkRenderTarget.cpp:115: return true/*???*/ ? Flags::kOffscreen :
Flags::kNone;
Can we deduce here whether the RT is offscreen?

https://codereview.chromium.org/2225303002/diff/1/tests/ProxyTest.cpp
File tests/ProxyTest.cpp (right):

https://codereview.chromium.org/2225303002/diff/1/tests/ProxyTest.cpp#newcode155
tests/ProxyTest.cpp:155: backendDesc.fRenderTargetHandle = 0;
Would this also work for vulkan?

[csmartdalton, 2016-08-09 06:53:12]

csmartdalton@google.com changed reviewers:
+ robertphillips@google.com
- robertphillips@google.comv

[csmartdalton, 2016-08-09 06:53:12]

1

[egdaniel, 2016-08-09 13:19:55]

On 2016/08/09 06:53:12, csmartdalton wrote:
> 1

I'm assuming by offscreen in GL you just mean not FBO 0 right? If that is the
case I do not believe there is anyway to tell if an RT is offscreen in vulkan.
You would need to explicitly pass in some flag at create time if that is
something you'd want to track.

[bsalomon, 2016-08-09 13:40:16]

On 2016/08/09 13:19:55, egdaniel wrote:
> On 2016/08/09 06:53:12, csmartdalton wrote:
> > 1
> 
> I'm assuming by offscreen in GL you just mean not FBO 0 right? If that is the
> case I do not believe there is anyway to tell if an RT is offscreen in vulkan.
> You would need to explicitly pass in some flag at create time if that is
> something you'd want to track.

Is window rectangle extension exposed through Vulkan yet? Maybe we can make the
new flag to more narrowly mean "is not window rectangle compatible" and not
worry about it setting it until an extension appears there?

[csmartdalton, 2016-08-09 15:40:47]

On 2016/08/09 13:40:16, bsalomon wrote:
> On 2016/08/09 13:19:55, egdaniel wrote:
> > On 2016/08/09 06:53:12, csmartdalton wrote:
> > I'm assuming by offscreen in GL you just mean not FBO 0 right? If that is
the
> > case I do not believe there is anyway to tell if an RT is offscreen in
vulkan.
> > You would need to explicitly pass in some flag at create time if that is
> > something you'd want to track.
> 
> Is window rectangle extension exposed through Vulkan yet? Maybe we can make
the
> new flag to more narrowly mean "is not window rectangle compatible" and not
> worry about it setting it until an extension appears there?

Yeah this extension does not exist in Vulkan. A flag that means "can use window
rectangles" would be fine, but I'm not sure what to call it. A flag specifically
about window rectangles in GrRenderTarget seems odd to me.

Is there a way we can name it where when the flag is set, it means the surface
is definitely offscreen, but when unset it means unknown?

[csmartdalton, 2016-08-09 16:13:28]

On 2016/08/09 15:40:47, csmartdalton wrote:
> On 2016/08/09 13:40:16, bsalomon wrote:
> > On 2016/08/09 13:19:55, egdaniel wrote:
> > > On 2016/08/09 06:53:12, csmartdalton wrote:
> > > I'm assuming by offscreen in GL you just mean not FBO 0 right? If that is
> the
> > > case I do not believe there is anyway to tell if an RT is offscreen in
> vulkan.
> > > You would need to explicitly pass in some flag at create time if that is
> > > something you'd want to track.
> > 
> > Is window rectangle extension exposed through Vulkan yet? Maybe we can make
> the
> > new flag to more narrowly mean "is not window rectangle compatible" and not
> > worry about it setting it until an extension appears there?
> 
> Yeah this extension does not exist in Vulkan. A flag that means "can use
window
> rectangles" would be fine, but I'm not sure what to call it. A flag
specifically
> about window rectangles in GrRenderTarget seems odd to me.
> 
> Is there a way we can name it where when the flag is set, it means the surface
> is definitely offscreen, but when unset it means unknown?

How about: renderTargetPriv.supportsWindowRectangles

[bsalomon, 2016-08-09 16:23:22]

On 2016/08/09 16:13:28, csmartdalton wrote:
> On 2016/08/09 15:40:47, csmartdalton wrote:
> > On 2016/08/09 13:40:16, bsalomon wrote:
> > > On 2016/08/09 13:19:55, egdaniel wrote:
> > > > On 2016/08/09 06:53:12, csmartdalton wrote:
> > > > I'm assuming by offscreen in GL you just mean not FBO 0 right? If that
is
> > the
> > > > case I do not believe there is anyway to tell if an RT is offscreen in
> > vulkan.
> > > > You would need to explicitly pass in some flag at create time if that is
> > > > something you'd want to track.
> > > 
> > > Is window rectangle extension exposed through Vulkan yet? Maybe we can
make
> > the
> > > new flag to more narrowly mean "is not window rectangle compatible" and
not
> > > worry about it setting it until an extension appears there?
> > 
> > Yeah this extension does not exist in Vulkan. A flag that means "can use
> window
> > rectangles" would be fine, but I'm not sure what to call it. A flag
> specifically
> > about window rectangles in GrRenderTarget seems odd to me.
> > 
> > Is there a way we can name it where when the flag is set, it means the
surface
> > is definitely offscreen, but when unset it means unknown?
> 
> How about: renderTargetPriv.supportsWindowRectangles

That sounds good to me. It could be documented to only have meaning if the caps
indicate support for rectangle windows.

[csmartdalton, 2016-08-09 16:37:49]

> It could be documented to only have meaning if the caps iindicate support for
rectangle windows.

What if it's false unless the caps don't support them? Then it's never undefined
and you only need to check one spot to see if you can use them?

[Mark Kilgard, 2016-08-09 16:53:58]

On 2016/08/09 13:40:16, bsalomon wrote:
> Is window rectangle extension exposed through Vulkan yet?

Yes, it's in the plan.  We'll share the extension with you all when available.

Because renderable Vulkan surfaces aren't really tied to window clipping since
there's not front buffered rendering or rendering into a common backbuffer,
Vulkan window rectangle testing support should be broadly available.

Keep in mind that mixed framebuffer samples is also restricted to "offscreen"
(FBO) rendering so the window rectangle testing is consistent with that.

[bsalomon, 2016-08-09 17:02:36]

On 2016/08/09 16:37:49, csmartdalton wrote:
> > It could be documented to only have meaning if the caps iindicate support
for
> rectangle windows.
> 
> What if it's false unless the caps don't support them? Then it's never
undefined
> and you only need to check one spot to see if you can use them?

sgtm

[bsalomon, 2016-08-09 17:03:36]

On 2016/08/09 16:53:58, Mark Kilgard wrote:
> On 2016/08/09 13:40:16, bsalomon wrote:
> > Is window rectangle extension exposed through Vulkan yet?
> 
> Yes, it's in the plan.  We'll share the extension with you all when available.
> 
> Because renderable Vulkan surfaces aren't really tied to window clipping since
> there's not front buffered rendering or rendering into a common backbuffer,
> Vulkan window rectangle testing support should be broadly available.
> 
> Keep in mind that mixed framebuffer samples is also restricted to "offscreen"
> (FBO) rendering so the window rectangle testing is consistent with that.

Thanks for the info. Glad to hear this is coming to Vulkan.

[Mark Kilgard, 2016-08-09 18:24:29]

markkilgard@gmail.com changed reviewers:
+ markkilgard@gmail.com

[Mark Kilgard, 2016-08-09 18:24:29]

unexpert review: seems good to me

good to see this infrastructure going in

if I understand correctly, in practice, in the browser use case, all rendering
is effectively off screen right?  seems like nanobench should reflect that usage

[bsalomon, 2016-08-09 18:47:53]

On 2016/08/09 18:24:29, Mark Kilgard wrote:
> unexpert review: seems good to me
> 
> good to see this infrastructure going in
> 
> if I understand correctly, in practice, in the browser use case, all rendering
> is effectively off screen right?  seems like nanobench should reflect that
usage

Yes, the browser always renders offscreen. nanobench renders offscreen currently
so window rectangles will be enabled.

[csmartdalton, 2016-08-09 19:08:08]

How's this? (Still need to update the commit message)

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
File include/private/GrRenderTargetProxy.h (right):

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
include/private/GrRenderTargetProxy.h:66: GrRenderTargetPriv::Flags
flags_ForTesting() const { return fFlags; }
Too ugly?

[bsalomon, 2016-08-09 19:23:22]

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
File include/private/GrRenderTargetProxy.h (right):

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
include/private/GrRenderTargetProxy.h:66: GrRenderTargetPriv::Flags
flags_ForTesting() const { return fFlags; }
On 2016/08/09 19:08:08, csmartdalton wrote:
> Too ugly?

I'm ok with it but could you move the implementation to GrTest.cpp?

[csmartdalton, 2016-08-09 22:35:16]

Description was changed from

==========
Add flag that indicates if a render target is offscreen

Adds a flag to GrRenderTarget that indicates if it is offscreen. This
will be required by window rectangles, which can only be used with
offscreen framebuffers.

Also attempts to clean up some of the mixed samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

to

==========
Add GrRenderTarget flag for window rectangles

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

[csmartdalton, 2016-08-10 17:24:58]

Description was changed from

==========
Add GrRenderTarget flag for window rectangles

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

to

==========
Add flag for window rectangles to GrRenderTarget

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

[csmartdalton, 2016-08-10 17:25:49]

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
File include/private/GrRenderTargetProxy.h (right):

https://codereview.chromium.org/2225303002/diff/20001/include/private/GrRende...
include/private/GrRenderTargetProxy.h:66: GrRenderTargetPriv::Flags
flags_ForTesting() const { return fFlags; }
On 2016/08/09 19:23:21, bsalomon wrote:
> On 2016/08/09 19:08:08, csmartdalton wrote:
> > Too ugly?
> 
> I'm ok with it but could you move the implementation to GrTest.cpp?

Done.

[bsalomon, 2016-08-10 17:44:00]

lgtm

[csmartdalton, 2016-08-10 17:44:18]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-08-10 17:44:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 18:09:10]

Description was changed from

==========
Add flag for window rectangles to GrRenderTarget

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002
==========

to

==========
Add flag for window rectangles to GrRenderTarget

Adds a flag to GrRenderTarget that indicates whether it can be used
with window rectangles. Also attempts to clean up some of the mixed
samples API.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2225303002

Committed:
https://skia.googlesource.com/skia/+/f9635999a4aa8810d04e8ef04594a9fbcc061e3d
==========

[commit-bot: I haz the power, 2016-08-10 18:09:11]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/f9635999a4aa8810d04e8ef04594a9fbcc061e3d