Create GLSLUniformHandler class for gpu backend

Create GLSLUniformHandler class for gpu backend

BUG=skia:

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

[egdaniel, 2015-12-02 21:17:27]

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

[joshualitt, 2015-12-02 21:41:08]

On 2015/12/02 21:17:28, egdaniel wrote:

I did look at every file, was there any actual logic changes, or is it mostly
just moving stuff around?

[egdaniel, 2015-12-02 22:29:37]

On 2015/12/02 21:41:08, joshualitt wrote:
> On 2015/12/02 21:17:28, egdaniel wrote:
> 
> I did look at every file, was there any actual logic changes, or is it mostly
> just moving stuff around?

I guess logically there shouldn't be any real changes. More about the
architectural changes around the new UniformHandler class and where we store
various uniform data. Also some minor changes to ProgramBuilder. Since only
ProgramBuilders helpers (shader builders, varying, uniform) really know about it
now I've exposed everything besides ctor as public so we don't need to do so
much friending. So mostly it is moving stuff from one place to another, but it
is more than a simple rename. So double check that in the moving I didn't change
any logic.

[bsalomon, 2015-12-02 23:07:25]

https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
File src/effects/SkArithmeticMode_gpu.cpp (right):

https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
src/effects/SkArithmeticMode_gpu.cpp:68: fKUni =
args.fUniformHandler->addUniform(GrGLSLUniformHandler::kFragment_Visibility,
Just wondering if it'd make sense to leave addUniform() on the builder and have
it forward on the call? It would have the side effect of not sharing all of
UniformBuilders public interface with Processor subclasses.

[egdaniel, 2015-12-02 23:56:40]

https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
File src/effects/SkArithmeticMode_gpu.cpp (right):

https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
src/effects/SkArithmeticMode_gpu.cpp:68: fKUni =
args.fUniformHandler->addUniform(GrGLSLUniformHandler::kFragment_Visibility,
On 2015/12/02 23:07:25, bsalomon wrote:
> Just wondering if it'd make sense to leave addUniform() on the builder and
have
> it forward on the call? It would have the side effect of not sharing all of
> UniformBuilders public interface with Processor subclasses.

I'm a little confused what you're suggesting here. Processors no longer have
access to the ProgramBuilder. They only get to see the ShaderBuiders,
VaryingHandler, and UniformHandler that are passed to them (some don't get
passed everything). This way we don't need to have all the weird diamond shaped
base classes of ProgramBuilder and instead the ProgramBuilder behind the scene
knows what type of helpers (shaderBuilders, varying, uniform) to send to a
specific processor type.

[bsalomon, 2015-12-03 00:15:55]

On 2015/12/02 23:56:40, egdaniel wrote:
>
https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
> File src/effects/SkArithmeticMode_gpu.cpp (right):
> 
>
https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
> src/effects/SkArithmeticMode_gpu.cpp:68: fKUni =
> args.fUniformHandler->addUniform(GrGLSLUniformHandler::kFragment_Visibility,
> On 2015/12/02 23:07:25, bsalomon wrote:
> > Just wondering if it'd make sense to leave addUniform() on the builder and
> have
> > it forward on the call? It would have the side effect of not sharing all of
> > UniformBuilders public interface with Processor subclasses.
> 
> I'm a little confused what you're suggesting here. Processors no longer have
> access to the ProgramBuilder. They only get to see the ShaderBuiders,
> VaryingHandler, and UniformHandler that are passed to them (some don't get
> passed everything). This way we don't need to have all the weird diamond
shaped
> base classes of ProgramBuilder and instead the ProgramBuilder behind the scene
> knows what type of helpers (shaderBuilders, varying, uniform) to send to a
> specific processor type.

Gotcha, I missed that the PB was no longer available. I'm mainly wondering if
there should be a veil between the uniform handler and the processors so that
the handler's interface can be rich enough to work with the builder family of
classes without exposing more than is necessary to the individual processors.
That could be a base class, a wrapper, ...

[egdaniel, 2015-12-03 00:41:01]

On 2015/12/03 00:15:55, bsalomon wrote:
> On 2015/12/02 23:56:40, egdaniel wrote:
> >
>
https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
> > File src/effects/SkArithmeticMode_gpu.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1490283004/diff/20001/src/effects/SkArithmeti...
> > src/effects/SkArithmeticMode_gpu.cpp:68: fKUni =
> > args.fUniformHandler->addUniform(GrGLSLUniformHandler::kFragment_Visibility,
> > On 2015/12/02 23:07:25, bsalomon wrote:
> > > Just wondering if it'd make sense to leave addUniform() on the builder and
> > have
> > > it forward on the call? It would have the side effect of not sharing all
of
> > > UniformBuilders public interface with Processor subclasses.
> > 
> > I'm a little confused what you're suggesting here. Processors no longer have
> > access to the ProgramBuilder. They only get to see the ShaderBuiders,
> > VaryingHandler, and UniformHandler that are passed to them (some don't get
> > passed everything). This way we don't need to have all the weird diamond
> shaped
> > base classes of ProgramBuilder and instead the ProgramBuilder behind the
scene
> > knows what type of helpers (shaderBuilders, varying, uniform) to send to a
> > specific processor type.
> 
> Gotcha, I missed that the PB was no longer available. I'm mainly wondering if
> there should be a veil between the uniform handler and the processors so that
> the handler's interface can be rich enough to work with the builder family of
> classes without exposing more than is necessary to the individual processors.
> That could be a base class, a wrapper, ...

So the processors need access to the addUniform, getUniform, and Uniform Name
functions. All of the rest are only needed by the ProgramBuilder. Given that
UniformHandler really is a utility of the ProgramBuilder I think it makes sense
to have ProgramBuilder friended by UniformHandler (actually already is), and
move all those needed functions to private. We could make an abstraction layer
(wrapper, base, etc.), but I feel like that is unneeded complexity.

[bsalomon, 2015-12-03 14:23:19]

https://codereview.chromium.org/1490283004/diff/20001/src/gpu/glsl/GrGLSLUnif...
File src/gpu/glsl/GrGLSLUniformHandler.h (right):

https://codereview.chromium.org/1490283004/diff/20001/src/gpu/glsl/GrGLSLUnif...
src/gpu/glsl/GrGLSLUniformHandler.h:62: void
addRTAdjustmentUniform(GrSLPrecision precision, const char* name, const char**
outName) {
I'm ok with not wrapping this or doing a priv extension (a la GrGpuResourcePriv)
but I don't think these functions should be in the public interface since this
is effectively a class in our public extension interface. If you prefer
friending, that's fine with me.

https://codereview.chromium.org/1490283004/diff/20001/src/gpu/glsl/GrGLSLUnif...
src/gpu/glsl/GrGLSLUniformHandler.h:86: const BuiltinUniformHandles&
builtinUniformHandles() const { return fUniformHandles; }
this as well

[egdaniel, 2015-12-03 15:14:57]

ptal. I believe this new update should address your worries about the public
functions on UniformHanlder

[egdaniel, 2015-12-03 15:16:23]

besides moving a couple things to private, the main change is that the
ProgramBuilder now holds on to the UniformHandles for the builtin uniforms which
I think makes more sense.

[bsalomon, 2015-12-03 16:05:23]

On 2015/12/03 15:16:23, egdaniel wrote:
> besides moving a couple things to private, the main change is that the
> ProgramBuilder now holds on to the UniformHandles for the builtin uniforms
which
> I think makes more sense.

definitely, lgtm

[egdaniel, 2015-12-03 16:07:38]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2015-12-03 16:07:42]

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

[commit-bot: I haz the power, 2015-12-03 17:20:47]

Description was changed from

==========
Create GLSLUniformHandler class for gpu backend

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

to

==========
Create GLSLUniformHandler class for gpu backend

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/7ea439b2203855db97330b25945b87dd4b170b8b
==========

[commit-bot: I haz the power, 2015-12-03 17:20:48]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/7ea439b2203855db97330b25945b87dd4b170b8b