1267147 - WebGL2 conformance test fail: conformance/misc/uninitialized-test.html

Status: RESOLVED WORKSFORME

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

We hit the assertion in CopyTexImage2D: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTextureUpload.cpp#1832.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: fix destination format in CopyTexImage2D

On windows platform, angle will return invalid operation[1] in this conformance test due to the mismatched format between source and destination. In CopyTexImae2D, we try to get the dstFormat by the internal format and srcFormat->componentType[2]. In this test, the internalFormat is RGBA and componentType is NormUInt, so we set dstFormat as RGBA8 while the srcFormat is RGBA4.
I think we may use the srcUsage as dstUsage directly while the internal format matches the source's unsized format to fix this problem.
 


[1] https://hg.mozilla.org/mozilla-central/annotate/fc15477ce628599519cb0055f52cc195d640dc94/gfx/angle/src/libANGLE/validationES3.cpp#l837
[2] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLTextureUpload.cpp#1747

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8744843 [details] [diff] [review]
fix destination format in CopyTexImage2D

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

I believe ANGLE is wrong.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8744843 [details] [diff] [review]
fix destination format in CopyTexImage2D

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

ANGLE was right, but we need a different change.

Relevant spec text is GLES 3.0.4 p141:

The effective internal format of the source buffer is determined with the fol-
lowing rules applied in order:
* If the source buffer is a texture or renderbuffer that was created with a sized
internal format then the effective internal format is the source buffer’s sized
internal format.
* If the source buffer is a texture that was created with an unsized base internal
format, then the effective internal format is the source image array’s effective
internal format, as specified by table 3.12, which is determined from the
`format` and `type` that were used when the source image array was specified
by `TexImage*`.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1747,5 @@
> +           (srcFormat->unsizedFormat == webgl::UnsizedFormat::LA && internalFormat == LOCAL_GL_LUMINANCE_ALPHA) ||
> +           (srcFormat->unsizedFormat == webgl::UnsizedFormat::L && internalFormat == LOCAL_GL_LUMINANCE) ||
> +           (srcFormat->unsizedFormat == webgl::UnsizedFormat::A && internalFormat == LOCAL_GL_ALPHA)) {
> +           // If the internal format matches source unsized format, use the srcUsage directly.
> +           dstUsage = srcUsage;

I think we doing this unconditionally.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: fix destination format in CopyTexImage2D

I found the spec p140 has some descriptions about how to determine effective internal format by source component size. So I tried to implement the rules. The p141 descriptions seem about the source format which we already have in this stage.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8750237 [details] [diff] [review]
fix destination format in CopyTexImage2D

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

Thanks for investigating this. It's just a super awful part of the spec. :(

::: dom/canvas/WebGLFormats.cpp
@@ +971,5 @@
> +    //  a luminance component, a luminance component is considered to match with a red
> +    //  component. Note that the above rules disallow matches where some components
> +    //  sizes are smaller and others are larger. If multiple possible matches exist in the
> +    //  same rule, the one with the closest component sizes is chosen."
> +    const FormatUsageInfo *greaterFormatInfo = nullptr;

Star to the left!

@@ +979,5 @@
> +
> +    for (const std::pair<const PackingInfo, const FormatUsageInfo*> texFormat : mUnsizedTexFormatMap) {
> +        if (texFormat.first.format == internalFormat) {
> +            const FormatUsageInfo *usageInfo = texFormat.second;
> +            if (usageInfo->format->estimatedBytesPerPixel == bytesPerPixel) {

Don't use estimatedBytesPerPixel for this. (it doesn't work)

This is actually a really insane part of the spec.
For instance, for a source effective format of RGB10_A2, internalformat=RGB appears to yield a dest effective format of R11F_G11F_B10F.

If we end up pursuing this, we should just hardcode the correct shortened format into the format info.

However, I'm going to propose we write this out of the spec. I think WebGL 2 should require that effective formats always match exactly. (it sounds like ANGLE already behaves like this)

Let's patch this based on that assumption.

@@ +995,5 @@
> +    }
> +
> +    if (greaterFormatInfo) {
> +        return greaterFormatInfo;
> +    }

Don't brace single-line returns.

Comment 6

[Kelsey Gilbert [:jgilbert]]

I'll propose the spec change.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Fix destination format in CopyTexImage2D

Address jgilbert's comments. Find a format with matched effective format.

Comment 8

[Ethan Lin[:ethlin]]

(In reply to Ethan Lin[:ethlin] from comment #7)
> Created attachment 8752075 [details] [diff] [review]
> Fix destination format in CopyTexImage2D
> 
> Address jgilbert's comments. Find a format with matched effective format.

This way still can fix the uninitialized-test.html failures.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: Fix destination format in CopyTexImage2D

Correct the indent.

Comment 10

[Kelsey Gilbert [:jgilbert]]

This WFM on Nightly now.
Can you confirm?

Comment 11

[Ethan Lin[:ethlin]]

Confirmed.