Add #define directives to GrGLSLShaderBuilder

Add #define directives to GrGLSLShaderBuilder

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1660813002

Committed: https://skia.googlesource.com/skia/+/d36f2c4bc047c2c531d73eee08daa5926e8cdaf2

[Chris Dalton, 2016-02-03 01:59:31]

Description was changed from

==========
Add declAppendf for all glsl shaders

Adds a mechanism for builders to add specific declarations at the top
of the shader.

- Moves declAppendf from GrGLSLFragmentBuilder into GrGLSLShaderBuilder

- Renames the existing declAppend in GrGLSLShaderBuilder to
  codeAppendDecl

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

to

==========
Add declAppendf for all glsl shaders

Adds a mechanism for builders to add specific declarations at the top
of the shader.

- Moves declAppendf from GrGLSLFragmentBuilder into GrGLSLShaderBuilder

- Renames the existing declAppend in GrGLSLShaderBuilder to
  codeAppendDecl

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-02-03 02:00:50]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, ethannicholas@google.com

[bsalomon, 2016-02-04 16:15:08]

What's the use case for the more flexible declAppendf?

[Chris Dalton, 2016-02-04 16:48:16]

For instanced shape rendering it's here:
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...

 - #define ...
 - static const float sampleLocations[] = {...};
 - layout(override_coverage) out int gl_SampleMask[1];

declAppendf was recently added to fragment shaders for PLS path rendering to
declare its __pixel_localEXT struct:
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...

This change essentially just moves it from FragmentBuilder to ShaderBuilder, and
renames the existing declAppend function in ShaderBuilder in order to avoid
confusion.

[bsalomon, 2016-02-04 16:59:43]

On 2016/02/04 16:48:16, Chris Dalton wrote:
> For instanced shape rendering it's here:
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> 
>  - #define ...
>  - static const float sampleLocations[] = {...};
>  - layout(override_coverage) out int gl_SampleMask[1];
> 
> declAppendf was recently added to fragment shaders for PLS path rendering to
> declare its __pixel_localEXT struct:
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> 
> This change essentially just moves it from FragmentBuilder to ShaderBuilder,
and
> renames the existing declAppend function in ShaderBuilder in order to avoid
> confusion.have arbitrary 

I was hesitant about that aspect of the PLS change and now feeling buyer's
remorse. It seems like we have a way for individual processors to add arbitrary
top level code which breaks our abstraction. Can we instead add helpers for
adding macro definitions and use a different mechanism for gl_SampleMask (maybe
get the var from the builder and use the GrGLShaderVar mechanism for declaring
it)?

[ethannicholas, 2016-02-04 17:38:02]

On 2016/02/04 16:59:43, bsalomon wrote:
> On 2016/02/04 16:48:16, Chris Dalton wrote:
> > For instanced shape rendering it's here:
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > 
> >  - #define ...
> >  - static const float sampleLocations[] = {...};
> >  - layout(override_coverage) out int gl_SampleMask[1];
> > 
> > declAppendf was recently added to fragment shaders for PLS path rendering to
> > declare its __pixel_localEXT struct:
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > 
> > This change essentially just moves it from FragmentBuilder to ShaderBuilder,
> and
> > renames the existing declAppend function in ShaderBuilder in order to avoid
> > confusion.have arbitrary 
> 
> I was hesitant about that aspect of the PLS change and now feeling buyer's
> remorse. It seems like we have a way for individual processors to add
arbitrary
> top level code which breaks our abstraction. Can we instead add helpers for
> adding macro definitions and use a different mechanism for gl_SampleMask
(maybe
> get the var from the builder and use the GrGLShaderVar mechanism for declaring
> it)?

I had actually intended to work out a better way to handle that before review,
and during the intervening months it slipped through the cracks. I certainly did
not intend to endorse that as the right way to do things.

[Chris Dalton, 2016-02-04 17:41:18]

