Closed Bug 1006908 Opened 6 years ago Closed 5 years ago

Clarify textureTarget vs texImageTarget

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jgilbert, Assigned: wlitwinczyk)

References

Details

Attachments

(1 file, 4 obsolete files)

OpenGL (and WebGL) have a somewhat subtle difference between 'texture' and 'texImage'. A texture is composed of different (mip) levels, and might have different faces for a cube map. A texImage is a single 'frame'/'slice' of a texture, as manipulated by (Copy|Compressed|)Tex(Sub)Image. A TEXTURE_2D with 3 mip levels has 3 texImages. A TEXTURE_CUBE_MAP with 3 mip levels has 3*6=18 texImages. (3 per face, 6 faces)

This leads to sometimes-cryptic code, such as:
http://hg.mozilla.org/mozilla-central/file/4e4e0f502969/content/canvas/src/WebGLTexture.cpp#l144

Let's fix it!
Blocks: 937009
Assignee: jgilbert → wlitwinczyk
Attached patch textarget_patch (obsolete) — Splinter Review
I'm not entirely sure of the scope of this patch. Also the comment about the TexImageTargetToTexTarget() function could probably be better (and maybe better name?).

But mainly:

The BindTexture() function doesn't use "activeBoundTextureForTarget()" because it's using a double pointer, where activeBoundTextureForTarget() returns just a single pointer.

This patch will probably reach out into the other functions as well and will need to most likely touch WebGLTexture a bit. Looking mostly for direction and scope.
Attachment #8476302 - Flags: feedback?(jgilbert)
Attachment #8476302 - Flags: feedback?(dglastonbury)
Comment on attachment 8476302 [details] [diff] [review]
textarget_patch

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

Umm, I guess. I think the bug came around because I wanted Jeff to explain what the ASSERT was saying. I fear WebGL2 may complicate that logic a bit with the introduction of TEXTURE_3D and TEXTURE_2D_ARRAY. Might need some more pondering and what this bug is supposed to address.
Attachment #8476302 - Flags: feedback?(dglastonbury) → feedback+
Comment on attachment 8476302 [details] [diff] [review]
textarget_patch

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

I think this is a good direction.
Attachment #8476302 - Flags: feedback?(jgilbert) → feedback+
Depends on: 1060180
I think I'm going to extend the scope of this patch, because this is a problem all over the place. Almost all of the texture code has invalid assumptions about the target vs texture image target.
Attached patch textarget_patch (obsolete) — Splinter Review
Eh, might as well put it up for review.

Personally I'd like to see the WebGL code use more types, even superficial (no meat) types that would require explicit conversions, so that the compiler could check for this kind of error. E.g. SetImageInfo would take a TexImgTarget or something. I suppose even separating the WebGL enums into different categories would be sufficent, versus passing GLenum around everywhere.

I think if we did that, a lot of asserts could disappear with the added benefit of having more confidence in the code.
Attachment #8476302 - Attachment is obsolete: true
Attachment #8481501 - Flags: review?(jgilbert)
Comment on attachment 8481501 [details] [diff] [review]
textarget_patch

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

::: dom/canvas/WebGLContext.h
@@ +135,5 @@
> +//
> +// -- Location note --
> +// This is now here instead of the more apt WebGLContextUtils because the
> +// templated TexImage2D function below needs it, and C++ needs the full
> +// definition available for templated functions. Also WebGLContextUtils depends

It only needs the declaration for functions it needs. You should be fine declaring it here and defining it elsewhere.

@@ +146,3 @@
>  {
> +    switch (texImageTarget) {
> +        case LOCAL_GL_TEXTURE_2D:

Keep with the existing case-on-same-line-as-switch.

@@ +257,5 @@
>      void DummyFramebufferOperation(const char *info);
>  
> +    WebGLTexture* activeBoundTextureForTarget(GLenum texImgTarget) const {
> +        const GLenum texTarget = TexImageTargetToTexTarget(texImgTarget);
> +        MOZ_ASSERT(texTarget == LOCAL_GL_TEXTURE_2D || LOCAL_GL_TEXTURE_CUBE_MAP);

Heh, you want:
MOZ_ASSERT(texTarget == LOCAL_GL_TEXTURE_2D ||
           texTarget == LOCAL_GL_TEXTURE_CUBE_MAP);

::: dom/canvas/WebGLTexture.cpp
@@ +141,5 @@
>                    GLsizei aWidth, GLsizei aHeight,
>                    GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus)
>  {
> +    MOZ_ASSERT(TexImageTargetToTexTarget(aTarget) == mTarget);
> +    if (TexImageTargetToTexTarget(aTarget) != mTarget)

Cool, but s/aTarget/aTexImageTarget/.
Attachment #8481501 - Flags: review?(jgilbert) → review+
Attached patch textarget_patch (obsolete) — Splinter Review
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 8481501 [details] [diff] [review]
> textarget_patch
> 
> Review of attachment 8481501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContext.h
> @@ +135,5 @@
> > +//
> > +// -- Location note --
> > +// This is now here instead of the more apt WebGLContextUtils because the
> > +// templated TexImage2D function below needs it, and C++ needs the full
> > +// definition available for templated functions. Also WebGLContextUtils depends
> 
> It only needs the declaration for functions it needs. You should be fine
> declaring it here and defining it elsewhere.

Yeah, I don't know what I was thinking.

> @@ +257,5 @@
> >      void DummyFramebufferOperation(const char *info);
> >  
> > +    WebGLTexture* activeBoundTextureForTarget(GLenum texImgTarget) const {
> > +        const GLenum texTarget = TexImageTargetToTexTarget(texImgTarget);
> > +        MOZ_ASSERT(texTarget == LOCAL_GL_TEXTURE_2D || LOCAL_GL_TEXTURE_CUBE_MAP);
> 
> Heh, you want:
> MOZ_ASSERT(texTarget == LOCAL_GL_TEXTURE_2D ||
>            texTarget == LOCAL_GL_TEXTURE_CUBE_MAP);

Doh! This fix is the reason for the new patch, found a couple of other broken areas. Tested it against the all/conformance/texture tests so hopefully I found all the areas that needed changing/differentiating.
Attachment #8481501 - Attachment is obsolete: true
Attachment #8482856 - Flags: review?(jgilbert)
Comment on attachment 8482856 [details] [diff] [review]
textarget_patch

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

::: dom/canvas/WebGLContext.h
@@ +123,5 @@
>  };
>  
> +// From WebGLContextUtils
> +GLenum
> +TexImageTargetToTexTarget(GLenum texImageTarget);

It's sort of silly, but we usually keep the type and name of function declarations on the same line, even though we want the type and name of function *definitions* on different lines.
Attachment #8482856 - Flags: review?(jgilbert) → review+
Attached patch textarget_patch (obsolete) — Splinter Review
r=jgilbert carried forward
Attachment #8482856 - Attachment is obsolete: true
Attachment #8483139 - Flags: review+
Attached patch textarget_patchSplinter Review
Try problems! :(
https://tbpl.mozilla.org/?tree=Try&rev=8ecc0f7c196b

So video textures were skating by on almost no validation...
Attachment #8483139 - Attachment is obsolete: true
Attachment #8483692 - Flags: review?(jgilbert)
Comment on attachment 8483692 [details] [diff] [review]
textarget_patch

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

Shoot, you're right. Let's try to backport this.
Attachment #8483692 - Flags: review?(jgilbert) → review+
Blocks: 1063053
https://hg.mozilla.org/mozilla-central/rev/48d31f840373
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.