Closed Bug 1230089 Opened 4 years ago Closed 4 years ago

Pass WEBLG2 sampler-drawing-test test

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

()

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 5 obsolete files)

No description provided.
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.
Attachment #8695204 - Flags: review?(jgilbert) → review-
Attachment #8695204 - Attachment is obsolete: true
Clear cache when sampler is bind/unbind/removed.
Attachment #8705027 - Flags: review?(jgilbert)
Attachment #8705023 - Attachment is obsolete: true
Attachment #8705023 - Flags: review?(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.
Attachment #8705027 - Flags: review?(jgilbert) → review-
Fix jgilbert's comment.
Attachment #8705464 - Flags: review?(jgilbert)
Attachment #8705027 - Attachment is obsolete: true
Fix compile error.
Attachment #8705469 - Flags: review?(jgilbert)
Attachment #8705464 - Attachment is obsolete: true
Attachment #8705464 - Flags: review?(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");
Attachment #8705469 - Flags: review?(jgilbert) → review+
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.
Attachment #8705469 - Flags: review+ → review-
Add rest of sampler state support.
Attachment #8707322 - Flags: review?(jgilbert)
Attachment #8705469 - Attachment is obsolete: true
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.
Attachment #8707322 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/56626252ef75
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
This doesn't pass for me on OS X.
You need to log in before you can comment on or make changes to this bug.