On 2016/02/04 16:59:43, bsalomon wrote:
> On 2016/02/04 16:48:16, Chris Dalton wrote:
> > For instanced shape rendering it's here:
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > 
> >  - #define ...
> >  - static const float sampleLocations[] = {...};
> >  - layout(override_coverage) out int gl_SampleMask[1];
> > 
> > declAppendf was recently added to fragment shaders for PLS path rendering to
> > declare its __pixel_localEXT struct:
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > 
> > This change essentially just moves it from FragmentBuilder to ShaderBuilder,
> and
> > renames the existing declAppend function in ShaderBuilder in order to avoid
> > confusion.have arbitrary 
> 
> I was hesitant about that aspect of the PLS change and now feeling buyer's
> remorse. It seems like we have a way for individual processors to add
arbitrary
> top level code which breaks our abstraction. Can we instead add helpers for
> adding macro definitions and use a different mechanism for gl_SampleMask
(maybe
> get the var from the builder and use the GrGLShaderVar mechanism for declaring
> it)?

In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an array
length of one and a layout qualifier of "override_coverage" and then called
GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were you
suggesting that the fragment builder itself should know about gl_SampleMask?
Maybe an enum of all the built-ins and the ability to redeclare one with layout
qualifiers?

And what about static const int sampleMask[]? Add some sort of mechanism for
defining const GrGLSLShaderVars at the top as well?

I see how this all could work, I'm just trying to understand the benefit of
abstracting how code is written to the top of the file when you can append raw
strings into the function body? We also need to be careful not to make the
resulting C++ code harder to read and maintain than if it emitted raw strings...

[Chris Dalton, 2016-02-04 19:27:56]

On 2016/02/04 17:41:18, Chris Dalton wrote:
> On 2016/02/04 16:59:43, bsalomon wrote:
> > On 2016/02/04 16:48:16, Chris Dalton wrote:
> > > For instanced shape rendering it's here:
> > >
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > > 
> > >  - #define ...
> > >  - static const float sampleLocations[] = {...};
> > >  - layout(override_coverage) out int gl_SampleMask[1];
> > > 
> > > declAppendf was recently added to fragment shaders for PLS path rendering
to
> > > declare its __pixel_localEXT struct:
> > >
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > > 
> > > This change essentially just moves it from FragmentBuilder to
ShaderBuilder,
> > and
> > > renames the existing declAppend function in ShaderBuilder in order to
avoid
> > > confusion.have arbitrary 
> > 
> > I was hesitant about that aspect of the PLS change and now feeling buyer's
> > remorse. It seems like we have a way for individual processors to add
> arbitrary
> > top level code which breaks our abstraction. Can we instead add helpers for
> > adding macro definitions and use a different mechanism for gl_SampleMask
> (maybe
> > get the var from the builder and use the GrGLShaderVar mechanism for
declaring
> > it)?
> 
> In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an
array
> length of one and a layout qualifier of "override_coverage" and then called
> GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were you
> suggesting that the fragment builder itself should know about gl_SampleMask?
> Maybe an enum of all the built-ins and the ability to redeclare one with
layout
> qualifiers?
> 
> And what about static const int sampleMask[]? Add some sort of mechanism for
> defining const GrGLSLShaderVars at the top as well?
> 
> I see how this all could work, I'm just trying to understand the benefit of
> abstracting how code is written to the top of the file when you can append raw
> strings into the function body? We also need to be careful not to make the
> resulting C++ code harder to read and maintain than if it emitted raw
strings...

Ok, so the fragment builder can add the override_coverage on gl_SampleMask
inside enableFeature.

For defines we can add a simple helper.

What about "const char sampleOffsets[] = {...}"? I could just add that inline
with codeAppendf, but I do like it better at the top of the file. We would need
to add support for the const keyword, and for the initializer list.

[bsalomon, 2016-02-04 19:53:06]

