skia: add ability to load command_buffer_gles2

skia: add ability to load command_buffer_gles2

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/885bf0925514b9dfe3365bab227d36897d866b5d

[hendrikw, 2015-08-25 23:07:56]

Patchset #4 (id:60001) has been deleted

[hendrikw, 2015-08-26 18:10:27]

hendrikw@chromium.org changed reviewers:
+ bsalomon@google.com

[hendrikw, 2015-08-26 18:10:27]

PTAL, I think most of this is ok, but probably not the SkOSWindow changes, still
looking at this, pls let me know what you think so far, thanks!

[bsalomon, 2015-08-26 18:19:55]

Mostly seems fine. Does SampleApp actually work through the command buffer?

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
File src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp (right):

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:40: static HMODULE
ghLibrary = nullptr;
Should we use SkONCE here for thread safety?

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:158: if
(!gfFunctionsLoadedSuccessfully)
style nit, {} and 4 space indent

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:179: if
(!gfFunctionsLoadedSuccessfully)
{}/4

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:187: if
(!gfFunctionsLoadedSuccessfully)
{}/4

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:195: if
(!gfFunctionsLoadedSuccessfully)
{}/4

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
File src/views/win/SkOSWindow_win.cpp (right):

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
src/views/win/SkOSWindow_win.cpp:546: bool
create_CommandBuffer(EGLNativeWindowType hWnd,
Can a lot of this be shared with the ANGLE function? Maybe everything except
getting the display?

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
src/views/win/SkOSWindow_win.cpp:569: EGLDisplay display =
SkANGLEGLContext::GetD3DEGLDisplay(GetDC(hWnd));
Seems a bit weird to use angle here to get the display. Should you expose it
through SkCommandBufferGLContext?

[hendrikw, 2015-08-27 01:28:35]

PTAL, thanks!!!!!

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
File src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp (right):

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:40: static HMODULE
ghLibrary = nullptr;
On 2015/08/26 18:19:55, bsalomon wrote:
> Should we use SkONCE here for thread safety?

Done.

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:158: if
(!gfFunctionsLoadedSuccessfully)
On 2015/08/26 18:19:55, bsalomon wrote:
> style nit, {} and 4 space indent

Done.

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:179: if
(!gfFunctionsLoadedSuccessfully)
On 2015/08/26 18:19:55, bsalomon wrote:
> {}/4

Done.

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:187: if
(!gfFunctionsLoadedSuccessfully)
On 2015/08/26 18:19:55, bsalomon wrote:
> {}/4

Done.

https://codereview.chromium.org/1306823003/diff/100001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:195: if
(!gfFunctionsLoadedSuccessfully)
On 2015/08/26 18:19:55, bsalomon wrote:
> {}/4

Done.

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
File src/views/win/SkOSWindow_win.cpp (right):

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
src/views/win/SkOSWindow_win.cpp:546: bool
create_CommandBuffer(EGLNativeWindowType hWnd,
On 2015/08/26 18:19:55, bsalomon wrote:
> Can a lot of this be shared with the ANGLE function? Maybe everything except
> getting the display?

All this code is bogus, I'm replacing it with the context, we should do
something similar with ANGLE as it reduces some code duplication.

https://codereview.chromium.org/1306823003/diff/100001/src/views/win/SkOSWind...
src/views/win/SkOSWindow_win.cpp:569: EGLDisplay display =
SkANGLEGLContext::GetD3DEGLDisplay(GetDC(hWnd));
On 2015/08/26 18:19:55, bsalomon wrote:
> Seems a bit weird to use angle here to get the display. Should you expose it
> through SkCommandBufferGLContext?

All of this code is gone, yay.

[bsalomon, 2015-08-27 13:47:14]

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
File include/gpu/gl/command_buffer/SkCommandBufferGLContext.h (right):

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:32:
SkCommandBufferGLContext* ctx = SkNEW_ARGS(SkCommandBufferGLContext,
can just use plain old new. (We're expunging the macros)

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:35: SkDELETE(ctx);
delete

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
File src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp (right):

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:46: static void
LoadCommandBufferFunctions()
hacker_style_for_nonclass_statics

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:47: {
{ on prev line

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:249: SkAutoTUnref<const
GrGLInterface> intf(GrGLCreateCommandBufferInterface());
I think you can just use this->gl() to get a GrGLInterface rather than
creating/destroying one here.

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:254:
onPlatformSwapBuffers();
this->

https://codereview.chromium.org/1306823003/diff/120001/src/views/win/SkOSWind...
File src/views/win/SkOSWindow_win.cpp (right):

https://codereview.chromium.org/1306823003/diff/120001/src/views/win/SkOSWind...
src/views/win/SkOSWindow_win.cpp:556: fCommandBuffer =
SkCommandBufferGLContext::Create((HWND)fHWND, msaaSampleCount);
I like using the SkGLContext here... nice improvement

[hendrikw, 2015-08-27 14:41:58]

PTAL, thanks!

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
File include/gpu/gl/command_buffer/SkCommandBufferGLContext.h (right):

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:32:
SkCommandBufferGLContext* ctx = SkNEW_ARGS(SkCommandBufferGLContext,
On 2015/08/27 13:47:13, bsalomon wrote:
> can just use plain old new. (We're expunging the macros)

Ah, great, I wasn't a big fan of these macros :D

https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:35: SkDELETE(ctx);
On 2015/08/27 13:47:13, bsalomon wrote:
> delete

Done.

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
File src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp (right):

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:46: static void
LoadCommandBufferFunctions()
On 2015/08/27 13:47:13, bsalomon wrote:
> hacker_style_for_nonclass_statics

Done.

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:47: {
On 2015/08/27 13:47:13, bsalomon wrote:
> { on prev line

Done.

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:249: SkAutoTUnref<const
GrGLInterface> intf(GrGLCreateCommandBufferInterface());
On 2015/08/27 13:47:13, bsalomon wrote:
> I think you can just use this->gl() to get a GrGLInterface rather than
> creating/destroying one here.

Done.

https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:254:
onPlatformSwapBuffers();
On 2015/08/27 13:47:13, bsalomon wrote:
> this->

Done.

[bsalomon, 2015-08-27 15:29:05]

On 2015/08/27 14:41:58, hendrikw wrote:
> PTAL, thanks!
> 
>
https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
> File include/gpu/gl/command_buffer/SkCommandBufferGLContext.h (right):
> 
>
https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
> include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:32:
> SkCommandBufferGLContext* ctx = SkNEW_ARGS(SkCommandBufferGLContext,
> On 2015/08/27 13:47:13, bsalomon wrote:
> > can just use plain old new. (We're expunging the macros)
> 
> Ah, great, I wasn't a big fan of these macros :D
> 
>
https://codereview.chromium.org/1306823003/diff/120001/include/gpu/gl/command...
> include/gpu/gl/command_buffer/SkCommandBufferGLContext.h:35: SkDELETE(ctx);
> On 2015/08/27 13:47:13, bsalomon wrote:
> > delete
> 
> Done.
> 
>
https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
> File src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp (right):
> 
>
https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
> src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:46: static void
> LoadCommandBufferFunctions()
> On 2015/08/27 13:47:13, bsalomon wrote:
> > hacker_style_for_nonclass_statics
> 
> Done.
> 
>
https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
> src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:47: {
> On 2015/08/27 13:47:13, bsalomon wrote:
> > { on prev line
> 
> Done.
> 
>
https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
> src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:249: SkAutoTUnref<const
> GrGLInterface> intf(GrGLCreateCommandBufferInterface());
> On 2015/08/27 13:47:13, bsalomon wrote:
> > I think you can just use this->gl() to get a GrGLInterface rather than
> > creating/destroying one here.
> 
> Done.
> 
>
https://codereview.chromium.org/1306823003/diff/120001/src/gpu/gl/command_buf...
> src/gpu/gl/command_buffer/SkCommandBufferGLContext.cpp:254:
> onPlatformSwapBuffers();
> On 2015/08/27 13:47:13, bsalomon wrote:
> > this->
> 
> Done.

lgtm

[hendrikw, 2015-08-27 16:27:16]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-08-27 16:27:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823003/140001

[commit-bot: I haz the power, 2015-08-27 16:28:11]

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

[commit-bot: I haz the power, 2015-08-27 16:28:11]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[hendrikw, 2015-08-27 17:32:56]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-27 17:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1306823003/#ps180001
(title: "and NULL->nullptr, just because.")

[commit-bot: I haz the power, 2015-08-27 17:33:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306823003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306823003/180001

[commit-bot: I haz the power, 2015-08-27 17:38:45]

Committed patchset #9 (id:180001) as
https://skia.googlesource.com/skia/+/885bf0925514b9dfe3365bab227d36897d866b5d