Initial version of external_oes texture support and unit test

Initial version of external_oes texture support and unit test

Committed: https://skia.googlesource.com/skia/+/27a048700778d4cebfc23301d1780649791b0e03

Committed: https://skia.googlesource.com/skia/+/7ea33f5e1ae9eb1fb1e7377d6edf6acdcf71d103

[bsalomon, 2015-11-16 15:13:19]

Description was changed from

==========
start on egl image caps

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

to

==========
start on egl image caps

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

[bsalomon, 2015-11-20 19:04:24]

bsalomon@google.com changed reviewers:
+ egdaniel@google.com, joshualitt@google.com

[bsalomon, 2015-11-20 19:04:25]

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/GrGLInt...
File include/gpu/gl/GrGLInterface.h (right):

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/GrGLInt...
include/gpu/gl/GrGLInterface.h:501: GLPtr<GrEGLCreateImageProc> fEGLCreateImage;
Decided egl funcs should be prefixed since there exist foos for which glFoo and
eglFoo both exist.

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

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2984: for (int i = 0; i < programCount; ++i) {
the code inside the loop is exactly the old code, just tabbed in, some
fCopyProgram->fCopyPrograms[i] changes, and adding the #extension string for the
external copy program.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:500: }                           fCopyPrograms[2];
Need a separate copy program for external images (uses a different sampler
variable type).

[bsalomon, 2015-11-20 19:04:41]

Description was changed from

==========
start on egl image caps

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

to

==========
Initial version of external_oes texture support and unit test
==========

[egdaniel, 2015-11-20 21:57:43]

havne't gone through test yet, but some initial comments

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
include/gpu/gl/SkGLContext.h:76: virtual SkGLContext* createNew() const { return
nullptr; }
is this for testing? if some add that to comment

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

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2830: // CopyTex(Sub)Image writes to a texture and we
have no way of dynamically wrapping and RT in a
and -> a

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:183: return
SkANGLEGLContext::Create(this->gl()->fStandard, fIsGLBackend);
does this need to call makeCurrent?

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.h:36: kExternalSampler_GLSLFeature = 1,
is this public feature enum needed?

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
File src/gpu/glsl/GrGLSLTextureSampler.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
src/gpu/glsl/GrGLSLTextureSampler.h:34: GrSLType      fSamplerType;
do you set fSampleType anywhere?

[joshualitt, 2015-11-20 21:58:39]