On 2016/02/04 17:41:18, Chris Dalton wrote:
> On 2016/02/04 16:59:43, bsalomon wrote:
> > On 2016/02/04 16:48:16, Chris Dalton wrote:
> > > For instanced shape rendering it's here:
> > >
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > > 
> > >  - #define ...
> > >  - static const float sampleLocations[] = {...};
> > >  - layout(override_coverage) out int gl_SampleMask[1];
> > > 
> > > declAppendf was recently added to fragment shaders for PLS path rendering
to
> > > declare its __pixel_localEXT struct:
> > >
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > > 
> > > This change essentially just moves it from FragmentBuilder to
ShaderBuilder,
> > and
> > > renames the existing declAppend function in ShaderBuilder in order to
avoid
> > > confusion.have arbitrary 
> > 
> > I was hesitant about that aspect of the PLS change and now feeling buyer's
> > remorse. It seems like we have a way for individual processors to add
> arbitrary
> > top level code which breaks our abstraction. Can we instead add helpers for
> > adding macro definitions and use a different mechanism for gl_SampleMask
> (maybe
> > get the var from the builder and use the GrGLShaderVar mechanism for
declaring
> > it)?
> 
> In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an
array
> length of one and a layout qualifier of "override_coverage" and then called
> GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were you
> suggesting that the fragment builder itself should know about gl_SampleMask?
> Maybe an enum of all the built-ins and the ability to redeclare one with
layout
> qualifiers?

I think the builder should know about it and make it available (similar to how
the dst color works in the fragment shaders). It means it won't get doubly
defined if in the future two processors wanted to use it. Also, if there are
ever extensions that offer similar functionality under a different name or rev
of GLSL that deprecates it in favor of a different mechanism (a la named shader
outputs instead of gl_FragColor) we don't have to update individual processors.

> 
> And what about static const int sampleMask[]? Add some sort of mechanism for
> defining const GrGLSLShaderVars at the top as well?
> 
> I see how this all could work, I'm just trying to understand the benefit of
> abstracting how code is written to the top of the file when you can append raw
> strings into the function body? We also need to be careful not to make the
> resulting C++ code harder to read and maintain than if it emitted raw
strings...

