1230089 - Pass WEBLG2 sampler-drawing-test test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler.

Comment 2

[Kelsey Gilbert [:jgilbert]]

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.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler v2.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler v3.

Clear cache when sampler is bind/unbind/removed.

Comment 5

[Kelsey Gilbert [:jgilbert]]

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.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler v4.

Fix jgilbert's comment.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler v5.

Fix compile error.

Comment 8

[Kelsey Gilbert [:jgilbert]]

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");

Comment 9

[Kelsey Gilbert [:jgilbert]]

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.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: If sampler is bound, use parameter of sampler v6.

Add rest of sampler state support.

Comment 11

[Kelsey Gilbert [:jgilbert]]

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.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Fix again

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56626252ef75

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/56626252ef75

Comment 17

[Jeff Muizelaar [:jrmuizel]]

This doesn't pass for me on OS X.