some nits and comments, feel free to ignore.  Looks sensible.

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

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2781: const GrSurface* src,
\t

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2807: const GrSurface* src,
\t

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2984: for (int i = 0; i < programCount; ++i) {
On 2015/11/20 19:04:25, bsalomon wrote:
> the code inside the loop is exactly the old code, just tabbed in, some
> fCopyProgram->fCopyPrograms[i] changes, and adding the #extension string for
the
> external copy program.

create_copy_program helper? feel free to ignore

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:508: return 1;
Enum? No strong preference

[bsalomon, 2015-11-20 22:43:55]

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
include/gpu/gl/SkGLContext.h:76: virtual SkGLContext* createNew() const { return
nullptr; }
On 2015/11/20 21:57:42, egdaniel wrote:
> is this for testing? if some add that to comment

The whole class is for testing. Amended the comment above the class.

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

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2781: const GrSurface* src,
On 2015/11/20 21:58:39, joshualitt wrote:
> \t

Done.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2807: const GrSurface* src,
On 2015/11/20 21:58:39, joshualitt wrote:
> \t

Done.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2830: // CopyTex(Sub)Image writes to a texture and we
have no way of dynamically wrapping and RT in a
On 2015/11/20 21:57:43, egdaniel wrote:
> and -> a

Done.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2984: for (int i = 0; i < programCount; ++i) {
On 2015/11/20 21:58:39, joshualitt wrote:
> On 2015/11/20 19:04:25, bsalomon wrote:
> > the code inside the loop is exactly the old code, just tabbed in, some
> > fCopyProgram->fCopyPrograms[i] changes, and adding the #extension string for
> the
> > external copy program.
> 
> create_copy_program helper? feel free to ignore

ok

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h#n...
src/gpu/gl/GrGLGpu.h:508: return 1;
On 2015/11/20 21:58:39, joshualitt wrote:
> Enum? No strong preference

tried typing it, wasn't clearly better.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:183: return
SkANGLEGLContext::Create(this->gl()->fStandard, fIsGLBackend);
On 2015/11/20 21:57:43, egdaniel wrote:
> does this need to call makeCurrent?

Done.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
File src/gpu/glsl/GrGLSLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
src/gpu/glsl/GrGLSLFragmentShaderBuilder.h:36: kExternalSampler_GLSLFeature = 1,
On 2015/11/20 21:57:43, egdaniel wrote:
> is this public feature enum needed?

Done.

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
File src/gpu/glsl/GrGLSLTextureSampler.h (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
src/gpu/glsl/GrGLSLTextureSampler.h:34: GrSLType      fSamplerType;
On 2015/11/20 21:57:43, egdaniel wrote:
> do you set fSampleType anywhere? 

woops, this was an old way of doing things, removed member and getter.

[joshualitt, 2015-11-20 22:47:10]

On 2015/11/20 22:43:55, bsalomon wrote:
>
https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
> File include/gpu/gl/SkGLContext.h (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/include/gpu/gl/SkGLCon...
> include/gpu/gl/SkGLContext.h:76: virtual SkGLContext* createNew() const {
return
> nullptr; }
> On 2015/11/20 21:57:42, egdaniel wrote:
> > is this for testing? if some add that to comment
> 
> The whole class is for testing. Amended the comment above the class.
> 
> https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp
> File src/gpu/gl/GrGLGpu.cpp (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:2781: const GrSurface* src,
> On 2015/11/20 21:58:39, joshualitt wrote:
> > \t
> 
> Done.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:2807: const GrSurface* src,
> On 2015/11/20 21:58:39, joshualitt wrote:
> > \t
> 
> Done.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:2830: // CopyTex(Sub)Image writes to a texture and we
> have no way of dynamically wrapping and RT in a
> On 2015/11/20 21:57:43, egdaniel wrote:
> > and -> a
> 
> Done.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:2984: for (int i = 0; i < programCount; ++i) {
> On 2015/11/20 21:58:39, joshualitt wrote:
> > On 2015/11/20 19:04:25, bsalomon wrote:
> > > the code inside the loop is exactly the old code, just tabbed in, some
> > > fCopyProgram->fCopyPrograms[i] changes, and adding the #extension string
for
> > the
> > > external copy program.
> > 
> > create_copy_program helper? feel free to ignore
> 
> ok
> 
> https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h
> File src/gpu/gl/GrGLGpu.h (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/GrGLGpu.h#n...
> src/gpu/gl/GrGLGpu.h:508: return 1;
> On 2015/11/20 21:58:39, joshualitt wrote:
> > Enum? No strong preference
> 
> tried typing it, wasn't clearly better.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
> File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
> src/gpu/gl/angle/SkANGLEGLContext.cpp:183: return
> SkANGLEGLContext::Create(this->gl()->fStandard, fIsGLBackend);
> On 2015/11/20 21:57:43, egdaniel wrote:
> > does this need to call makeCurrent?
> 
> Done.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
> File src/gpu/glsl/GrGLSLFragmentShaderBuilder.h (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLFra...
> src/gpu/glsl/GrGLSLFragmentShaderBuilder.h:36: kExternalSampler_GLSLFeature =
1,
> On 2015/11/20 21:57:43, egdaniel wrote:
> > is this public feature enum needed?
> 
> Done.
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
> File src/gpu/glsl/GrGLSLTextureSampler.h (right):
> 
>
https://codereview.chromium.org/1451683002/diff/180001/src/gpu/glsl/GrGLSLTex...
> src/gpu/glsl/GrGLSLTextureSampler.h:34: GrSLType      fSamplerType;
> On 2015/11/20 21:57:43, egdaniel wrote:
> > do you set fSampleType anywhere? 
> 
> woops, this was an old way of doing things, removed member and getter.

lgtm

[robertphillips, 2015-11-20 23:09:01]

robertphillips@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2015-11-20 23:09:02]

lgtm + nits

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

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:447: #endif
yodify ?

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:139: GrEGLint attribs[] = {
GR_EGL_GL_TEXTURE_LEVEL, 0,
tab these guys over ?

[egdaniel, 2015-11-21 01:09:34]

one more nit then lgtm

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:183: return
SkANGLEGLContext::Create(this->gl()->fStandard, fIsGLBackend);
On 2015/11/20 22:43:55, bsalomon wrote:
> On 2015/11/20 21:57:43, egdaniel wrote:
> > does this need to call makeCurrent?
> 
> Done.

need if (ctx)?

[egdaniel, 2015-11-21 01:09:34]

one more nit then lgtm

[bsalomon, 2015-11-21 01:50:32]

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/180001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:183: return
SkANGLEGLContext::Create(this->gl()->fStandard, fIsGLBackend);
On 2015/11/21 01:09:34, egdaniel wrote:
> On 2015/11/20 22:43:55, bsalomon wrote:
> > On 2015/11/20 21:57:43, egdaniel wrote:
> > > does this need to call makeCurrent?
> > 
> > Done.
> 
> need if (ctx)?

Done.

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

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:447: #endif
On 2015/11/20 23:09:02, robertphillips wrote:
> yodify ?

Done.

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/angle/SkANG...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

https://codereview.chromium.org/1451683002/diff/220001/src/gpu/gl/angle/SkANG...
src/gpu/gl/angle/SkANGLEGLContext.cpp:139: GrEGLint attribs[] = {
GR_EGL_GL_TEXTURE_LEVEL, 0,
On 2015/11/20 23:09:02, robertphillips wrote:
> tab these guys over ?

Done.

[bsalomon, 2015-11-21 02:00:37]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-21 02:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com, egdaniel@google.com, robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1451683002/#ps260001
(title: "address comments")

[commit-bot: I haz the power, 2015-11-21 02:00:50]

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

[commit-bot: I haz the power, 2015-11-21 03:34:40]

Committed patchset #14 (id:260001) as
https://skia.googlesource.com/skia/+/27a048700778d4cebfc23301d1780649791b0e03

[bsalomon, 2015-11-21 04:24:14]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1459323004/ by bsalomon@google.com.

The reason for reverting is: Error wrapping external texture in GrTexture on
some devices..

[bsalomon, 2015-11-21 21:15:03]

Description was changed from

==========
Initial version of external_oes texture support and unit test

Committed:
https://skia.googlesource.com/skia/+/27a048700778d4cebfc23301d1780649791b0e03
==========

to

==========
Initial version of external_oes texture support and unit test

Committed:
https://skia.googlesource.com/skia/+/27a048700778d4cebfc23301d1780649791b0e03
==========

[bsalomon, 2015-11-22 22:38:43]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-22 22:38:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com, egdaniel@google.com, robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1451683002/#ps320001
(title: "again")

[commit-bot: I haz the power, 2015-11-22 22:38:51]

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

[commit-bot: I haz the power, 2015-11-22 22:51:03]

Committed patchset #17 (id:320001) as
https://skia.googlesource.com/skia/+/7ea33f5e1ae9eb1fb1e7377d6edf6acdcf71d103