Closed
Bug 1081125
Opened 10 years ago
Closed 10 years ago
WebGL2: basic support for 3D textures
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(6 files, 4 obsolete files)
12.51 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
64.56 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
43.41 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
4.06 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
5.89 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
121.17 KB,
patch
|
Details | Diff | Splinter Review |
By basic support I mean: 1) Hardcoding TEX_IMAGE_HEIGHT to 0, its default value, i.e. no support for changing it yet with pixelStorei. In other words: assume tight packing between (X,Y) planes. 2) No support for any format conversion, stride adjustment, alpha premultiplication, or flipping. Not sure if any of that will be wanted for 3D textures, sounds like the WG is still discussing this kind of thing, and that would be very expensive to implement (many man-hours), and certainly not very useful for early demos. 3) Only support uploading from TypedArray / ArrayBufferView, not from DOM elements or ImageData. Will that even be allowed once WebGL2 is finalized?
Assignee | ||
Comment 1•10 years ago
|
||
Note: Dan had a WIP patch for 3D textures on his branch. This is largely inspired by it.
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Not really needed for this 3D textures work, but this was hurting my eyes.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
I'll write a minimal conformance test before I ask for review.
Assignee | ||
Updated•10 years ago
|
Attachment #8503161 -
Attachment is patch: true
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bjacob
Assignee | ||
Comment 6•10 years ago
|
||
See above comment. Not really related to this bug, was just bugging me.
Attachment #8503154 -
Attachment is obsolete: true
Attachment #8503157 -
Attachment is obsolete: true
Attachment #8503160 -
Attachment is obsolete: true
Attachment #8503161 -
Attachment is obsolete: true
Attachment #8503880 -
Flags: review?(jgilbert)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8503881 -
Flags: review?(jgilbert)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8503884 -
Flags: review?(jgilbert)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8503886 -
Flags: review?(jgilbert)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8503887 -
Flags: review?(jgilbert)
Assignee | ||
Comment 11•10 years ago
|
||
@Jeff - combined patch to help reviewing. @David - this should be ready to expose to emscripten. I've written conformance tests for 3d textures, and this passes them, so this should be working decently. https://github.com/KhronosGroup/WebGL/pull/748 The catch is that I haven't checked any actual rendering - but as far as texSubImage3D and texStorage3D entry points are concerned, this should be it. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7047fb305bf0
Assignee | ||
Comment 12•10 years ago
|
||
The failures on try were due to not rejecting TEXTURE_3D in bindTexture in WebGL 1 mode. Simple 2-line fix; not worth disturbing the review process, and not needed for David's emscriptening work which can focus on WebGL 2 mode; new try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=caf830ec6768
Comment 13•10 years ago
|
||
Comment on attachment 8503880 [details] [diff] [review] Part 0: stop using plain ints for jsArrayTypes Review of attachment 8503880 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContextGL.cpp @@ +3646,5 @@ > GLint border, > GLenum format, > GLenum type, > void* data, uint32_t byteLength, > + js::Scalar::Type jsArrayType, Replace the old comment with a new one about our special meaning for TypeMax.
Attachment #8503880 -
Flags: review?(jgilbert) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8503881 [details] [diff] [review] Part 1: add WebGLTexDimensions enum instead of using plain ints to distinguish between 2D and 3D calls Review of attachment 8503881 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContext.h @@ -482,5 @@ > { > if (IsContextLost()) > return; > > - auto dims = 2; Haha, best use of auto ever. ::: dom/canvas/WebGLContextValidate.cpp @@ +387,5 @@ > { > bool valid = IsWebGL2(); > if (!valid) { > ErrorInvalidEnum("%s: invalid format %s: requires WebGL version 2.0 or newer", > + InfoFrom(func, dims), EnumName(format)); I like this InfoFrom thing. ::: dom/canvas/WebGLTypes.h @@ +139,5 @@ > CompTexSubImage, > MOZ_END_ENUM_CLASS(WebGLTexImageFunc) > > +MOZ_BEGIN_ENUM_CLASS(WebGLTexDimensions, uint8_t) > + Tex2D, Yay!
Attachment #8503881 -
Flags: review?(jgilbert) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8503884 [details] [diff] [review] Part 2: update our texture state tracking to handle 3D textures Review of attachment 8503884 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextTextures.cpp @@ +158,5 @@ > internalformat, > WebGLImageDataStatus::UninitializedImageData); > } > + w = std::max(1, w >> 1); > + h = std::max(1, h >> 1); Stick with division. The compiler should pick up this optimization anyways. ::: dom/canvas/WebGLContextValidate.cpp @@ +822,5 @@ > > // TODO: WebGL 2 > if (texImageTarget == LOCAL_GL_TEXTURE_3D) { > if (depth < 0) { > ErrorInvalidValue("%s: depth must be >= 0", InfoFrom(func, dims)); Doesn't depth need to be >0, not >=0? A 2D texture technically has a depth of 1, implicitly. Does the API say differently? ::: dom/canvas/WebGLTexture.cpp @@ +93,5 @@ > > // checks if custom level>0 images are all defined up to the highest level defined > // and have the expected dimensions > + for (size_t level = 0; level <= mMaxLevelWithCustomImages; ++level) > + { This is all one line, so { stays at the end. @@ +199,5 @@ > EnsureMaxLevelWithCustomImagesAtLeast(maxLevel); > > for (size_t level = 1; level <= maxLevel; ++level) { > // again, since the sizes are powers of two, no need for any max(1,x) computation > + imageInfo.mWidth = std::max(imageInfo.mWidth >> 1, 1); This is going to conflict with :dvander's NPOT stuff, fwiw. ::: dom/canvas/WebGLTexture.h @@ +127,5 @@ > + * Note that mWidth and mHeight are inherited from WebGLRectangleObject. > + * It's a pity to store a useless mDepth on non-3D texture images, but > + * the size of GLsizei is negligible compared to the typical size of a texture image. > + */ > + GLsizei mDepth; I don't love how we have mWidth/mHeight from WebGLRectangleObject, but mDepth here.
Attachment #8503884 -
Flags: review?(jgilbert) → review+
Updated•10 years ago
|
Attachment #8503886 -
Flags: review?(jgilbert) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8503887 [details] [diff] [review] Part 4: implement texSubImage3d Review of attachment 8503887 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextTextures.cpp @@ +217,5 @@ > + return; > + > + TexImageTarget texImageTarget(rawTarget); > + > + WebGLTexture *tex = activeBoundTextureForTexImageTarget(texImageTarget); Star against type.
Attachment #8503887 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #15) > Comment on attachment 8503884 [details] [diff] [review] > Part 2: update our texture state tracking to handle 3D textures > > Review of attachment 8503884 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGL2ContextTextures.cpp > @@ +158,5 @@ > > internalformat, > > WebGLImageDataStatus::UninitializedImageData); > > } > > + w = std::max(1, w >> 1); > > + h = std::max(1, h >> 1); > > Stick with division. The compiler should pick up this optimization anyways. OK. Slightly more complex code is generated as GLsizei is signed, but it's still bit ops, reasonably cheap, and this isn't very performance criticial anyways. > > ::: dom/canvas/WebGLContextValidate.cpp > @@ +822,5 @@ > > > > // TODO: WebGL 2 > > if (texImageTarget == LOCAL_GL_TEXTURE_3D) { > > if (depth < 0) { > > ErrorInvalidValue("%s: depth must be >= 0", InfoFrom(func, dims)); > > Doesn't depth need to be >0, not >=0? > A 2D texture technically has a depth of 1, implicitly. Does the API say > differently? But here we are in the case of a 3D texture, and the 'depth' is not the depth of the whole texture image, it's the depth of the region being updated by this texSubImage3D call. The case when width/height/depth is zero is a no-operation. > > ::: dom/canvas/WebGLTexture.h > @@ +127,5 @@ > > + * Note that mWidth and mHeight are inherited from WebGLRectangleObject. > > + * It's a pity to store a useless mDepth on non-3D texture images, but > > + * the size of GLsizei is negligible compared to the typical size of a texture image. > > + */ > > + GLsizei mDepth; > > I don't love how we have mWidth/mHeight from WebGLRectangleObject, but > mDepth here. I agree. I considered dropping the WebGLRectangleObject inheritance, but 1) it would have been a more intrusive patch and 2) we rely on it in a nontrivial way in WebGLFramebuffer::Attachment::RectangleObject().
Assignee | ||
Comment 18•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5383ac93d36d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/25a2488b32d7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/191b4351c850 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/160578cdf191 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/212571e28648
Comment 19•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #14) > ::: dom/canvas/WebGLContextValidate.cpp > > ErrorInvalidEnum("%s: invalid format %s: requires WebGL version 2.0 or newer", > > + InfoFrom(func, dims), EnumName(format)); > > I like this InfoFrom thing. Thank you.
Assignee | ||
Comment 20•10 years ago
|
||
Ah yes, that's one of the many things taken from Dan's original patch :)
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5383ac93d36d https://hg.mozilla.org/mozilla-central/rev/25a2488b32d7 https://hg.mozilla.org/mozilla-central/rev/191b4351c850 https://hg.mozilla.org/mozilla-central/rev/160578cdf191 https://hg.mozilla.org/mozilla-central/rev/212571e28648
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•