Cache render targets that render to wrapped textures

Cache render targets that render to wrapped textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1810323002

[Kimmo Kinnunen, 2016-03-18 12:11:58]

Description was changed from

==========
Cache external render targets of textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Fixes a bug where adopted texture id fed with
SkSurface:wrapBackendTextureAsRenderTarget() would be leaked.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
==========

to

==========
Cache external render targets of textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Fixes a bug where adopted texture id fed with
SkSurface:wrapBackendTextureAsRenderTarget() would be leaked.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Kimmo Kinnunen, 2016-03-18 12:13:27]

kkinnunen@nvidia.com changed reviewers:
+ bsalomon@google.com

[Kimmo Kinnunen, 2016-03-18 12:13:28]

Submitting this for review to see if the use-case is ok, before investing time
in the VK.

[Kimmo Kinnunen, 2016-03-18 12:14:39]

Description was changed from

==========
Cache external render targets of textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Fixes a bug where adopted texture id fed with
SkSurface:wrapBackendTextureAsRenderTarget() would be leaked.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Cache render targets that render to wrapped textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Fixes a bug where adopted texture id fed with
SkSurface:wrapBackendTextureAsRenderTarget() would be leaked.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-03-18 13:29:58]

On 2016/03/18 12:13:28, Kimmo Kinnunen wrote:
> Submitting this for review to see if the use-case is ok, before investing time
> in the VK.

I don't really understand what the use case looks like from the clients
perspective (what APIs they use). Does it require that the caller use
GrTextureProvider as I don't see changes in SkSurface.h? Leaving FBOs around in
the cache will hurt performance on some tilers that have tiling data structures
associated with FBOs.

[Kimmo Kinnunen, 2016-03-18 20:00:13]

On 2016/03/18 13:29:58, bsalomon wrote:
> On 2016/03/18 12:13:28, Kimmo Kinnunen wrote:
> > Submitting this for review to see if the use-case is ok, before investing
time
> > in the VK.
> 
> I don't really understand what the use case looks like from the clients
> perspective (what APIs they use). Does it require that the caller use
> GrTextureProvider as I don't see changes in SkSurface.h? Leaving FBOs around
in
> the cache will hurt performance on some tilers that have tiling data
structures
> associated with FBOs.

The api is mentioned in the commit message?
There's no extra FBOs left in practice.
When Chrome uses the render target, it will use the surface in the cache.
The description is in the bug..

[Kimmo Kinnunen, 2016-03-18 20:02:19]

It improves chromium MSAA quite substantially

https://codereview.chromium.org/1810323002/diff/1/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1810323002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:665: GrWrapOwnership ownership) {
This is the implementation.

[Kimmo Kinnunen, 2016-03-21 12:44:51]

I can also make this applicable only to platforms that use renderbuffers, if
needed.

[bsalomon, 2016-03-21 13:58:07]

On 2016/03/21 12:44:51, Kimmo Kinnunen wrote:
> I can also make this applicable only to platforms that use renderbuffers, if
> needed.

I'm not crazy about the direction this is going. It seems like it is papering
over the fact that our render target abstraction is overloaded and conflates
different ideas. Also we shouldn't be using the resource/texture provider in the
GPU object.  If we need to resolve the performance issues in around recreating
SkSurfaces which recreate FBOs and render buffers is there a way to make
progress on the cc side by retaining SkSurfaces?

[Kimmo Kinnunen, 2016-03-21 14:20:02]

On 2016/03/21 13:58:07, bsalomon wrote:
> If we need to resolve the performance issues in around recreating
> SkSurfaces which recreate FBOs and render buffers is there a way to make
> progress on the cc side by retaining SkSurfaces?

It's tricky from the code structure pov. Nobody with better vision for CC has
replied.

There's the perspective that if you do not share the render target then each
holder will own the render target.
This in turn means that each tile holds 4x or 8x MSAA renderbuffer, which I'm
predict the Chrome guys have issue with.

[Kimmo Kinnunen, 2016-03-21 14:20:02]

On 2016/03/21 13:58:07, bsalomon wrote:
> If we need to resolve the performance issues in around recreating
> SkSurfaces which recreate FBOs and render buffers is there a way to make
> progress on the cc side by retaining SkSurfaces?

It's tricky from the code structure pov. Nobody with better vision for CC has
replied.

There's the perspective that if you do not share the render target then each
holder will own the render target.
This in turn means that each tile holds 4x or 8x MSAA renderbuffer, which I'm
predict the Chrome guys have issue with.

[Kimmo Kinnunen, 2016-03-21 14:20:03]

On 2016/03/21 13:58:07, bsalomon wrote:
> If we need to resolve the performance issues in around recreating
> SkSurfaces which recreate FBOs and render buffers is there a way to make
> progress on the cc side by retaining SkSurfaces?

It's tricky from the code structure pov. Nobody with better vision for CC has
replied.

There's the perspective that if you do not share the render target then each
holder will own the render target.
This in turn means that each tile holds 4x or 8x MSAA renderbuffer, which I'm
predict the Chrome guys have issue with.

