Wrap NV_path_rendering API with GrGLPathRendering

Adds a GrGLPathRendering class that wraps the NV_path_rendering
extension and manages its various API versions. It also provides
backup implementations when certain NVpr methods from later API
versions are not present on the current system.

Committed: https://skia.googlesource.com/skia/+/5672da0fa54f31c9727568e9dd5fe82c6e1585bc

[Kimmo Kinnunen, 2014-07-31 08:58:50]

I'm not a reviewer, but adding version specific checks seems a bit inelegant, at
least at this point..

In the ES patch, I've just switched the mode based on checking GLStandard.

The new api does not work fully with current or upcoming desktop drivers, so 1.3
features can not probably be enabled fully on desktop yet. It will then become a
bit confusing to  check for 1.3 for stencilthencover, but still use the legacy
fixed function for texturing.

The ES patch:
https://codereview.chromium.org/367643004/

[Kimmo Kinnunen, 2014-07-31 09:37:44]

On 2014/07/31 08:58:50, kkinnunen wrote:
> I'm not a reviewer, but adding version specific checks seems a bit inelegant,
at
> least at this point..
> 
> In the ES patch, I've just switched the mode based on checking GLStandard.
> 
> The new api does not work fully with current or upcoming desktop drivers, so
1.3
..

But of course, if need be, we can inverse things and bake the driver problem
checks to the version check flag..

[bsalomon, 2014-07-31 14:02:28]

On 2014/07/31 09:37:44, kkinnunen wrote:
> On 2014/07/31 08:58:50, kkinnunen wrote:
> > I'm not a reviewer, but adding version specific checks seems a bit
inelegant,
> at
> > least at this point..
> > 
> > In the ES patch, I've just switched the mode based on checking GLStandard.
> > 
> > The new api does not work fully with current or upcoming desktop drivers, so
> 1.3
> ..
> 
> But of course, if need be, we can inverse things and bake the driver problem
> checks to the version check flag..

You guys have a much better handle on what makes sense WRT differentiating based
on versions of the extension and GLES vs GL. So I'm giving this an lgtm but
assuming you'll resolve the best way forward between yourselves before landing.

Also, I'm wondering whether the ES extension should have a different name if it
will stay significantly different for an extended period of time.

[Mark Kilgard, 2014-07-31 19:02:18]

LGTM, I have no problems with the actual code

my only quibbles are with the comments that say v1.3 when the
glStencilThenCover* commands are technically added to v1.2 which has been
shipping for some time now (like 7 months I think).

it might be more helpful to say fPathRenderingSingleSTCSupport instead of V13
just because that will be more readable

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLFunctions.h
File include/gpu/gl/GrGLFunctions.h (left):

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLFunctions...
include/gpu/gl/GrGLFunctions.h:252: 
Thought glProgramPathFragmentInputGenNV is a v1.3 feature

a "v1.3" comment here might help

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLFunctions.h
File include/gpu/gl/GrGLFunctions.h (right):

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLFunctions...
include/gpu/gl/GrGLFunctions.h:248: // NV_path_rendering v1.3
technically the glStencilThenCover* commands are part of v1.2

see the "NVIDIA Implementation Details" section of
https://www.opengl.org/registry/specs/NV/path_rendering.txt

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLInterface.h
File include/gpu/gl/GrGLInterface.h (right):

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLInterface...
include/gpu/gl/GrGLInterface.h:338: // NV_path_rendering v1.3
again, glStencilThenCover* commands are v1.2

https://codereview.chromium.org/437473002/diff/1/include/gpu/gl/GrGLInterface...
include/gpu/gl/GrGLInterface.h:342:
GLPtr<GrGLStencilThenCoverStrokePathInstancedProc>
fStencilThenCoverStrokePathInstanced;
glProgramPathFragmentInputGenNV is actually v1.3

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

