Closed
Bug 1075195
Opened 10 years ago
Closed 10 years ago
WebGL2: implement texStorage2D
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
16.67 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
19.05 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8497814 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8497815 -
Flags: review?(dglastonbury)
Comment on attachment 8497815 [details] [diff] [review] Rest of texStorage2D implementation Review of attachment 8497815 [details] [diff] [review]: ----------------------------------------------------------------- r- for: * Why TexStorage3D was removed from previous patch. * Late error message after fCompressedTexImage call. ::: dom/canvas/WebGL2Context.h @@ +244,5 @@ > > WebGL2Context(); > > bool ValidateSizedInternalFormat(GLenum internalFormat, const char* info); > + bool ValidateTexStorageCall(GLenum target, GLsizei levels, GLenum internalformat, Any reason for changing the name from Params to Call? ::: dom/canvas/WebGL2ContextTextures.cpp @@ +160,5 @@ > + GLsizei w = width; > + GLsizei h = height; > + for (size_t l = 0; l < size_t(levels); l++) { > + for (size_t f = 0; f < facesCount; f++) { > + TexImageTarget imageTarget = target == LOCAL_GL_TEXTURE_2D It might be good to extract this code into a function. @@ +185,5 @@ > void > WebGL2Context::TexStorage3D(GLenum target, GLsizei levels, GLenum internalformat, > GLsizei width, GLsizei height, GLsizei depth) > { > + MOZ_CRASH("Not Implemented."); Why back to not implemented? ::: dom/canvas/WebGLContextGL.cpp @@ +3356,5 @@ > MakeContextCurrent(); > gl->fCompressedTexImage2D(texImageTarget.get(), level, internalformat, width, height, border, byteLength, view.Data()); > WebGLTexture* tex = activeBoundTextureForTexImageTarget(texImageTarget); > MOZ_ASSERT(tex); > + if (tex->IsImmutable()) { Isn't this too late? fCompressedTexImage2D will have caused an error in tex is immutable. Did I put immutable checks into the functions in WebGL2ContextValidate? If I didn't, I think that's where I intended for them to go. (Just looked through my patches. I see I hadn't got to it yet.)
Attachment #8497815 -
Flags: review?(dglastonbury) → review-
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #3) > Comment on attachment 8497815 [details] [diff] [review] > Rest of texStorage2D implementation > > Review of attachment 8497815 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for: > * Why TexStorage3D was removed from previous patch. > * Late error message after fCompressedTexImage call. > > ::: dom/canvas/WebGL2Context.h > @@ +244,5 @@ > > > > WebGL2Context(); > > > > bool ValidateSizedInternalFormat(GLenum internalFormat, const char* info); > > + bool ValidateTexStorageCall(GLenum target, GLsizei levels, GLenum internalformat, > > Any reason for changing the name from Params to Call? Because this function is validating the entire call wrt current state, not just the params. I'll just call it ValidateTexStorage. > > ::: dom/canvas/WebGL2ContextTextures.cpp > @@ +160,5 @@ > > + GLsizei w = width; > > + GLsizei h = height; > > + for (size_t l = 0; l < size_t(levels); l++) { > > + for (size_t f = 0; f < facesCount; f++) { > > + TexImageTarget imageTarget = target == LOCAL_GL_TEXTURE_2D > > It might be good to extract this code into a function. OK. > > @@ +185,5 @@ > > void > > WebGL2Context::TexStorage3D(GLenum target, GLsizei levels, GLenum internalformat, > > GLsizei width, GLsizei height, GLsizei depth) > > { > > + MOZ_CRASH("Not Implemented."); > > Why back to not implemented? Because it will need to have immutability tracking before it can land, and it also won't be useful anyway before 3D textures land, so I thought I might as well defer it entirely to when we do 3D textures, which I intend to do next. > > ::: dom/canvas/WebGLContextGL.cpp > @@ +3356,5 @@ > > MakeContextCurrent(); > > gl->fCompressedTexImage2D(texImageTarget.get(), level, internalformat, width, height, border, byteLength, view.Data()); > > WebGLTexture* tex = activeBoundTextureForTexImageTarget(texImageTarget); > > MOZ_ASSERT(tex); > > + if (tex->IsImmutable()) { > > Isn't this too late? fCompressedTexImage2D will have caused an error in tex > is immutable. Very good catch, thanks! New patch coming. > > Did I put immutable checks into the functions in WebGL2ContextValidate? If I > didn't, I think that's where I intended for them to go. (Just looked through > my patches. I see I hadn't got to it yet.) You had written an immutability check based on querying the underlying OpenGL state. I think that's the kind of state tracking that we want to do ourselves, because it controls memory allocations, the kind of thing that we're not keen on trusting the underlying GL for.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8497815 -
Attachment is obsolete: true
Attachment #8498123 -
Flags: review?(dglastonbury)
Attachment #8498123 -
Flags: review?(dglastonbury) → review+
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80fc984aaa6 (folded 2 patches into one with both our identities)
Assignee: nobody → bjacob
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b80fc984aaa6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 8•8 years ago
|
||
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/texStorage2D
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•