[Kimmo Kinnunen, 2016-03-21 14:30:54]

I could imagine holding some common Skia render target in CC, and re-seating it
to different textures as opposed to current Skia API of creating a new RT for
each tile rasterization. It's just that this would be everything against the
current Skia API of "create sksurface to render".

As said, holding per-tile Skia render target might be hard in cc.

[bsalomon, 2016-03-22 13:21:17]

+Robert

I think we need to consider how MSAA render buffers will interact with the
multi-drawbuffer work and proxy render targets. There may be an initial answer
and a longer term answer to that.

[Kimmo Kinnunen, 2016-03-22 14:17:59]

On 2016/03/22 13:21:17, bsalomon wrote:
> +Robert
> 
> I think we need to consider how MSAA render buffers will interact with the
> multi-drawbuffer work and proxy render targets. There may be an initial answer
> and a longer term answer to that.

A comment, not claiming to be any sort of visionary wrt future of the system:
To me it appears that the work I did year ago in
https://bugs.chromium.org/p/skia/issues/detail?id=3388 is still relevant. E.g.
Skia could be refactored to draw to render targets, but "resolve" the results to
"textures". In this context, then "resolve" means that subsequent stages that
use the rendering would obtain read-only reference to the contents of the
rendering. This should enable reusing of small amount, potentially single,
physical render targets. The evidence pointing towards this is that  in this bug
I'm trying to solve a problem where 4 render target creations prevent reasonable
rasterization of a simple scene (chrome tiling the svg tiger). Thus it should
prove that scenes can not really allocate new physical render targets during
playback. This should be true for multi-drawbuffer too; If I understand
correctly, there is no reason to have the render targets attached to their
textures. Multi-drawbuffer should amplify the rationale of not having
rendertargets-as-textures, as the topology-sorted draw order may remove the need
for storing the render targets in the cache at all (in my limited undestanding).


However, I feel this might take a while to change (even though I already
implemented it).

It'd be great if you could reconsider the possibility of storing these "wrapped
but detached" render targets to the cache, as it really helps the real-world
Chrome. It's also not terribly different to current practice of storing render
targets to the cache. It's also otherwise not that surprising or architecturally
high-impact code (to me, naturally).  Alternatively, it'd be really great to
hear an viable alternative that is pursuable or actively pursued instead of
punting with "long term strategy". As an example: solving this probably has
bigger perf impact than any NVPR work ever done or probably ever will be done.
Of course, this might also speak about NVPR work in general, but but that's
another topic...

(The solution in issue 3388 would've solved this issue, too, btw.)
(Naturally I understand that while these are high-impact for me, these might not
be pressing to solve for example if multisampled-render-to-texture is more
important than renderbuffers)

[Kimmo Kinnunen, 2016-03-22 17:56:29]

Just to highlight: this particular change does not affect that much NVPR -- this
is improving the Skia MSAA backend sans NVPR+mixed samples.

[Kimmo Kinnunen, 2016-04-06 14:30:38]

moved the budgeting related changes to the other patch.
moved the resourceprovider access to resourceprovider, but it looks odd so
probably that was not what you had in mind.

Do you have a suggestion how should I work forward in getting the underlying
issue fixed?

[Kimmo Kinnunen, 2016-04-06 14:30:53]

Description was changed from

==========
Cache render targets that render to wrapped textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Fixes a bug where adopted texture id fed with
SkSurface:wrapBackendTextureAsRenderTarget() would be leaked.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Cache render targets that render to wrapped textures

Store render targets to the cache if they are created with
SkSurface:wrapBackendTextureAsRenderTarget(). This avoids expensive
creation and deletion of FBO objects (FBOs, renderbuffers) on
cache hit.

Leaves the wrapped texture bound to the FBO when the surface is
destroyed. The texture will not be used, rather the FBO will be
re-assigned when new surface needs it. NOTE: if some drivers have
problems with textures bound to FBOs, then this will happen here.

Speeds up cases where MSAA SkSurface is created
for rendering and then destroyed in repeated manner. Mostly benefits
HW which uses renderbuffers.

WIP: Vulkan parts missing.

BUG=594928
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-04-06 14:42:48]

On 2016/04/06 14:30:38, Kimmo Kinnunen wrote:
> moved the budgeting related changes to the other patch.
> moved the resourceprovider access to resourceprovider, but it looks odd so
> probably that was not what you had in mind.
> 
> Do you have a suggestion how should I work forward in getting the underlying
> issue fixed?

Can this not be fixed on the Chromium side by retaining the SkSurfaces for
tiles? I'd much prefer that.

[Kimmo Kinnunen, 2016-04-07 06:25:14]

On 2016/04/06 14:42:48, bsalomon wrote:
> On 2016/04/06 14:30:38, Kimmo Kinnunen wrote:
> > Do you have a suggestion how should I work forward in getting the underlying
> > issue fixed?
> 
> Can this not be fixed on the Chromium side by retaining the SkSurfaces for
> tiles? I'd much prefer that.

Would this mean that it is ok to hold multisample renderbuffers for color and
stecil for separately for each tile?