1305832 - Fix CopyTexImage format selection and texture zeroing

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1305832 - CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. -

Only needed for ANGLE_depth_texture.

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

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1305832 - ZeroTextureData doesn't need x/y/zOffset, so remove those args. -

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

Comment 3

[Ethan Lin[:ethlin]]

Comment on attachment 8795453 [details]
Bug 1305832 - ZeroTextureData doesn't need x/y/zOffset, so remove those args. -

https://reviewboard.mozilla.org/r/81492/#review80288

Comment 4

[Ethan Lin[:ethlin]]

Comment on attachment 8795452 [details]
Bug 1305832 - CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. -

https://reviewboard.mozilla.org/r/81490/#review80290

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this still need to land and possibly get uplifted? It's listed as a blocking crash for WebGL2.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Yes, thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b70ccd12b42
Only use ZeroTexImageWithClear if we really have to. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97b53fd3b7e
ZeroTextureData doesn't need x/y/zOffset, so remove those args. - r=ethlin

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8795452 [details]
Bug 1305832 - CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. -

Approval Request Comment
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8795452 [details]
Bug 1305832 - CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. -

OK, let's try this crash fix on aurora and beta.

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/db2aab7c784b for Windows assertion failures, https://treeherder.mozilla.org/logviewer.html#?job_id=40142069&repo=mozilla-inbound

Comment 11

[Phil Ringnalda (:philor)]

And a different, non-assertion, Mac opt failure: https://treeherder.mozilla.org/logviewer.html#?job_id=40144277&repo=mozilla-inbound

Comment 12

[Phil Ringnalda (:philor)]

And a different assertion failure, Mac debug: https://treeherder.mozilla.org/logviewer.html#?job_id=40144377&repo=mozilla-inbound

Comment 13

[Phil Ringnalda (:philor)]

And what I hope is the last one, Windows opt permaorange looking like it's just bug 1292461 but being perma: https://treeherder.mozilla.org/logviewer.html#?job_id=40146598&repo=mozilla-inbound

Comment 14

[Phil Ringnalda (:philor)]

Sigh, and then considerably later backed out the other cset in https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3b96cee2ea when I finally realized I'd only done one the first try.

Comment 15

[Carsten Book [:Tomcat]]

Jeff, i guess comments 10-14 are for you. Seems we need to fix first the issues that came up on inbound before uplifting

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8795452 [details]
Bug 1305832 - CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. -

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

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1305832 - Only glClear for ANGLE's ANGLE_depth_texture textures. -

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

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1305832 - Move texture clearing code into WebGLTexture.cpp. -

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

Comment 19

[Ethan Lin[:ethlin]]

Comment on attachment 8817670 [details]
Bug 1305832 - Move texture clearing code into WebGLTexture.cpp. -

https://reviewboard.mozilla.org/r/97890/#review98900

Comment 20

[Ethan Lin[:ethlin]]

Comment on attachment 8817669 [details]
Bug 1305832 - Only glClear for ANGLE's ANGLE_depth_texture textures. -

https://reviewboard.mozilla.org/r/97888/#review98898

::: dom/canvas/WebGLTextureUpload.cpp:1962
(Diff revision 1)
>      do {
>          const auto& idealUnpack = dstUsage->idealUnpack;
>          if (!isSubImage) {
> +            UniqueBuffer buffer;
> +
> +            if (rwWidth != dstWidth || rwHeight != dstHeight) {

Why do you do the calloc only when the size is mismatched? Can you explain a little bit?

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8817669 [details]
Bug 1305832 - Only glClear for ANGLE's ANGLE_depth_texture textures. -

https://reviewboard.mozilla.org/r/97888/#review98898

> Why do you do the calloc only when the size is mismatched? Can you explain a little bit?

Sure: We don't need to upload zeros if we're going to copyTexSubImage over the whole texImage.

Comment 22

[Ethan Lin[:ethlin]]

Comment on attachment 8817669 [details]
Bug 1305832 - Only glClear for ANGLE's ANGLE_depth_texture textures. -

https://reviewboard.mozilla.org/r/97888/#review99014

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3beb2dd5adea
Only use ZeroTexImageWithClear if we really have to. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5504fbe50a91
ZeroTextureData doesn't need x/y/zOffset, so remove those args. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/128cffb39a39
CopyTex[Sub]Image should specify using TexImage to convince the driver to use the right internal format. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8f02c6edba
Only glClear for ANGLE's ANGLE_depth_texture textures. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/80265d017a07
Move texture clearing code into WebGLTexture.cpp. - r=ethlin

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3beb2dd5adea
https://hg.mozilla.org/mozilla-central/rev/5504fbe50a91
https://hg.mozilla.org/mozilla-central/rev/128cffb39a39
https://hg.mozilla.org/mozilla-central/rev/ea8f02c6edba
https://hg.mozilla.org/mozilla-central/rev/80265d017a07

Comment 25

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ddf24a98ed7

Comment 26

[Kelsey Gilbert [:jgilbert]]

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