https://codereview.chromium.org/437473002/diff/1/src/gpu/gl/GrGLInterface.cpp...
src/gpu/gl/GrGLInterface.cpp:483: // v1.3 methods always exist on ES
really v1.2 methods

[Chris Dalton, 2014-08-01 00:59:59]

After taking Kimmo's comments into consideration and discussing this more with
Mark and Brian, a v13 flag does seem like the wrong approach.

Brian suggested encapsulating all the nvpr calls checks into its own class and
that seems like a better path forward.

This is a work in progress, but passes gm and prototypes what a
GrGLPathRendering class would look like. It also cleans up all the path-specific
stuff out of GrGpuGL and consolidates it into one place with the rest of nvpr. I
quite like it.

I'd appreciate some feedback before I take this further.

[Kimmo Kinnunen, 2014-08-01 12:07:35]

On 2014/08/01 00:59:59, Chris Dalton wrote:
> Brian suggested encapsulating all the nvpr calls checks into its own class and
> that seems like a better path forward.
..
> I'd appreciate some feedback before I take this further.

This might not be today's issue, but:
That class might be quite redundant abstraction if in the end v1.3 is the only
version of the api we have implemented. The old implementations should be
removed once the driver is more widely available for developers. For real world
apps, eg. Chrome, no other code that v1.3(+) will ever be executed anyway.

[Kimmo Kinnunen, 2014-08-01 12:14:24]

On 2014/08/01 12:07:35, kkinnunen wrote:
> That class might be quite redundant abstraction...

And even though I'm as fond of (redundant) abstractions as the next guy, we'd
probably be better off removing things vptrs and deep callstacks, not adding
them. :)

[Chris Dalton, 2014-08-01 18:31:18]

The code in genPaths/deletePaths that manages a client-local pool of path names
will always be useful. Though it could be moved back to GrGpuGL.

It is true that in the future we will most likely just require 1.3, but since
not all public drivers on all platforms currently have full 1.3 support, we need
some form of version checking today. Plus (as much as I am opposed to "designing
for the future") I bet that once v1.3 is required, we will want version checking
for the cool new v1.6 features.

There are a lot of old drivers out there, so I'd like to have a fallback for
StencilThenCover (especially since it is so easy).

[Mark Kilgard, 2014-08-04 17:08:53]

LGTM

This is a very nice structure.  Good work Chris.

This idea would allow us to support other later features such as conics and
rounded rectangles well too.

https://codereview.chromium.org/437473002/diff/40001/src/gpu/gl/GrGLPathRende...
File src/gpu/gl/GrGLPathRendering.cpp (right):

https://codereview.chromium.org/437473002/diff/40001/src/gpu/gl/GrGLPathRende...
src/gpu/gl/GrGLPathRendering.cpp:243: GrGLint components, const GrGLfloat
*coeffs) {
good, I like this

[bsalomon, 2014-08-04 17:41:41]

lgtm, I really like encapsulating all the PR code in the helper class hierarchy.

[Chris Dalton, 2014-08-04 17:44:45]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-08-04 17:45:50]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/437473002/40001

[commit-bot: I haz the power, 2014-08-04 17:57:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[commit-bot: I haz the power, 2014-08-04 17:59:21]

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

[commit-bot: I haz the power, 2014-08-04 17:59:22]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

[Chris Dalton, 2014-08-04 18:00:10]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-08-04 18:00:41]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/437473002/60001

[Chris Dalton, 2014-08-04 18:07:46]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-08-04 18:08:00]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/437473002/80001

[commit-bot: I haz the power, 2014-08-04 18:19:17]

Change committed as 5672da0fa54f31c9727568e9dd5fe82c6e1585bc

[bungeman-skia, 2014-08-06 14:48:01]

A revert of this CL has been created in
https://codereview.chromium.org/443133003/ by bungeman@google.com.

The reason for reverting is: This appears to be blocking the Skia roll by
causing failures in the blink layout tests on the canvas-lost-gpu-context.html
test.

