Closed
Bug 990851
Opened 10 years ago
Closed 10 years ago
TexSubImage calls fail on cubemap targets.
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: u480271, Assigned: u480271)
Details
Attachments
(1 file, 1 obsolete file)
1.50 KB,
patch
|
jgilbert
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
TexSubImage on cubemap faces is incorrectly insisting that width != height.
Fix bug discovered by https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/texture-sub-image-cube-maps.html
Attachment #8400356 -
Flags: review?(jgilbert)
Comment 2•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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 7•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e351fbac5789
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 11•10 years ago
|
||
Beta is affected. Is this the correct way to ask if this patch should be uplifted to Beta?
status-firefox30:
--- → affected
tracking-firefox30:
--- → ?
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox31:
--- → fixed
Updated•10 years ago
|
Attachment #8407243 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Comment 16•10 years ago
|
||
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.
Description
•