Improve Ganesh helpers.

Improve Ganesh helpers.

Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.

R=abarth@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/297d5e5e81b07d9a0a0576a86fd43e4efbbe2d70

[jeffbrown, 2015-12-18 00:05:00]

Description was changed from

==========
Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

R=abarth, jamesr, viettrungluu
BUG=
==========

to

==========
Improve Ganesh helpers.
    
    Properly reset and flush the Ganesh context when needed.
    
    When intermixing Ganesh rendering with other GL operations, the
    Ganesh renderer's cached GPU state can get out of sync with the
    actual state of the GL context, resulting in broken behavior.
    
    To fix this problem, reset the Ganesh context when entering its
    scope and flush it when exiting its scope.  This operation has a
    small performance cost but only when entering and exiting the scope
    which should be infrequent (ideally no more than once per frame).
    
    Added a helper for creating SkImages from a texture and updated
    the GaneshTextureSurface to use the new GrGLTextureInfo type
    which was introduced in the latest Skia roll.
    
    Moved helpers into the mojo::skia namespace.
==========

[jeffbrown, 2015-12-18 00:06:26]

Description was changed from

==========
Improve Ganesh helpers.
    
    Properly reset and flush the Ganesh context when needed.
    
    When intermixing Ganesh rendering with other GL operations, the
    Ganesh renderer's cached GPU state can get out of sync with the
    actual state of the GL context, resulting in broken behavior.
    
    To fix this problem, reset the Ganesh context when entering its
    scope and flush it when exiting its scope.  This operation has a
    small performance cost but only when entering and exiting the scope
    which should be infrequent (ideally no more than once per frame).
    
    Added a helper for creating SkImages from a texture and updated
    the GaneshTextureSurface to use the new GrGLTextureInfo type
    which was introduced in the latest Skia roll.
    
    Moved helpers into the mojo::skia namespace.
==========

to

==========
Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.
==========

[viettrungluu, 2015-12-18 16:34:48]

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
File mojo/skia/ganesh_context.cc (right):

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:23: DCHECK(context_);
I wonder if we shouldn't just inline the definition of Scope.

