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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Walter Litwinczyk [:walter]]

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.

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

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.

Comment 2

[Kelsey Gilbert [: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.

Comment 3

[Walter Litwinczyk [:walter]]

(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.

Comment 4

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

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.

Comment 5

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

Should the enum be replaced with one of the strongly typed enums? In case something like a "HasComponent()" method needs to be added?

Comment 6

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

Cleaned up a bit, added a struct that handles component comparison.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b0d259b70201

Comment 7

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

Found some accidental regressions. They have now been fixed.

Comment 8

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

Apparently I uploaded the patch with a typo.

New Try: https://tbpl.mozilla.org/?tree=Try&rev=9f43e3743503

Comment 9

[u480271]

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.

Comment 10

[u480271]

Drive-by review and apologies for my tortuous grammar. ("already lots of code already", I need to learn the art of proof-reading.)

Comment 11

[Walter Litwinczyk [:walter]]

(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.

Comment 12

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

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

Comment 13

[u480271]

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.

Comment 14

[Walter Litwinczyk [:walter]]

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.

Comment 15

[Walter Litwinczyk [:walter]]

(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.

Comment 16

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

The new validate code is a little cleaner.

New Try: https://tbpl.mozilla.org/?tree=Try&rev=8e64498ba74b

Comment 17

[u480271]

Comment on attachment 8475525 [details] [diff] [review]
copy_tex_invalid_op_patch

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

LGTM

Comment 18

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

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

Comment 19

[Kelsey Gilbert [: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.

Comment 20

[Walter Litwinczyk [:walter]]

Attachment: copy_tex_invalid_op_patch

r=jgilbert carried forward

Comment 21

[Walter Litwinczyk [:walter]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6d0945affda2

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9467a61ff89

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b9467a61ff89