Frankly, the processors' shader code generators are already hard to read and I
don't think this will make it significantly more difficult. Long term we know we
need a replacement for the GrGL*Builders and Gr*Processor::emitCode(). Processor
subclasses are supposed to have constrained interactions with the shading
language. It's not a security issue and can easily be defeated (e.g. a processor
can close its enclosing block, add code and then open a new block) but is a
mental model. The constraints help us assign responsibilities for different
parts of shader generation to different processor subclasses, refactor how code
gets emitted (e.g. we could decide to place each processor's code in a
function), and have a easier way of recreating the processors for an alternate
backend in the future (e.g. our own skia-centric shading language or a different
API's shading language). It's a gray line but proving a mechanism for a fragment
processor, for example, to add arbitrary varyings to the fragment shader is too
far IMO and if our code looks like that's a supported feature then we'll start
seeing changes from well meaning contributors that try to use it.

[bsalomon, 2016-02-04 19:58:54]

On 2016/02/04 19:27:56, Chris Dalton wrote:
> On 2016/02/04 17:41:18, Chris Dalton wrote:
> > On 2016/02/04 16:59:43, bsalomon wrote:
> > > On 2016/02/04 16:48:16, Chris Dalton wrote:
> > > > For instanced shape rendering it's here:
> > > >
> > >
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > > > 
> > > >  - #define ...
> > > >  - static const float sampleLocations[] = {...};
> > > >  - layout(override_coverage) out int gl_SampleMask[1];
> > > > 
> > > > declAppendf was recently added to fragment shaders for PLS path
rendering
> to
> > > > declare its __pixel_localEXT struct:
> > > >
> > >
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > > > 
> > > > This change essentially just moves it from FragmentBuilder to
> ShaderBuilder,
> > > and
> > > > renames the existing declAppend function in ShaderBuilder in order to
> avoid
> > > > confusion.have arbitrary 
> > > 
> > > I was hesitant about that aspect of the PLS change and now feeling buyer's
> > > remorse. It seems like we have a way for individual processors to add
> > arbitrary
> > > top level code which breaks our abstraction. Can we instead add helpers
for
> > > adding macro definitions and use a different mechanism for gl_SampleMask
> > (maybe
> > > get the var from the builder and use the GrGLShaderVar mechanism for
> declaring
> > > it)?
> > 
> > In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an
> array
> > length of one and a layout qualifier of "override_coverage" and then called
> > GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were
you
> > suggesting that the fragment builder itself should know about gl_SampleMask?
> > Maybe an enum of all the built-ins and the ability to redeclare one with
> layout
> > qualifiers?
> > 
> > And what about static const int sampleMask[]? Add some sort of mechanism for
> > defining const GrGLSLShaderVars at the top as well?
> > 
> > I see how this all could work, I'm just trying to understand the benefit of
> > abstracting how code is written to the top of the file when you can append
raw
> > strings into the function body? We also need to be careful not to make the
> > resulting C++ code harder to read and maintain than if it emitted raw
> strings...
> 
> Ok, so the fragment builder can add the override_coverage on gl_SampleMask
> inside enableFeature.
> 
> For defines we can add a simple helper.
> 
> What about "const char sampleOffsets[] = {...}"? I could just add that inline
> with codeAppendf, but I do like it better at the top of the file. We would
need
> to add support for the const keyword, and for the initializer list.

That's also something that we probably don't ever want to specify more than once
in a single shader. I could imagine it being used by the GP and a clipping FP at
in the same shader. Perhaps it should be handled by the builders?

[bsalomon, 2016-02-04 19:58:55]

On 2016/02/04 19:27:56, Chris Dalton wrote:
> On 2016/02/04 17:41:18, Chris Dalton wrote:
> > On 2016/02/04 16:59:43, bsalomon wrote:
> > > On 2016/02/04 16:48:16, Chris Dalton wrote:
> > > > For instanced shape rendering it's here:
> > > >
> > >
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > > > 
> > > >  - #define ...
> > > >  - static const float sampleLocations[] = {...};
> > > >  - layout(override_coverage) out int gl_SampleMask[1];
> > > > 
> > > > declAppendf was recently added to fragment shaders for PLS path
rendering
> to
> > > > declare its __pixel_localEXT struct:
> > > >
> > >
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > > > 
> > > > This change essentially just moves it from FragmentBuilder to
> ShaderBuilder,
> > > and
> > > > renames the existing declAppend function in ShaderBuilder in order to
> avoid
> > > > confusion.have arbitrary 
> > > 
> > > I was hesitant about that aspect of the PLS change and now feeling buyer's
> > > remorse. It seems like we have a way for individual processors to add
> > arbitrary
> > > top level code which breaks our abstraction. Can we instead add helpers
for
> > > adding macro definitions and use a different mechanism for gl_SampleMask
> > (maybe
> > > get the var from the builder and use the GrGLShaderVar mechanism for
> declaring
> > > it)?
> > 
> > In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an
> array
> > length of one and a layout qualifier of "override_coverage" and then called
> > GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were
you
> > suggesting that the fragment builder itself should know about gl_SampleMask?
> > Maybe an enum of all the built-ins and the ability to redeclare one with
> layout
> > qualifiers?
> > 
> > And what about static const int sampleMask[]? Add some sort of mechanism for
> > defining const GrGLSLShaderVars at the top as well?
> > 
> > I see how this all could work, I'm just trying to understand the benefit of
> > abstracting how code is written to the top of the file when you can append
raw
> > strings into the function body? We also need to be careful not to make the
> > resulting C++ code harder to read and maintain than if it emitted raw
> strings...
> 
> Ok, so the fragment builder can add the override_coverage on gl_SampleMask
> inside enableFeature.
> 
> For defines we can add a simple helper.
> 
> What about "const char sampleOffsets[] = {...}"? I could just add that inline
> with codeAppendf, but I do like it better at the top of the file. We would
need
> to add support for the const keyword, and for the initializer list.

That's also something that we probably don't ever want to specify more than once
in a single shader. I could imagine it being used by the GP and a clipping FP at
in the same shader. Perhaps it should be handled by the builders?

[Chris Dalton, 2016-02-04 20:13:35]

On 2016/02/04 19:58:55, bsalomon wrote:
> On 2016/02/04 19:27:56, Chris Dalton wrote:
> > On 2016/02/04 17:41:18, Chris Dalton wrote:
> > > On 2016/02/04 16:59:43, bsalomon wrote:
> > > > On 2016/02/04 16:48:16, Chris Dalton wrote:
> > > > > For instanced shape rendering it's here:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1653183004/diff/1/src/gpu/effects/GrShapeProc...
> > > > > 
> > > > >  - #define ...
> > > > >  - static const float sampleLocations[] = {...};
> > > > >  - layout(override_coverage) out int gl_SampleMask[1];
> > > > > 
> > > > > declAppendf was recently added to fragment shaders for PLS path
> rendering
> > to
> > > > > declare its __pixel_localEXT struct:
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/skia/+/5366a09ed07e886dd5fd1b94828241c53df3...
> > > > > 
> > > > > This change essentially just moves it from FragmentBuilder to
> > ShaderBuilder,
> > > > and
> > > > > renames the existing declAppend function in ShaderBuilder in order to
> > avoid
> > > > > confusion.have arbitrary 
> > > > 
> > > > I was hesitant about that aspect of the PLS change and now feeling
buyer's
> > > > remorse. It seems like we have a way for individual processors to add
> > > arbitrary
> > > > top level code which breaks our abstraction. Can we instead add helpers
> for
> > > > adding macro definitions and use a different mechanism for gl_SampleMask
> > > (maybe
> > > > get the var from the builder and use the GrGLShaderVar mechanism for
> > declaring
> > > > it)?
> > > 
> > > In the past I created a GrGLSLShaderVar named "gl_SampleMaskOut", with an
> > array
> > > length of one and a layout qualifier of "override_coverage" and then
called
> > > GrGLSLShaderBuilder::addOutput. But addOutput doesn't exist anymore. Were
> you
> > > suggesting that the fragment builder itself should know about
gl_SampleMask?
> > > Maybe an enum of all the built-ins and the ability to redeclare one with
> > layout
> > > qualifiers?
> > > 
> > > And what about static const int sampleMask[]? Add some sort of mechanism
for
> > > defining const GrGLSLShaderVars at the top as well?
> > > 
> > > I see how this all could work, I'm just trying to understand the benefit
of
> > > abstracting how code is written to the top of the file when you can append
> raw
> > > strings into the function body? We also need to be careful not to make the
> > > resulting C++ code harder to read and maintain than if it emitted raw
> > strings...
> > 
> > Ok, so the fragment builder can add the override_coverage on gl_SampleMask
> > inside enableFeature.
> > 
> > For defines we can add a simple helper.
> > 
> > What about "const char sampleOffsets[] = {...}"? I could just add that
inline
> > with codeAppendf, but I do like it better at the top of the file. We would
> need
> > to add support for the const keyword, and for the initializer list.
> 
> That's also something that we probably don't ever want to specify more than
once
> in a single shader. I could imagine it being used by the GP and a clipping FP
at
> in the same shader. Perhaps it should be handled by the builders?

Ok, that definitely makes sense thinking about from the perspective of an FP
rather than just a geometry processor. That's a good idea to handle sample
offsets from the builders, I'll work that out.

[Chris Dalton, 2016-02-05 03:27:57]

Description was changed from

==========
Add declAppendf for all glsl shaders

Adds a mechanism for builders to add specific declarations at the top
of the shader.

- Moves declAppendf from GrGLSLFragmentBuilder into GrGLSLShaderBuilder

- Renames the existing declAppend in GrGLSLShaderBuilder to
  codeAppendDecl

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add declAppendf for all glsl shaders

Adds a mechanism for builders to add specific declarations at the top
of the shader.

- Moves declAppendf from GrGLSLFragmentBuilder into GrGLSLShaderBuilder

- Renames the existing declAppend in GrGLSLShaderBuilder to
  codeAppendDecl

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-02-09 22:21:16]

Description was changed from

==========
Add declAppendf for all glsl shaders

Adds a mechanism for builders to add specific declarations at the top
of the shader.

- Moves declAppendf from GrGLSLFragmentBuilder into GrGLSLShaderBuilder

- Renames the existing declAppend in GrGLSLShaderBuilder to
  codeAppendDecl

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add #define directives to GrGLSLShaderBuilder

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-02-09 22:23:44]

Reworking this issue so it only allows builders to add #define directives

[bsalomon, 2016-02-11 21:32:26]

lgtm

[Chris Dalton, 2016-02-11 21:32:57]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-02-11 21:33:09]

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

[commit-bot: I haz the power, 2016-02-11 22:10:40]

Description was changed from

==========
Add #define directives to GrGLSLShaderBuilder

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add #define directives to GrGLSLShaderBuilder

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d36f2c4bc047c2c531d73eee08daa5926e8cdaf2
==========

[commit-bot: I haz the power, 2016-02-11 22:10:40]

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