Closed Bug 1075195 Opened 5 years ago Closed 5 years ago

WebGL2: implement texStorage2D

Categories

(Core :: Canvas: WebGL, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

No description provided.
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-
(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.
Attachment #8497815 - Attachment is obsolete: true
Attachment #8498123 - Flags: review?(dglastonbury)
Attachment #8498123 - Flags: review?(dglastonbury) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80fc984aaa6

(folded 2 patches into one with both our identities)
Assignee: nobody → bjacob
https://hg.mozilla.org/mozilla-central/rev/b80fc984aaa6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.