The bisect for this can be seen at:

https://codereview.chromium.org/449473002/.

[bsalomon, 2014-08-06 16:36:53]

On 2014/08/06 14:48:01, bungeman1 wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/443133003/ by mailto:bungeman@google.com.
> 
> The reason for reverting is: This appears to be blocking the Skia roll by
> causing failures in the blink layout tests on the canvas-lost-gpu-context.html
> test.
> 
> The bisect for this can be seen at:
> 
> https://codereview.chromium.org/449473002/.


This also repros in dm when run with --abandonGpuContext

Runs of dm with this flag are slated to be added to the valgrind and MSAN bots
soonish.


To repro the layout test from a Chromium checkout:

ninja -C out/Debug blink_tests

./webkit/tools/layout_tests/run_webkit_tests.sh
virtual/gpu/fast/canvas/canvas-lost-gpu-context.html --debug

Here is the stack trace:

[6:6:0806/122732:2624978403864:FATAL:SkTemplates.h(119)] SK_CRASH
#0 0x00000088a8ae base::debug::StackTrace::StackTrace()
#1 0x0000007ab125 logging::LogMessage::~LogMessage()
#2 0x00000157e050 SkDebugf_FileLine()
#3 0x0000013982fa SkAutoTDelete<>::operator->()
#4 0x00000139c6cb GrGpuGL::abandonResources()
#5 0x000001306c64 GrContext::abandonContext()
#6 0x000006593665 GrContext::contextDestroyed()
#7 0x00000659314b webkit::gpu::GrContextForWebGraphicsContext3D::OnLostContext()
#8 0x0000055c3b4d content::ContextProviderCommandBuffer::OnLostContext()
#9 0x0000055c4d89
content::ContextProviderCommandBuffer::LostContextCallbackProxy::onContextLost()
#10 0x0000055f4d9b
content::WebGraphicsContext3DCommandBufferImpl::OnGpuChannelLost()
#11 0x0000055f7a12 base::internal::RunnableAdapter<>::Run()
#12 0x0000055f7993 base::internal::InvokeHelper<>::MakeItSo()
#13 0x0000055f7945 base::internal::Invoker<>::Run()
#14 0x00000049153e base::Callback<>::Run()
#15 0x0000055b12ca content::CommandBufferProxyImpl::OnDestroyed()
#16 0x0000055bc496 DispatchToMethod<>()
#17 0x0000055b4bc7 GpuCommandBufferMsg_Destroyed::Dispatch<>()
#18 0x0000055b0ed1 content::CommandBufferProxyImpl::OnMessageReceived()
#19 0x0000055b164f content::CommandBufferProxyImpl::OnMessageReceived()
#20 0x0000055de2ca base::internal::RunnableAdapter<>::Run()
#21 0x0000055de228 base::internal::InvokeHelper<>::MakeItSo()
#22 0x0000055de1bc base::internal::Invoker<>::Run()
#23 0x00000049153e base::Callback<>::Run()
#24 0x0000007b5df8 base::MessageLoop::RunTask()
#25 0x0000007b611b base::MessageLoop::DeferOrRunPendingTask()
#26 0x0000007b6345 base::MessageLoop::DoWork()
#27 0x0000007c8dc8 base::MessagePumpDefault::Run()
#28 0x0000007b5730 base::MessageLoop::RunHandler()
#29 0x0000007f66a2 base::RunLoop::Run()
#30 0x0000007b4fd1 base::MessageLoop::Run()
#31 0x000004d8a175 content::RendererMain()
#32 0x00000058c096 content::RunZygote()
#33 0x00000058c3c9 content::RunNamedProcessTypeMain()
#34 0x00000058ea38 content::ContentMainRunnerImpl::Run()
#35 0x00000058b8e5 content::ContentMain()
#36 0x000000478e3c main
#37 0x7f3ee163f78d __libc_start_main
#38 0x000000478d29 <unknown>