Comment on attachment 8695204 [details] [diff] [review] If sampler is bound, use parameter of sampler. Review of attachment 8695204 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLSampler.cpp @@ +48,5 @@ > return dom::WebGLSamplerBinding::Wrap(cx, this, givenProto); > } > > +void > +WebGLSampler::SamplerParameter(GLenum pname, const WebGLIntOrFloat& param) Since this is the only way we use it, why not just SamplerParameter1i(GLenum, GLint)? ::: dom/canvas/WebGLSampler.h @@ +37,5 @@ > > NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(WebGLSampler) > NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(WebGLSampler) > > + TexMinFilter mMinFilter; These should match the order of GLES 3.0.4 p253. ::: dom/canvas/WebGLTexture.cpp @@ +540,5 @@ > bool > WebGLTexture::ResolveForDraw(const char* funcName, uint32_t texUnit, > FakeBlackType* const out_fakeBlack) > { > + if (!mIsResolved || mContext->mBoundSamplers[texUnit]) { I don't think this is sufficient, since it doesn't handle clearing our cache when a sampler object is unbound/removed.

Clear cache when sampler is bind/unbind/removed.

Comment on attachment 8705027 [details] [diff] [review] If sampler is bound, use parameter of sampler v3. Review of attachment 8705027 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLSampler.cpp @@ +51,5 @@ > +void > +WebGLSampler::SamplerParameter1i(GLenum pname, GLint param) > +{ > + switch (pname) { > + case LOCAL_GL_TEXTURE_MIN_FILTER: These cases all need to invalidate the cache. ::: dom/canvas/WebGLTexture.cpp @@ +542,5 @@ > bool > WebGLTexture::ResolveForDraw(const char* funcName, uint32_t texUnit, > FakeBlackType* const out_fakeBlack) > { > + if (!mIsResolved || mContext->mBoundSamplers[texUnit]) { Remove the check against mBoundSamplers here, since mIsResolved should be cleared by invalidation.

Fix jgilbert's comment.

Fix compile error.

Comment on attachment 8705469 [details] [diff] [review] If sampler is bound, use parameter of sampler v5. Review of attachment 8705469 [details] [diff] [review]: ----------------------------------------------------------------- Alright, almost there. We just need to add support for all the rest of the sampler state. ::: dom/canvas/WebGLSampler.cpp @@ +66,5 @@ > + > + case LOCAL_GL_TEXTURE_WRAP_T: > + mWrapT = param; > + break; > + } Add: default: MOZ_CRASH("Unhandled pname");

To be clear, this patch is r+, but it shouldn't really be landed without a follow-up patch to add support for the rest of the sampler state. Given that, I'm going to mark this r- unless there's a reason to take this with partial support.

Add rest of sampler state support.

Comment on attachment 8707322 [details] [diff] [review] If sampler is bound, use parameter of sampler v6. Review of attachment 8707322 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks! ::: dom/canvas/WebGLSampler.cpp @@ +113,5 @@ > + break; > + } > + > + for (int i = 0; i < mContext->mGLMaxTextureUnits; ++i) { > + if (this == mContext->mBoundSamplers[i]) Ideally iterate over mBoundSamples.Length(), instead of relying on the implicit link between it and mGLMaxTextureUnits.

Fix build error and try again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e801c34593

Fix again https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecffcb5ec2fe

This doesn't pass for me on OS X.