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?

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.

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

Change naming.

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.

Fixed a minor problem and added comments.

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 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.

Fix the indent problem of the patch.

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()

Addressed jeff's comments.

Modify the commit comments of the patch.

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

This caused bug 1259702 and should be reverted.

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.

I'm going to take this to fix it.

bump

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

I'm trying to close the crash bug 1259702.

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

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

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 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 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.

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