1241042 - [WebGL2] pass getFragDataLocation in gl-object-get-calls.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

FAIL gl.getFragDataLocation(program, "fragColor") should be 0. Was -1.

Comment 1

[Ethan Lin[:ethlin]]

The shader was:

   #version 300 es
   in lowp vec4 position;
   layout(location = 0) out mediump vec4 fragColor;
   void main (void)
   {
          fragColor = position;
   }

After angle translation, we got:

   version 410
   in vec4 webgl_fa67bdd9ecec0ae;
   layout(location = 0) out vec4 webgl_e30b609a99317854;
   void main() {
     (webgl_e30b609a99317854 = webgl_fa67bdd9ecec0ae);
   }

And then we failed to get the fragColor.
   gl.getFragDataLocation(program, "fragColor")

I guess the problem may because the fragColor is translated to webgl_e30b609a99317854.
jrmuizel, do you have any idea?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

We are missing the code that maps to the translated names.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I believe we need to query and store this information using ShGetOutputVariables similar to ShGetVaryings, ShGetAttributes etc.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

I retrieve the mapped name from the validator to fix the problem.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

Change naming.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8711562 [details] [diff] [review]
Retrieve frag varying from validator.

Review of attachment 8711562 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +340,5 @@
> +    const std::vector<sh::OutputVariable>* fragOutput = prog->GetFragOutputVariables();
> +    if (fragOutput) {
> +        for (uint32_t i = 0; i < fragOutput->size(); i++) {
> +            info->fragDataMap.insert(std::make_pair(nsCString((*fragOutput)[i].name.c_str()),
> +                                                    nsCString((*fragOutput)[i].mappedName.c_str())));

Unless it's not possible, this code should look like all the blocks above, where we leave open the possibility that we don't use the shader validator.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

Fixed a minor problem and added comments.

Comment 8

[Ethan Lin[:ethlin]]

jgilbert,
There may be no way to enumerate fragment shader outputs[1]. I think I could check if the shader validator exists here[2] to do the workaround. If there is no shader validator, then that function will always return true with original baseUserName.

[1] https://www.khronos.org/bugzilla/show_bug.cgi?id=588
[2] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLProgram.h#118

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8711562 [details] [diff] [review]
Retrieve frag varying from validator.

Review of attachment 8711562 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +338,5 @@
> +    // Frag varying
> +
> +    const std::vector<sh::OutputVariable>* fragOutput = prog->GetFragOutputVariables();
> +    if (fragOutput) {
> +        for (uint32_t i = 0; i < fragOutput->size(); i++) {

You should be able to use a range based for loop here.

Comment 10

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

I storage the frag data from validator. If the validator is invalid, the data will be null pointer and we will use the base name directly.

Comment 11

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

Fix the indent problem of the patch.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8729401 [details] [diff] [review]
Retrieve frag varying from validator.

Review of attachment 8729401 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.h
@@ +67,5 @@
>      // user-facing `GLActiveInfo::name`s, without any final "[0]".
>      std::map<nsCString, const WebGLActiveInfo*> attribMap;
>      std::map<nsCString, const WebGLActiveInfo*> uniformMap;
>      std::map<nsCString, const WebGLActiveInfo*> transformFeedbackVaryingsMap;
> +    const std::vector<sh::OutputVariable>* fragOutput;

This will be unneeded.

@@ +113,5 @@
>          return false;
>      }
>  
>      bool FindFragData(const nsCString& baseUserName,
>                        nsCString* const out_baseMappedName) const

Remove FindFragData and replace it with FindActiveOutputMappedNameByUserName

::: dom/canvas/WebGLShaderValidator.h
@@ +43,5 @@
>      void GetOutput(nsACString* out) const;
>      bool CanLinkTo(const ShaderValidator* prev, nsCString* const out_log) const;
>      size_t CalcNumSamplerUniforms() const;
>      size_t NumAttributes() const;
> +    const std::vector<sh::OutputVariable>* GetActiveOutputVariables();

Instead of exporting this back to the WebGLProgram we should just add a function called FindActiveOutputMappedNameByUserName()

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator.

Addressed jeff's comments.

Comment 14

[Ethan Lin[:ethlin]]

Attachment: Retrieve frag varying from validator. (carry r+: jmuizelaar)

Modify the commit comments of the patch.

Comment 15

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca48fad55f22

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74282731c3d1

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74282731c3d1

Comment 18

[Kelsey Gilbert [:jgilbert]]

This caused bug 1259702 and should be reverted.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8730117 [details] [diff] [review]
Retrieve frag varying from validator. (carry r+: jmuizelaar)

Review of attachment 8730117 [details] [diff] [review]:
-----------------------------------------------------------------

Marking this r- for clarity.

::: dom/canvas/WebGLProgram.cpp
@@ +567,5 @@
>  
>      const NS_LossyConvertUTF16toASCII userName(userName_wide);
>  
>      nsCString mappedName;
> +    if (!FindActiveOutputMappedNameByUserName(userName, &mappedName)) {

Specifically, we must attach this information to the ProgramInfo object, as mVertShader and mFragShader can be freely detached at any time.

Comment 20

[Kelsey Gilbert [:jgilbert]]

I'm going to take this to fix it.

Comment 21

[Kelsey Gilbert [:jgilbert]]

bump

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: save frag translated varying names into LinkedProgramInfo. v1

Save the frag name info to the LinkedProgramInfo object. Then the fragment shader can be freely detached at any time.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: remove the original implementation. v1

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I'm trying to close the crash bug 1259702.

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Pass the "gl.getFragDataLocation(program, "fragColor")" test in
https://www.khronos.org/registry/webgl/sdk/tests/conformance2/state/gl-object-get-calls.html?webglVersion=2&quiet=0

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df65cb3ba95

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8767587 [details] [diff] [review]
P1: save frag translated varying names into LinkedProgramInfo. v1

Review of attachment 8767587 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +1188,5 @@
>      return false;
>  }
>  
> +void
> +WebGLProgram::EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings)

s/FragVaryings/FragData/ here and elsewhere.

FragData are outputs from the frag shader, while varyings are outputs from the vert shader and inputs to the frag shader.

::: dom/canvas/WebGLProgram.h
@@ +117,5 @@
> +    bool
> +    FindFragData(const nsCString& baseUserName,
> +                 nsCString* const out_baseMappedName) const
> +    {
> +        auto itr = fragDataMap.find(baseUserName);

const auto itr

@@ +120,5 @@
> +    {
> +        auto itr = fragDataMap.find(baseUserName);
> +        if (itr == fragDataMap.end()) {
> +            return false;
> +        }

No need for {} around returns like this. (single-line returns after single-line conditionals)

@@ +195,5 @@
>      void TransformFeedbackVaryings(const dom::Sequence<nsString>& varyings,
>                                     GLenum bufferMode);
>      already_AddRefed<WebGLActiveInfo> GetTransformFeedbackVarying(GLuint index);
>  
> +    void EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings);

This function can be const.

::: dom/canvas/WebGLShader.cpp
@@ +418,5 @@
> +     out_FragVaryings.clear();
> +
> +     if (!mValidator) {
> +         return;
> +     }

No need for {} around returns like this.

::: dom/canvas/WebGLShaderValidator.cpp
@@ +540,5 @@
>  
> +void
> +ShaderValidator::EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings) const
> +{
> +    const std::vector<sh::OutputVariable>* fragOutput = ShGetOutputVariables(mHandle);

const auto& fragOutputs

@@ +543,5 @@
> +{
> +    const std::vector<sh::OutputVariable>* fragOutput = ShGetOutputVariables(mHandle);
> +
> +    if (fragOutput) {
> +          for (uint32_t i = 0, num = fragOutput->size(); i < num; ++i) {

for (const auto& fragOutput : *fragOutputs) {
}

@@ +545,5 @@
> +
> +    if (fragOutput) {
> +          for (uint32_t i = 0, num = fragOutput->size(); i < num; ++i) {
> +              out_FragVaryings.insert(std::make_pair(nsCString((*fragOutput)[i].name.c_str()),
> +                                                     nsCString((*fragOutput)[i].mappedName.c_str())));

You might be able to use {} instead of make_pair.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8767587 [details] [diff] [review]
P1: save frag translated varying names into LinkedProgramInfo. v1

Review of attachment 8767587 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +1188,5 @@
>      return false;
>  }
>  
> +void
> +WebGLProgram::EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings)

FragOutputs is better, sorry!

Comment 29

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8767587 [details] [diff] [review]
P1: save frag translated varying names into LinkedProgramInfo. v1

Review of attachment 8767587 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +1188,5 @@
>      return false;
>  }
>  
> +void
> +WebGLProgram::EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings)

checked

::: dom/canvas/WebGLProgram.h
@@ +117,5 @@
> +    bool
> +    FindFragData(const nsCString& baseUserName,
> +                 nsCString* const out_baseMappedName) const
> +    {
> +        auto itr = fragDataMap.find(baseUserName);

checked

@@ +120,5 @@
> +    {
> +        auto itr = fragDataMap.find(baseUserName);
> +        if (itr == fragDataMap.end()) {
> +            return false;
> +        }

According to mozilla c++ coding style, single-line conditional still needs {}.

@@ +195,5 @@
>      void TransformFeedbackVaryings(const dom::Sequence<nsString>& varyings,
>                                     GLenum bufferMode);
>      already_AddRefed<WebGLActiveInfo> GetTransformFeedbackVarying(GLuint index);
>  
> +    void EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings);

checked

::: dom/canvas/WebGLShaderValidator.cpp
@@ +540,5 @@
>  
> +void
> +ShaderValidator::EnumerateFragVaryings(std::map<nsCString, const nsCString> &out_FragVaryings) const
> +{
> +    const std::vector<sh::OutputVariable>* fragOutput = ShGetOutputVariables(mHandle);

checked

@@ +543,5 @@
> +{
> +    const std::vector<sh::OutputVariable>* fragOutput = ShGetOutputVariables(mHandle);
> +
> +    if (fragOutput) {
> +          for (uint32_t i = 0, num = fragOutput->size(); i < num; ++i) {

checked

@@ +545,5 @@
> +
> +    if (fragOutput) {
> +          for (uint32_t i = 0, num = fragOutput->size(); i < num; ++i) {
> +              out_FragVaryings.insert(std::make_pair(nsCString((*fragOutput)[i].name.c_str()),
> +                                                     nsCString((*fragOutput)[i].mappedName.c_str())));

checked

Comment 30

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: save frag translated varying names into LinkedProgramInfo. v2. r=jgilbert

Save the frag name info to the LinkedProgramInfo object. Then the fragment shader can be freely detached at any time.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5270726357
save frag translated varying names into LinkedProgramInfo. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ceca97f85de
remove the original implementation. r=jgilbert

Comment 32

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] (UTC+8) (PTO:7/11-7/20) from comment #29)
> 
> @@ +120,5 @@
> > +    {
> > +        auto itr = fragDataMap.find(baseUserName);
> > +        if (itr == fragDataMap.end()) {
> > +            return false;
> > +        }
> 
> According to mozilla c++ coding style, single-line conditional still needs
> {}.

Not in this module. :)

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #32)
> (In reply to Jerry Shih[:jerry] (UTC+8) (PTO:7/11-7/20) from comment #29)
> > According to mozilla c++ coding style, single-line conditional still needs
> > {}.
> 
> Not in this module. :)

got it, thx.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad5270726357
https://hg.mozilla.org/mozilla-central/rev/5ceca97f85de