Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy into integer cube map texture should work #2369

Merged
merged 2 commits into from Apr 18, 2017

Conversation

qkmiao
Copy link
Contributor

@qkmiao qkmiao commented Apr 17, 2017

This is a regression test for crbug.com/712117.

This is a regression test for crbug.com/712117.
Copy link
Contributor

@zhenyao zhenyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for debugging this and figuring out the driver issue on AMD Linux.

A few minor suggestions.

@@ -2,6 +2,7 @@
--min-version 2.0.1 angle-stuck-depth-textures.html
canvas-remains-unchanged-after-used-in-webgl-texture.html
--min-version 2.0.1 compressed-tex-from-pbo-crash.html
copy-texture-cube-map-AMD-bug.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add --min-version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added min-version 2.0.1 for this test.

var width = 16;
var height = 16;
var level = 0;
var internalformat = gl.RGBA8UI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expand the test to cover also RGBA8I, RGBA8, RGBA32F?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RGBA32F isn't supported by CopyTexImage2D in chrome now. Should we support floating formats if https://www.khronos.org/registry/webgl/extensions/EXT_color_buffer_float/ is enabled? I didn't see explicit specification about this in the extension spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added other formats. We can easily add floating formats in the test if we decide to.

@qkmiao
Copy link
Contributor Author

qkmiao commented Apr 18, 2017

Thanks @zhenyao . I updated this pr to fix your comments. PTAL.

@MarkCallow
Copy link
Collaborator

Should we support floating formats if https://www.khronos.org/registry/webgl/extensions/EXT_color_buffer_float/ is enabled? I didn't see explicit specification about this in the extension spec.

That's because it is mostly covered in the core spec. The extension spec. modifies the CopyTexImage INVALID_OPERATION error on p.139 so it is only raised when trying to copy floating point data to a format that is not floating- or fixed-point.

ReadPixels is modified to support RGBA and FLOAT. (Table 3.15 should have been modified to include RGBA and FLOAT but I the modification to ReadPixels makes this combination valid.

Table 3.17 is modified to include RG{B,BA}32F.

The core spec. says

If internalformat is sized, the internal format of the new texel array is internalformat, and this is also the new texel array’s effective internal format. If the component sizes of internalformat do not exactly match the corresponding component sizes of the source buffer’s effective internal format, described below, an INVALID_OPERATION error is generated.

So the conclusion is that copying RGBA32F to RGBA32F is supported.

@qkmiao
Copy link
Contributor Author

qkmiao commented Apr 18, 2017

@msc- I think you are right. Thanks. I will fix this issue in chrome and add conformance test to check this.

@zhenyao
Copy link
Contributor

zhenyao commented Apr 18, 2017

LGTM
Please add float test case with extension check in a followup CL

@zhenyao zhenyao merged commit 3cbadd4 into KhronosGroup:master Apr 18, 2017
@qkmiao qkmiao deleted the AMD-copy-cube-map-texture-bug branch April 28, 2017 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants