Closed Bug 990851 Opened 6 years ago Closed 6 years ago

TexSubImage calls fail on cubemap targets.

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox30 - verified
firefox31 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

Details

Attachments

(1 file, 1 obsolete file)

TexSubImage on cubemap faces is incorrectly insisting that width != height.
Comment on attachment 8400356 [details] [diff] [review]
Fix cubemap + TexSubImage dimension check

Review of attachment 8400356 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +941,5 @@
>      MOZ_ASSERT(level >= 0, "level should already be validated");
>  
>      const GLuint maxTexImageSize = MaxTextureSizeForTarget(target) >> level;
>      const bool isCubemapTarget = IsTexImageCubemapTarget(target);
> +    const bool isTex = IsTexFunc(func);

When is this false?
(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8400356 [details] [diff] [review]
> Fix cubemap + TexSubImage dimension check
> 
> Review of attachment 8400356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +941,5 @@
> >      MOZ_ASSERT(level >= 0, "level should already be validated");
> >  
> >      const GLuint maxTexImageSize = MaxTextureSizeForTarget(target) >> level;
> >      const bool isCubemapTarget = IsTexImageCubemapTarget(target);
> > +    const bool isTex = IsTexFunc(func);
> 
> When is this false?

When func is a TexSubImage variant. It's effectively !IsSubFunc. I had that first but felt bad about the inverted sense of the bool.
(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > Comment on attachment 8400356 [details] [diff] [review]
> > Fix cubemap + TexSubImage dimension check
> > 
> > Review of attachment 8400356 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/canvas/src/WebGLContextValidate.cpp
> > @@ +941,5 @@
> > >      MOZ_ASSERT(level >= 0, "level should already be validated");
> > >  
> > >      const GLuint maxTexImageSize = MaxTextureSizeForTarget(target) >> level;
> > >      const bool isCubemapTarget = IsTexImageCubemapTarget(target);
> > > +    const bool isTex = IsTexFunc(func);
> > 
> > When is this false?
> 
> When func is a TexSubImage variant. It's effectively !IsSubFunc. I had that
> first but felt bad about the inverted sense of the bool.

Oh. I think it's most clear if we have a IsTexSubFunc, and deal with things like `!isTexSub`.
Comment on attachment 8400356 [details] [diff] [review]
Fix cubemap + TexSubImage dimension check

Review of attachment 8400356 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +941,5 @@
>      MOZ_ASSERT(level >= 0, "level should already be validated");
>  
>      const GLuint maxTexImageSize = MaxTextureSizeForTarget(target) >> level;
>      const bool isCubemapTarget = IsTexImageCubemapTarget(target);
> +    const bool isTex = IsTexFunc(func);

Let's just use !IsSubFunc.
Attachment #8400356 - Flags: review?(jgilbert) → review-
Address Jeff's comments on first patch.
Attachment #8407243 - Flags: review?(jgilbert)
Attachment #8400356 - Attachment is obsolete: true
Comment on attachment 8407243 [details] [diff] [review]
Fix TexSubImage with cubemap face failure

Review of attachment 8407243 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!
Attachment #8407243 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/e351fbac5789
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Beta is affected. Is this the correct way to ask if this patch should be uplifted to Beta?
(In reply to Dan Glastonbury :djg :kamidphish from comment #11)
> Beta is affected. Is this the correct way to ask if this patch should be
> uplifted to Beta?

Also just ask for beta approval in the patch.
Comment on attachment 8407243 [details] [diff] [review]
Fix TexSubImage with cubemap face failure

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 966624
User impact if declined: Unable to update
Testing completed (on m-c, etc.): aurora, m-c
Risk to taking this patch (and alternatives if risky): Low risk. Landed on m-c back in April. Tested against Khronos 1.0.3b conformance suite.
String or IDL/UUID changes made by this patch:
Attachment #8407243 - Flags: approval-mozilla-beta?
Attachment #8407243 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Loaded the testcase from comment 1 on Nightly from 2014-04-01, and I could reproduce the issue, tests failed. Verified that the tests passed using Firefox 30 beta 9 and latest Aurora on Windows 8.1 64bit, Ubuntu 13.04 64bit and Mac OS X 10.8.5
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.