1323626 - [WebGL2 conformance test] Pass conformance2/textures/misc/tex-mipmap-levels.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Daosheng Mu[:daoshengmu]]

https://www.khronos.org/registry/webgl/sdk/tests/conformance2/textures/misc/tex-mipmap-levels.html?webglVersion=2&quiet=0

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: texSize.hlsl

wtu.checkCanvas(gl, [255, 0, 0, 255], "should draw with [255, 0, 0, 255]");
FAIL should draw with [255, 0, 0, 255]

I am thinking that this is a fragement shader:

fragColor = (textureSize(tex, lod) == texSize ? vec4(255, 0, 0, 255) : vec4(0, 0, 0, 255));

In ANGLE, we will convert it to HLSL as this attachment. I have confirmed we get the same shader code as Chromium.

Comment 2

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #1)
> Created attachment 8818777 [details]
> texSize.hlsl
> 
> wtu.checkCanvas(gl, [255, 0, 0, 255], "should draw with [255, 0, 0, 255]");
> FAIL should draw with [255, 0, 0, 255]
> 
> I am thinking that this is a fragement shader:
> 
> fragColor = (textureSize(tex, lod) == texSize ? vec4(255, 0, 0, 255) :
> vec4(0, 0, 0, 255));
> 
The value of textureSize(tex, lod) is always (1, 1). No matter I change the texStorage2D or gl.TEXTURE_BASE_LEVEL.

HLSL doesn't support textureSize(), so ANGLE uses DriverConstants for this kind of stuff. But, I notice the SamplerMetadataD3D11 doesn't be consistent with the TextureState of Texture. (https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/libANGLE/renderer/d3d/d3d11/Renderer11.cpp#2525)

> In ANGLE, we will convert it to HLSL as this attachment. I have confirmed we
> get the same shader code as Chromium.

Comment 3

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8818777 [details]
texSize.hlsl

>
>static float4 out_webgl_b79fa51523a10f88 = {0, 0, 0, 0};
>
>cbuffer DriverConstants : register(b1)
>{
>    struct SamplerMetadata
>    {
>        int baseLevel;

Ideally, baseLevel should be 1 because we call gl.texParameteri(gl.TEXTURE_3D, gl.TEXTURE_BASE_LEVEL, 1).
I have confirmed that SetTexParameteri() set the baseLevel to 1 to TextureState, but DriverConstants do not get this value.

>        int internalFormatBits;
>        int wrapModes;
>        int padding;
>    };
>    SamplerMetadata samplerMetadata[1] : packoffset(c4);
>};
>
>int3 gl_texture3DSize(const uint samplerIndex, int lod)
>{
>    const uint textureIndex = samplerIndex - textureIndexOffset3D;
>    const uint samplerArrayIndex = samplerIndex - samplerIndexOffset3D;
>int baseLevel = samplerMetadata[samplerIndex].baseLevel;
>    uint width; uint height; uint depth; uint numberOfLevels;
>    textures3D[textureIndex].GetDimensions(baseLevel, width, height, depth, numberOfLevels);
>    width = max(width >> lod, 1);
>    height = max(height >> lod, 1);
>    depth = max(depth >> lod, 1);
>    return int3(width, height, depth);}
>

Comment 4

[Daosheng Mu[:daoshengmu]]

On MacOSX runs native GL, textureSize(tex, lod) is (0,0). I think we might has some issues at the mipmap generation or texStorage function.

Comment 5

[Daosheng Mu[:daoshengmu]]

After set isDataInitialized to true at https://dxr.mozilla.org/mozilla-central/rev/d4b3146a5567a7ddbcdfa5244945db55616cb8d1/dom/canvas/WebGLTextureUpload.cpp#1145, we can fix this bug on Windows. On MacOSX, we still have something wrong for GL_TEXTURE_BASE_LEVEL. No matter the value of GL_TEXTURE_BASE_LEVEL, it would always be 0 at the result.

Comment 6

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #5)
> After set isDataInitialized to true at
> https://dxr.mozilla.org/mozilla-central/rev/
> d4b3146a5567a7ddbcdfa5244945db55616cb8d1/dom/canvas/WebGLTextureUpload.
> cpp#1145, we can fix this bug on Windows. On MacOSX, we still have something
> wrong for GL_TEXTURE_BASE_LEVEL. No matter the value of
> GL_TEXTURE_BASE_LEVEL, it would always be 0 at the result.

Probably, we have to make isDataInitialized to be true, otherwise, the out_fakeBlack would be  FakeBlackType::RGBA0001 at
https://dxr.mozilla.org/mozilla-central/rev/567894f026558e6dada617a3998f29aed06ac7d8/dom/canvas/WebGLTexture.cpp#474, and then BindFakeBlack().

Comment 7

[Kelsey Gilbert [:jgilbert]]

Thanks for the investigation!

It turns out our optimization that uses FakeBlack textures for uninitialized texture data is incompatible with glsl's textureSize().

That's fine, since this isn't a useful optimization anyway.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1323626 - Initialize tex image data during completeness checks. -

Review commit: https://reviewboard.mozilla.org/r/99976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99976/

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8820481 [details]
Bug 1323626 - Initialize tex image data during completeness checks. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99976/diff/1-2/

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8820481 [details]
Bug 1323626 - Initialize tex image data during completeness checks. -

https://reviewboard.mozilla.org/r/99976/#review100514

r+ after checking the redundant function.

::: dom/canvas/WebGLTexture.h:196
(Diff revision 1)
>  
>          bool IsDataInitialized() const { return mIsDataInitialized; }
>  
>          void SetIsDataInitialized(bool isDataInitialized, WebGLTexture* tex);
> +
> +        bool Initialize();

Could you help me check if it is a redundant function? I didn't see any implementation on it.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> Comment on attachment 8820481 [details]
> Bug 1323626 - Initialize tex image data during completeness checks. -
> 
> https://reviewboard.mozilla.org/r/99976/#review100514
> 
> r+ after checking the redundant function.
> 
> ::: dom/canvas/WebGLTexture.h:196
> (Diff revision 1)
> >  
> >          bool IsDataInitialized() const { return mIsDataInitialized; }
> >  
> >          void SetIsDataInitialized(bool isDataInitialized, WebGLTexture* tex);
> > +
> > +        bool Initialize();
> 
> Could you help me check if it is a redundant function? I didn't see any
> implementation on it.

Yep, my bad. Ended up discarding it but forgot about the header decl.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8820481 [details]
Bug 1323626 - Initialize tex image data during completeness checks. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99976/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec779e626454
Initialize tex image data during completeness checks. - r=daoshengmu

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/774d2940581c

Comment 16

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a700c236e596