(The DCHECK is of pretty limited value, since it'll almost certainly crash in an
obvious way if |context_| is null.)

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:72: if (!scope_nest_count_) {
(jamesr or abarth should probably look at this, and decide if we really want to
allow nested scopes, but see my comments below.)

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:81: gr_context_->resetContext();
If the intent is to allow scopes from different GaneshContexts to be nested,
then you'd have to flush the previous "gr_context_" here, right? (And then, on
exiting the scope, you'd have to reset the previous gr_context_ as well.)

So I guess that doesn't work/isn't allowed, so you're only allowed to nest
scopes from the GaneshContext? I'm not sure that's a good idea, since it
encourages a certain level of sloppiness (for reasons similar to why I don't
like recursive mutexes).

(TBH, I'm not a big fan of GaneshContext::Scope in general. IIRC, in some
previous life, I was going to delete it.)

[jeffbrown, 2015-12-18 17:48:24]

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
File mojo/skia/ganesh_context.cc (right):

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:23: DCHECK(context_);
On 2015/12/18 16:34:48, viettrungluu wrote:
> I wonder if we shouldn't just inline the definition of Scope.
> 
> (The DCHECK is of pretty limited value, since it'll almost certainly crash in
an
> obvious way if |context_| is null.)

Yeah that's a good idea.  Now that it's just calling EnterScope / ExitScope
there isn't much else for this function to do.

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:81: gr_context_->resetContext();
On 2015/12/18 16:34:48, viettrungluu wrote:
> If the intent is to allow scopes from different GaneshContexts to be nested,
> then you'd have to flush the previous "gr_context_" here, right? (And then, on
> exiting the scope, you'd have to reset the previous gr_context_ as well.)
> 
> So I guess that doesn't work/isn't allowed, so you're only allowed to nest
> scopes from the GaneshContext? I'm not sure that's a good idea, since it
> encourages a certain level of sloppiness (for reasons similar to why I don't
> like recursive mutexes).
> 
> (TBH, I'm not a big fan of GaneshContext::Scope in general. IIRC, in some
> previous life, I was going to delete it.)

FWIW, I'm with you on recursive mutexes (yuck).  Open to suggestion as to
whether to support recursive GaneshContexts.

The use-case this appeared in was in some helper code which gets passed a
GaneshContext* to do a little bit of work.  For safety, it enters a Scope at the
very beginning of the function.  If the caller also happened to be in the same
scope then recursion occurs and it would be unfortunate if we flushed / reset
the context repeatedly across this transition.

An alternative solution would be to change the helpers to take a GrContext*
instead so we move responsibility for managing the scope to a higher level.  A
little inconvenient but perhaps better.

Another approach would be to pass in the Scope itself.  We could move the
"gr_context()" and "gl_context()" methods to the Scope so there's never any
ambiguity about whether the scope has been entered, assuming it exists. 
IsInScope() can go away.

Thoughts?

[viettrungluu, 2015-12-18 18:05:41]

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
File mojo/skia/ganesh_context.cc (right):

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:81: gr_context_->resetContext();
On 2015/12/18 17:48:24, jeffbrown wrote:
> On 2015/12/18 16:34:48, viettrungluu wrote:
> > If the intent is to allow scopes from different GaneshContexts to be nested,
> > then you'd have to flush the previous "gr_context_" here, right? (And then,
on
> > exiting the scope, you'd have to reset the previous gr_context_ as well.)
> > 
> > So I guess that doesn't work/isn't allowed, so you're only allowed to nest
> > scopes from the GaneshContext? I'm not sure that's a good idea, since it
> > encourages a certain level of sloppiness (for reasons similar to why I don't
> > like recursive mutexes).
> > 
> > (TBH, I'm not a big fan of GaneshContext::Scope in general. IIRC, in some
> > previous life, I was going to delete it.)
> 
> FWIW, I'm with you on recursive mutexes (yuck).  Open to suggestion as to
> whether to support recursive GaneshContexts.
> 
> The use-case this appeared in was in some helper code which gets passed a
> GaneshContext* to do a little bit of work.  For safety, it enters a Scope at
the
> very beginning of the function.  If the caller also happened to be in the same
> scope then recursion occurs and it would be unfortunate if we flushed / reset
> the context repeatedly across this transition.
> 
> An alternative solution would be to change the helpers to take a GrContext*
> instead so we move responsibility for managing the scope to a higher level.  A
> little inconvenient but perhaps better.

I think my tendency would be to go with something like this.

I think I dislike GaneshContext::Scope because entering/leaving a Ganesh context
(well, really, using Ganesh at all) actually has side effects on the GL context,
so callers of any function that uses Ganesh need to know that.

> Another approach would be to pass in the Scope itself.  We could move the
> "gr_context()" and "gl_context()" methods to the Scope so there's never any
> ambiguity about whether the scope has been entered, assuming it exists. 
> IsInScope() can go away.
> 
> Thoughts?

[jeffbrown, 2015-12-18 18:12:57]

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
File mojo/skia/ganesh_context.cc (right):

https://codereview.chromium.org/1534033002/diff/60001/mojo/skia/ganesh_contex...
mojo/skia/ganesh_context.cc:81: gr_context_->resetContext();
On 2015/12/18 18:05:41, viettrungluu wrote:
> On 2015/12/18 17:48:24, jeffbrown wrote:
> > On 2015/12/18 16:34:48, viettrungluu wrote:
> > > If the intent is to allow scopes from different GaneshContexts to be
nested,
> > > then you'd have to flush the previous "gr_context_" here, right? (And
then,
> on
> > > exiting the scope, you'd have to reset the previous gr_context_ as well.)
> > > 
> > > So I guess that doesn't work/isn't allowed, so you're only allowed to nest
> > > scopes from the GaneshContext? I'm not sure that's a good idea, since it
> > > encourages a certain level of sloppiness (for reasons similar to why I
don't
> > > like recursive mutexes).
> > > 
> > > (TBH, I'm not a big fan of GaneshContext::Scope in general. IIRC, in some
> > > previous life, I was going to delete it.)
> > 
> > FWIW, I'm with you on recursive mutexes (yuck).  Open to suggestion as to
> > whether to support recursive GaneshContexts.
> > 
> > The use-case this appeared in was in some helper code which gets passed a
> > GaneshContext* to do a little bit of work.  For safety, it enters a Scope at
> the
> > very beginning of the function.  If the caller also happened to be in the
same
> > scope then recursion occurs and it would be unfortunate if we flushed /
reset
> > the context repeatedly across this transition.
> > 
> > An alternative solution would be to change the helpers to take a GrContext*
> > instead so we move responsibility for managing the scope to a higher level. 
A
> > little inconvenient but perhaps better.
> 
> I think my tendency would be to go with something like this.
> 
> I think I dislike GaneshContext::Scope because entering/leaving a Ganesh
context
> (well, really, using Ganesh at all) actually has side effects on the GL
context,
> so callers of any function that uses Ganesh need to know that.
> 
> > Another approach would be to pass in the Scope itself.  We could move the
> > "gr_context()" and "gl_context()" methods to the Scope so there's never any
> > ambiguity about whether the scope has been entered, assuming it exists. 
> > IsInScope() can go away.
> > 
> > Thoughts?

Sounds good.  I'll do that.

[jeffbrown, 2016-01-16 01:12:19]

Description was changed from

==========
Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.
==========

to

==========
Improve Ganesh helpers.

Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.
==========

[jeffbrown, 2016-01-16 01:14:53]

Can someone take a quick look at this?  I applied Trung's feedback from before
the holidays.  It seems to work quite well in this shape and I've found the
scoping to be a little less error-prone.

[abarth, 2016-01-18 04:18:40]

LGTM

Not sure if you want to hear back from trung before landing.

https://codereview.chromium.org/1534033002/diff/110001/mojo/skia/ganesh_helpe...
File mojo/skia/ganesh_helpers.h (right):

https://codereview.chromium.org/1534033002/diff/110001/mojo/skia/ganesh_helpe...
mojo/skia/ganesh_helpers.h:6: #define MOJO_SKIA_GANESH_HELPERS_H_
ganesh_helpers.h isn't a great name.  It's vague and likely to become a dumping
ground for random code.  How about create_image_from_texture.h ?

[jeffbrown, 2016-01-26 09:02:46]

rebase

[jeffbrown, 2016-01-26 23:50:16]

Description was changed from

==========
Improve Ganesh helpers.

Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.
==========

to

==========
Improve Ganesh helpers.

Properly reset and flush the Ganesh context when needed.

When intermixing Ganesh rendering with other GL operations, the
Ganesh renderer's cached GPU state can get out of sync with the
actual state of the GL context, resulting in broken behavior.

To fix this problem, reset the Ganesh context when entering its
scope and flush it when exiting its scope.  This operation has a
small performance cost but only when entering and exiting the scope
which should be infrequent (ideally no more than once per frame).

Added a helper for creating SkImages from a texture and updated
the GaneshTextureSurface to use the new GrGLTextureInfo type
which was introduced in the latest Skia roll.

Moved helpers into the mojo::skia namespace.

R=abarth@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/297d5e5e81b07d9a0a0576a86fd...
==========

[jeffbrown, 2016-01-26 23:50:17]

Committed patchset #8 (id:130001) manually as
297d5e5e81b07d9a0a0576a86fd43e4efbbe2d70 (presubmit successful).