We plain get this wrong. In particular, the TRANSFORM_FEEDBACK_BUFFER_BINDING should be attached to the TransformFeedback object. (though Win+NV's driver gets this wrong too. Easy enough to work around)

Comment on attachment 8792650 [details] Bug 1300946 - Binding a deleted TFO should be INVALID_OP. - https://reviewboard.mozilla.org/r/79562/#review78280 The 'ValidateObjectAllowNull' always set 'InvalidValue' if the object is deleted, so we have to do additional checking here for returning 'InvalidOperation'.

(In reply to Ethan Lin[:ethlin] from comment #10) > Comment on attachment 8792650 [details] > Bug 1300946 - Binding a deleted TFO should be INVALID_OP. - > > https://reviewboard.mozilla.org/r/79562/#review78280 > > The 'ValidateObjectAllowNull' always set 'InvalidValue' if the object is > deleted, so we have to do additional checking here for returning > 'InvalidOperation'. Yep, it sucks. It's not a perf hit though. (this command isn't in any hot loops)

Comment on attachment 8792651 [details] Bug 1300946 - Only clear TFO indexed bindings on delete if TFO is inactive. - https://reviewboard.mozilla.org/r/79564/#review78304

Comment on attachment 8792648 [details] Bug 1300946 - Implement transform feedback. - https://reviewboard.mozilla.org/r/79558/#review78238 ::: dom/canvas/WebGL2Context.cpp:218 (Diff revision 2) > + > + //// > + > + gl->fBindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, 0); > + gl->fBindBuffer(LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER, 0); > + */ This can go ::: dom/canvas/WebGL2ContextBuffers.cpp:147 (Diff revision 2) > - * the WebGL API to ensure that data are access > - * consistently. This applies even if the buffer is currently > - * bound to a transform feedback binding point. > - */ > > - void* ptr = gl->fMapBufferRange(target, offset, data.LengthAllowShared(), > + const auto ptr = gl->fMapBufferRange(target, offset, data.LengthAllowShared(), This hides the type of ptr which doesn't seem to have much value. ::: dom/canvas/WebGLContextBuffers.cpp:68 (Diff revision 2) > +} > + > +WebGLBuffer* > +WebGLContext::ValidateBufferSelection(const char* funcName, GLenum target) > +{ > + const auto& slot = ValidateBufferSlot(funcName, target); Why do you want a reference to a pointer? ::: dom/canvas/WebGLContextBuffers.cpp:71 (Diff revision 2) > +WebGLContext::ValidateBufferSelection(const char* funcName, GLenum target) > +{ > + const auto& slot = ValidateBufferSlot(funcName, target); > + if (!slot) > + return nullptr; > + const auto& buffer = *slot; Same here. Also, this function and others like it would be easier to read using explicit types instead of 'auto'. Having to lookup the return type of ValidateBufferSlot to understand what's going on isn't so nice. ::: dom/canvas/WebGLContextBuffers.cpp:518 (Diff revision 2) > const dom::ArrayBufferView& data) > { > BufferSubDataT(target, byteOffset, data); > } > > +//////////////////////////////////////// What is the purpose of these breaks? How do you decide when to put them in? ::: dom/canvas/WebGLContextDraw.cpp (Diff revision 2) > + return; > > { > ScopedMaskWorkaround autoMask(*this); > - > + gl->fDrawElements(mode, vertCount, type, > - if (gl->IsSupported(gl::GLFeature::draw_range_elements)) { Why is DrawRangeElements being dropped? ::: dom/canvas/WebGLProgram.h:92 (Diff revision 2) > > WebGLProgram* const prog; > > std::vector<AttribInfo> attribs; > std::vector<UniformInfo*> uniforms; // Owns its contents. > - std::vector<const UniformBlockInfo*> uniformBlocks; // Owns its contents. > + std::vector<UniformBlockInfo*> uniformBlocks; // Owns its contents. Can you change these to UniquePtrs in a separate patch? ::: dom/canvas/WebGLProgram.h:99 (Diff revision 2) > > // Needed for draw call validation. > std::vector<UniformInfo*> uniformSamplers; > > + mutable std::vector<size_t> componentsPerTFVert; > + mutable std::set<WebGLTransformFeedback*> activeTFOs; is this unused? ::: dom/canvas/WebGLProgram.h:100 (Diff revision 2) > // Needed for draw call validation. > std::vector<UniformInfo*> uniformSamplers; > > + mutable std::vector<size_t> componentsPerTFVert; > + mutable std::set<WebGLTransformFeedback*> activeTFOs; > + mutable std::vector<IndexedBufferBinding> uniformBlockBindings; Is this used?

Comment on attachment 8792649 [details] Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. - https://reviewboard.mozilla.org/r/79560/#review78610 ::: dom/canvas/WebGLBuffer.cpp:194 (Diff revision 2) > { > return mCache->BeenUsedWithMultipleTypes(); > } > > +bool > +WebGLBuffer::ValidateCanBindToTarget(const char* funcName, GLenum target) Did anything change in this function? The diff doesn't say.

Comment on attachment 8792647 [details] Bug 1300946 - GetCurrentBinding is the wrong approach. - https://reviewboard.mozilla.org/r/79556/#review78612

Comment on attachment 8792649 [details] Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. - https://reviewboard.mozilla.org/r/79560/#review78610 > Did anything change in this function? The diff doesn't say. The mBoundForTF bit is new.

Comment on attachment 8792648 [details] Bug 1300946 - Implement transform feedback. - https://reviewboard.mozilla.org/r/79558/#review78238 > Why do you want a reference to a pointer? It decays into a value if given a value. `const auto&` is roughly "give this value a name, and don't make a copy if you con't need to". > Same here. Also, this function and others like it would be easier to read using explicit types instead of 'auto'. Having to lookup the return type of ValidateBufferSlot to understand what's going on isn't so nice. I guess? I generally have to look up the function I'm using anyways, to make sure I know what it's doing. This generally tells me what I'm being handed 'for free'. > What is the purpose of these breaks? How do you decide when to put them in? It's just a form of pseudo-whitespace separation. It's like paragraph breaks in prose. > Why is DrawRangeElements being dropped? It has no future, since we'll be moving to drawElements+robust_buffer_access_behavior soon. Might as well drop the complexity now. > Can you change these to UniquePtrs in a separate patch? This is now bug 1304507. > is this unused? Yes. > Is this used? No.

Comment on attachment 8792648 [details] Bug 1300946 - Implement transform feedback. - https://reviewboard.mozilla.org/r/79558/#review79196

Comment on attachment 8792649 [details] Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. - https://reviewboard.mozilla.org/r/79560/#review78668

Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a01c95a3bb4e GetCurrentBinding is the wrong approach. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6514210303 Implement transform feedback. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3ec914ce4d Forbid simultaneous binding to TF and non-TF bind points. - r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/3c256ba175d7 Binding a deleted TFO should be INVALID_OP. - r=ethlin https://hg.mozilla.org/integration/mozilla-inbound/rev/84f04631a9a2 Only clear TFO indexed bindings on delete if TFO is inactive. - r=ethlin https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9844b0dee3 TF test passes on Windows.

https://hg.mozilla.org/mozilla-central/rev/a01c95a3bb4e https://hg.mozilla.org/mozilla-central/rev/7a6514210303 https://hg.mozilla.org/mozilla-central/rev/ff3ec914ce4d https://hg.mozilla.org/mozilla-central/rev/3c256ba175d7 https://hg.mozilla.org/mozilla-central/rev/84f04631a9a2 https://hg.mozilla.org/mozilla-central/rev/fa9844b0dee3

https://hg.mozilla.org/mozilla-central/rev/a01c95a3bb4e https://hg.mozilla.org/mozilla-central/rev/7a6514210303 https://hg.mozilla.org/mozilla-central/rev/ff3ec914ce4d https://hg.mozilla.org/mozilla-central/rev/3c256ba175d7 https://hg.mozilla.org/mozilla-central/rev/84f04631a9a2 https://hg.mozilla.org/mozilla-central/rev/fa9844b0dee3

Comment on attachment 8792647 [details] Bug 1300946 - GetCurrentBinding is the wrong approach. - Approval Request Comment [Feature/regressing bug #]: webgl2 [User impact if declined]: bad webgl2 [Describe test coverage new/current, TreeHerder]: conformance suite [Risks and why]: low. This is pretty well understood. [String/UUID change made/needed]: none This applies to all patches in this bug.

Comment on attachment 8792647 [details] Bug 1300946 - GetCurrentBinding is the wrong approach. - WebGL2 is planned for Beta50, let's stabilize this on Aurora51 and if there are no problems I plan to include this in 50.0b4 on 10/03.

I had issues uplifting these patches. Could we get a rebased version?

https://hg.mozilla.org/releases/mozilla-aurora/rev/37e1580285ce https://hg.mozilla.org/releases/mozilla-aurora/rev/92905db1d7e3 https://hg.mozilla.org/releases/mozilla-aurora/rev/3c7bf5fec116 https://hg.mozilla.org/releases/mozilla-aurora/rev/10d4c1119501 https://hg.mozilla.org/releases/mozilla-aurora/rev/f2ec118dca15 https://hg.mozilla.org/releases/mozilla-aurora/rev/1a2156ed8774

Oh, guess I tried in the wrong order, doing this last worked.

Comment on attachment 8792647 [details] Bug 1300946 - GetCurrentBinding is the wrong approach. - WebGL2 is planned for Fx50, Beta+ for all patches.

Jeff landed this. Had to be backed out for bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=1706115&repo=mozilla-beta https://hg.mozilla.org/releases/mozilla-beta/rev/ca2593daaa91f717e3ae65f049308309a31969a2