Closed Bug 1241042 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
firefox50 --- fixed

People

(Reporter: ethlin, Assigned: jerry)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 7 obsolete files)

FAIL gl.getFragDataLocation(program, "fragColor") should be 0. Was -1.
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?
Flags: needinfo?(jmuizelaar)
We are missing the code that maps to the translated names.
I believe we need to query and store this information using ShGetOutputVariables similar to ShGetVaryings, ShGetAttributes etc.
Flags: needinfo?(jmuizelaar)
Whiteboard: [gfx-noted]
I retrieve the mapped name from the validator to fix the problem.
Attachment #8711561 - Flags: review?(jgilbert)
Change naming.
Attachment #8711561 - Attachment is obsolete: true
Attachment #8711561 - Flags: review?(jgilbert)
Attachment #8711562 - Flags: review?(jgilbert)
Assignee: nobody → ethlin
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.
Attachment #8711562 - Flags: review?(jgilbert) → review-
Fixed a minor problem and added comments.
Attachment #8711562 - Attachment is obsolete: true
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
Flags: needinfo?(jgilbert)
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.
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.
Attachment #8712032 - Attachment is obsolete: true
Attachment #8729385 - Flags: review?(jgilbert)
Attachment #8729385 - Flags: review?(jgilbert)
Fix the indent problem of the patch.
Attachment #8729385 - Attachment is obsolete: true
Flags: needinfo?(jgilbert)
Attachment #8729401 - Flags: review?(jgilbert)
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()
Attachment #8729401 - Flags: review?(jgilbert) → review-
Addressed jeff's comments.
Attachment #8729401 - Attachment is obsolete: true
Attachment #8729459 - Flags: review?(jmuizelaar)
Attachment #8729459 - Flags: review?(jmuizelaar) → review+
Modify the commit comments of the patch.
Attachment #8729459 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74282731c3d1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This caused bug 1259702 and should be reverted.
Blocks: 1259702
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8730117 - Flags: review-
I'm going to take this to fix it.
Assignee: ethlin → jgilbert
Save the frag name info to the LinkedProgramInfo object. Then the fragment shader can be freely detached at any time.
Attachment #8767587 - Flags: review?(jgilbert)
I'm trying to close the crash bug 1259702.
Assignee: jgilbert → hshih
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.
Attachment #8767587 - Flags: review?(jgilbert) → review+
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!
Attachment #8767588 - Flags: review?(jgilbert) → review+
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
Save the frag name info to the LinkedProgramInfo object. Then the fragment shader can be freely detached at any time.
Attachment #8771946 - Flags: review+
Attachment #8767587 - Attachment is obsolete: true
Pushed by hshih@mozilla.com:
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
(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. :)
(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.
https://hg.mozilla.org/mozilla-central/rev/ad5270726357
https://hg.mozilla.org/mozilla-central/rev/5ceca97f85de
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.