Closed Bug 1049960 Opened 6 years ago Closed 6 years ago

webgl-1.0.2 Doesn't generate error for invalid formats to copyTexImage2D - copy-tex-image-2d-formats.html

Categories

(Core :: Canvas: WebGL, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: wlitwinczyk, Assigned: wlitwinczyk)

References

Details

Attachments

(1 file, 9 obsolete files)

Related to bug 896693

https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/textures/copy-tex-image-2d-formats.html

This bug is only meant to fix the code so it generates INVALID_OPERATION errors when for copyTex[Sub]Image2D. The original regression will be fixed in the other bug.
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
I feel a little iffy about putting some state-altering code in the verification method, specifically CheckAndInitializeAttachments(), because it's otherwise state-less. I also created a helper method for getting the format from an FBO because it's pretty verbose normally.
Attachment #8468891 - Flags: review?(jgilbert)
Comment on attachment 8468891 [details] [diff] [review]
copy_tex_invalid_op_patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ -479,5 @@
> -
> -        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> -        if (!mBoundFramebuffer->HasCompletePlanes(readPlaneBits)) {
> -            return ErrorInvalidOperation("copyTexImage2D: Read source attachment doesn't have the"
> -                                         " correct color/depth/stencil type.");

I don't think we can remove this stuff.

@@ -586,5 @@
> -
> -        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> -        if (!mBoundFramebuffer->HasCompletePlanes(readPlaneBits)) {
> -            return ErrorInvalidOperation("copyTexSubImage2D: Read source attachment doesn't have the"
> -                                         " correct color/depth/stencil type.");

Or this.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +80,5 @@
> +WebGLFramebuffer::GetFormatForAttachment(const WebGLFramebuffer::Attachment& attachment) const
> +{
> +    if (!attachment.IsDefined()) {
> +        MOZ_ASSERT(false, "Should verify that the attachment is defined first!");
> +        return LOCAL_GL_NONE; /* Should cause another error somewhere else */

Just replace this block with MOZ_ASSERT(IsDefined)

@@ +84,5 @@
> +        return LOCAL_GL_NONE; /* Should cause another error somewhere else */
> +    }
> +
> +    if (attachment.Texture()) {
> +        const WebGLTexture& tex = *attachment.Texture();

This is an errant `*`, right?

@@ +86,5 @@
> +
> +    if (attachment.Texture()) {
> +        const WebGLTexture& tex = *attachment.Texture();
> +        if (!tex.HasImageInfoAt(tex.Target(), 0)) {
> +            MOZ_ASSERT(false, "Should verify that the attachment is complete first!");

Just assert this directly, then assume it.
Attachment #8468891 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8468891 [details] [diff] [review]
> copy_tex_invalid_op_patch
> 
> Review of attachment 8468891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextGL.cpp
> @@ -479,5 @@
> > -
> > -        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> > -        if (!mBoundFramebuffer->HasCompletePlanes(readPlaneBits)) {
> > -            return ErrorInvalidOperation("copyTexImage2D: Read source attachment doesn't have the"
> > -                                         " correct color/depth/stencil type.");
> 
> I don't think we can remove this stuff.

These haven't been removed, I moved them into WebGLContextValidate.cpp because they were the same for
both CopyTexImage2D and CopyTexSubImage2D (minus the name). The code path /should/ be equivalent. But I'll double check.

> @@ -586,5 @@
> > -
> > -        GLenum readPlaneBits = LOCAL_GL_COLOR_BUFFER_BIT;
> > -        if (!mBoundFramebuffer->HasCompletePlanes(readPlaneBits)) {
> > -            return ErrorInvalidOperation("copyTexSubImage2D: Read source attachment doesn't have the"
> > -                                         " correct color/depth/stencil type.");
> 
> Or this.

Above.

> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +80,5 @@
> > +WebGLFramebuffer::GetFormatForAttachment(const WebGLFramebuffer::Attachment& attachment) const
> > +{
> > +    if (!attachment.IsDefined()) {
> > +        MOZ_ASSERT(false, "Should verify that the attachment is defined first!");
> > +        return LOCAL_GL_NONE; /* Should cause another error somewhere else */
> 
> Just replace this block with MOZ_ASSERT(IsDefined)

Will do.

> @@ +84,5 @@
> > +        return LOCAL_GL_NONE; /* Should cause another error somewhere else */
> > +    }
> > +
> > +    if (attachment.Texture()) {
> > +        const WebGLTexture& tex = *attachment.Texture();
> 
> This is an errant `*`, right?

Unfortunately not, Texture() returns a pointer.

> @@ +86,5 @@
> > +
> > +    if (attachment.Texture()) {
> > +        const WebGLTexture& tex = *attachment.Texture();
> > +        if (!tex.HasImageInfoAt(tex.Target(), 0)) {
> > +            MOZ_ASSERT(false, "Should verify that the attachment is complete first!");
> 
> Just assert this directly, then assume it.

Will do.
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
The moved code should be O-K because it's inside ValidateTexImage which is called just prior to where the if statements were before in CopyTex[Sub]Image2D. So they will be checked before the rest of the CopyTex[Sub]Image2D like before.
Attachment #8468891 - Attachment is obsolete: true
Attachment #8469376 - Flags: review?(jgilbert)
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
Should the enum be replaced with one of the strongly typed enums? In case something like a "HasComponent()" method needs to be added?
Attachment #8469376 - Attachment is obsolete: true
Attachment #8469376 - Flags: review?(jgilbert)
Attachment #8470271 - Flags: review?(jgilbert)
Attachment #8470271 - Flags: review?(jgilbert)
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
Cleaned up a bit, added a struct that handles component comparison.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b0d259b70201
Attachment #8470271 - Attachment is obsolete: true
Attachment #8471675 - Flags: review?(jgilbert)
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
Found some accidental regressions. They have now been fixed.
Attachment #8471675 - Attachment is obsolete: true
Attachment #8471675 - Flags: review?(jgilbert)
Attachment #8471786 - Flags: review?(jgilbert)
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
Apparently I uploaded the patch with a typo.

New Try: https://tbpl.mozilla.org/?tree=Try&rev=9f43e3743503
Attachment #8471786 - Attachment is obsolete: true
Attachment #8471786 - Flags: review?(jgilbert)
Attachment #8471849 - Flags: review?(jgilbert)
Comment on attachment 8471849 [details] [diff] [review]
copy_tex_invalid_op_patch

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

r- for the IsCopyFunc() specific code added to ValidateTexImage.

::: dom/canvas/WebGLContextUtils.h
@@ +30,5 @@
> +        Stencil,
> +        MAX_INDEX
> +    };
> +
> +    bool mComponents[Component::MAX_INDEX];

This seems like a perfect thing for bit flags, but whatever.

::: dom/canvas/WebGLContextValidate.cpp
@@ +1318,5 @@
>          return false;
>      }
>  
> +    // Checks specific to the CopyTex[Sub]Image2D functions
> +    if (IsCopyFunc(func)) {

This is a lot of code to dump in here. I know there's already a lot of code here already, but it was intended to be just for common checks.

I'd like to see this code extracted into another method and called from CopyTex[Sub]Image2D.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +89,5 @@
> +        const WebGLTexture::ImageInfo& imgInfo = tex.ImageInfoAt(tex.Target(), 0);
> +        return imgInfo.WebGLFormat();
> +    }
> +
> +    if (attachment.Renderbuffer()) {

Remove extra {}s when only one line in if.
Attachment #8471849 - Flags: review?(jgilbert) → review-
Drive-by review and apologies for my tortuous grammar. ("already lots of code already", I need to learn the art of proof-reading.)
(In reply to Dan Glastonbury :djg :kamidphish from comment #9)
> Comment on attachment 8471849 [details] [diff] [review]
> copy_tex_invalid_op_patch
> 
> Review of attachment 8471849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the IsCopyFunc() specific code added to ValidateTexImage.
> 
> ::: dom/canvas/WebGLContextUtils.h
> @@ +30,5 @@
> > +        Stencil,
> > +        MAX_INDEX
> > +    };
> > +
> > +    bool mComponents[Component::MAX_INDEX];
> 
> This seems like a perfect thing for bit flags, but whatever.

Yeah, I guess I could change that. I don't think we're going to get many more components of color any time soon.

> 
> ::: dom/canvas/WebGLContextValidate.cpp
> @@ +1318,5 @@
> >          return false;
> >      }
> >  
> > +    // Checks specific to the CopyTex[Sub]Image2D functions
> > +    if (IsCopyFunc(func)) {
> 
> This is a lot of code to dump in here. I know there's already a lot of code
> here already, but it was intended to be just for common checks.
> 
> I'd like to see this code extracted into another method and called from
> CopyTex[Sub]Image2D.

Okay. I added it here because it seemed unnecessary to have the exact same code duplicated in both of the CopyTex[Sub]Image2D methods. There was also a little bit of precedent with IsSubFunc() (although subfunc more general), but I agree, will move it.

> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +89,5 @@
> > +        const WebGLTexture::ImageInfo& imgInfo = tex.ImageInfoAt(tex.Target(), 0);
> > +        return imgInfo.WebGLFormat();
> > +    }
> > +
> > +    if (attachment.Renderbuffer()) {
> 
> Remove extra {}s when only one line in if.

Will do.
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
The bitfields version is much cleaner I think, don't even need a loop now. I checked manually on Linux 1.0.2 and made sure there weren't any regressions.

Try: https://tbpl.mozilla.org/?tree=Try&rev=6bd0a12f3bd2
Attachment #8471849 - Attachment is obsolete: true
Attachment #8473268 - Flags: review?(jgilbert)
Attachment #8473268 - Flags: feedback?(dglastonbury)
Comment on attachment 8473268 [details] [diff] [review]
copy_tex_invalid_op_patch

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

Looks OK.

::: dom/canvas/WebGLContextGL.cpp
@@ +372,5 @@
>  
> +    if (!ValidateCopyTexImage(internalformat, func))
> +        return;
> +
> +    if (!mBoundFramebuffer) {

I'm on a rampage to exterminate {} for single lines.

::: dom/canvas/WebGLContextUtils.h
@@ +27,5 @@
> +            unsigned char mG : 1;
> +            unsigned char mB : 1;
> +            unsigned char mA : 1;
> +            unsigned char mDepth : 1;
> +            unsigned char mStencil : 1;

I don't think you need to go to these extremes.

I was thinking more #define/enum:
RED_BIT     = 0x01;
GREEN_BIT   = 0x02;
BLUE_BIT    = 0x04;
ALPHA_BIT   = 0x08;
DEPTH_BIT   = 0x10;
STENCIL_BIT = 0x20;

Just OR those into mComponents in GLComponents constructor. eg: mComponents |= (RED_BIT | GREEN_BIT | BLUE_BIT);

I'm not fussed about the names of RED_BIT, etc., I just picked those for illustration.
Attachment #8473268 - Flags: feedback?(dglastonbury) → feedback+
Comment on attachment 8473268 [details] [diff] [review]
copy_tex_invalid_op_patch

Had a little mix-up in another patch, so I'm replicating Jeff's review here, original is at:

https://bugzilla.mozilla.org/show_bug.cgi?id=937009#c5

------------------------------------------------------

Cool, but let's tighten it up a bit.

Oops, it looks like this is for the wrong bug. My comments still apply!

::: dom/canvas/WebGLContextGL.cpp
@@ +373,5 @@
> +    if (!ValidateCopyTexImage(internalformat, func))
> +        return;
> +
> +    if (!mBoundFramebuffer) {
> +        ClearBackbufferIfNeeded();

We should try to keep this after all errors, but it doesn't matter that much, since this is just us fulfilling our promise early.

::: dom/canvas/WebGLContextUtils.cpp
@@ +94,5 @@
> +
> +bool
> +GLComponents::IsSubsetOf(const GLComponents& other) const
> +{
> +    return (mComponents & other.mComponents) == mComponents;

Neat.

::: dom/canvas/WebGLContextUtils.h
@@ +29,5 @@
> +            unsigned char mA : 1;
> +            unsigned char mDepth : 1;
> +            unsigned char mStencil : 1;
> +        };
> +        unsigned char mComponents;

This is unnecessary, even if it is pretty cute. Just use a normal bitmask with `|`s.

::: dom/canvas/WebGLContextValidate.cpp
@@ +1299,5 @@
> +
> +    // Check for a required alpha component
> +    bool texFormatRequiresAlpha = FormatHasAlpha(format);
> +    bool fboFormatHasAlpha = mBoundFramebuffer ? mBoundFramebuffer->ColorAttachment(0).HasAlpha()
> +                                               : bool(gl->GetPixelFormat().alpha > 0);

I'm pretty sure this check should be included in the IsSubsetOf code, since it alpha is just a channel in the set of channels.
R- unless there's a reason this is here instead.
Attachment #8473268 - Flags: review?(jgilbert) → review-
(In reply to Walter Litwinczyk [:walter] from comment #14)
> ::: dom/canvas/WebGLContextUtils.h
> @@ +29,5 @@
> > +            unsigned char mA : 1;
> > +            unsigned char mDepth : 1;
> > +            unsigned char mStencil : 1;
> > +        };
> > +        unsigned char mComponents;
> 
> This is unnecessary, even if it is pretty cute. Just use a normal bitmask
> with `|`s.

:(
 
> ::: dom/canvas/WebGLContextValidate.cpp
> @@ +1299,5 @@
> > +
> > +    // Check for a required alpha component
> > +    bool texFormatRequiresAlpha = FormatHasAlpha(format);
> > +    bool fboFormatHasAlpha = mBoundFramebuffer ? mBoundFramebuffer->ColorAttachment(0).HasAlpha()
> > +                                               : bool(gl->GetPixelFormat().alpha > 0);
> 
> I'm pretty sure this check should be included in the IsSubsetOf code, since
> it alpha is just a channel in the set of channels.
> R- unless there's a reason this is here instead.

I only kept it here because of the default framebuffer check, as the subset code is inside of the mBoundFramebuffer() if statement, but I could probably refactor it to make this part unnecessary.
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
The new validate code is a little cleaner.

New Try: https://tbpl.mozilla.org/?tree=Try&rev=8e64498ba74b
Attachment #8473268 - Attachment is obsolete: true
Attachment #8475525 - Flags: review?(jgilbert)
Comment on attachment 8475525 [details] [diff] [review]
copy_tex_invalid_op_patch

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

LGTM
Attachment #8475525 - Flags: feedback+
Attached patch copy_tex_invalid_op_patch (obsolete) — Splinter Review
Right, forgot the real reason I left the other code, the default framebuffer may not have an alpha component.

Try: https://tbpl.mozilla.org/?tree=Try&rev=92f7a99f1773
Attachment #8475525 - Attachment is obsolete: true
Attachment #8475525 - Flags: review?(jgilbert)
Attachment #8475585 - Flags: review?(jgilbert)
Comment on attachment 8475585 [details] [diff] [review]
copy_tex_invalid_op_patch

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

::: dom/canvas/WebGLContextUtils.cpp
@@ +93,5 @@
> +
> +bool
> +GLComponents::IsSubsetOf(const GLComponents& other) const
> +{
> +    return (mComponents & other.mComponents) == mComponents;

I personally think it's more obvious if you do:
mComponents | other.mComponents == other.mComponents.
'If we add mComponents to other.mComponents, are there still no channels besides those in other.mComponents.'

::: dom/canvas/WebGLContextUtils.h
@@ +24,5 @@
> +    unsigned char mComponents;
> +
> +    enum Components {
> +        Red     = 0x01,
> +        Green   = 0x02,

Use (1 << 0) and friends.
Attachment #8475585 - Flags: review?(jgilbert) → review+
r=jgilbert carried forward
Attachment #8475585 - Attachment is obsolete: true
Attachment #8476213 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b9467a61ff89
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.