Add aspect back to GPUTextureCopyView #873

kainino0x · 2020-06-20T01:00:55Z

Per discussions on #652. It was removed in #143.

Preview | Diff

spec/index.bs

kainino0x · 2020-06-22T17:17:05Z

spec/index.bs

+  - If |format| is a depth-stencil format:
+    - |copiesWholeSubresources| must be true.
+    - |source| and |destination| must refer to the same set of aspects
+        (depth, stencil, or both depth and stencil).


On a related note, if we allow multi-aspect T2T copies then it makes about as much sense to allow multi-miplevel T2T copies, but honestly both are very unimportant. We could remove multi-aspect T2T copies.

kainino0x · 2020-06-22T21:27:03Z

Considering this to be soft-blocked on #652 (comment) because it's somewhat more difficult to discuss this without that resolved.

spec/index.bs

kainino0x · 2020-07-06T19:36:36Z

TODO:

Disallow T2T copies with aspects selected
Figure out if copies to/from stencil aspect of DS format in D3D12 is tightly packed (@RafaelCintron @damyanp)
Figure out if copies to/from stencil aspect of DS format in Metal is tightly packed (@litherum)

Kangz · 2020-08-14T15:27:08Z

@austinEng can you help push this forward? It seems that the aspect member was required for B2T and T2B copies of depth-stencil no?

austinEng

IIRC, in one of the meetings we already received feedback from @RafaelCintron that single-aspect copies from DS formats are tightly packed. Experimentally this is also true on Metal, but @litherum could you please check directly with the Metal team?

austinEng · 2020-08-14T17:15:51Z

spec/index.bs

+  - If |textureCopyView|.{{GPUTextureCopyView/texture}}.{{GPUTexture/[[format]]}}
+    has both depth and stencil aspects,
+    |textureCopyView|.{{GPUTextureCopyView/aspect}} must be
+    {{GPUTextureAspect/"depth-only"}} or {{GPUTextureAspect/"stencil-only"}}.


"all" should be allowed here for the T2T case
We should only validate the if "depth-only" or "stencil-only" is specified, that the texture actually has a depth or stencil aspect, respectively.

Based on #997 we also need validation that the entire subresource is selected for depth or stencil textures (with OutputAttachment usage TBD..)

We should move the "depth-only" and "stencil-only" part to validation of T2B, B2T and WriteTexture. Probably "all" should be allowed on depth32float? So have some language that the GPUTextureCopyView.aspect "resolves to" a single aspect.

And put the "all" restriction for T2T copies. maybe saying that GPUTextureCopyView.aspect "resolves to" all the texture aspects

kainino0x · 2020-08-14T17:31:00Z

The documentation and conclusion about D3D12 is here:
#652 (comment)
That issue is now assigned to Dean/Myles to figure out the answer on Metal

kainino0x · 2020-10-01T02:59:37Z

With #652 resolved I need to pick this back up again.

Per discussions on gpuweb#652. It was removed in gpuweb#143.

- Cannot the depth aspect in copyBufferToTexture - Must copy all aspects in copyTextureToTexture - Must copy a single aspect in copies between buffers/textures - Must copy the whole subresource for any copy texture command if the format is a depth-stencil format or sample count > 1

kdashg · 2020-10-21T20:41:03Z

spec/index.bs

                - |source|.{{GPUBufferCopyView/buffer}}.{{GPUBuffer/[[usage]]}} contains
                    {{GPUBufferUsage/COPY_SRC}}.
-                - [$validating GPUTextureCopyView$](|destination|) returns `true`.
+                - [$validating GPUTextureCopyView$](|destination|, |copySize|) returns `true`.


Are these copySize additions from another cset?

I could move this into a different PR. The existing validation only checked the whole-subresource-copied restriction for depth-stencil and multisampled textures on T2T copies, but in D3D12 the restriction is on T2T, T2B, and B2T.

For multisampled this was because T2B and B2T can't do multisampled at all. But I think it's okay to be slightly redundant.

kainino0x · 2020-10-21T20:42:36Z

Resolution: Make sure we write down somewhere it doesn't work with copies to/from depth24plus or copies to depth32float. Otherwise should be accepted

kvark · 2020-10-21T23:03:44Z

Is https://gpuweb.github.io/gpuweb/#depth-formats not sufficient to indicate what copies are valid?

kainino0x · 2020-10-21T23:10:04Z

I think it is, I just didn't know offhand during the meeting whether we had that already.

spec/index.bs

kainino0x

LGTM

kainino0x · 2020-10-23T02:08:52Z

spec/index.bs

                - |source|.{{GPUBufferCopyView/buffer}}.{{GPUBuffer/[[usage]]}} contains
                    {{GPUBufferUsage/COPY_SRC}}.
-                - [$validating GPUTextureCopyView$](|destination|) returns `true`.
+                - [$validating GPUTextureCopyView$](|destination|, |copySize|) returns `true`.


For multisampled this was because T2B and B2T can't do multisampled at all. But I think it's okay to be slightly redundant.

kainino0x · 2020-10-29T19:09:54Z

spec/index.bs

+                - If |source|.{{GPUTextureCopyView/texture}}.{{GPUTexture/[[format]]}} is a depth-stencil format:
+                    - |source|.{{GPUTextureCopyView/aspect}} must refer to a single copyable aspect of
+                        |source|.{{GPUTextureCopyView/texture}}.{{GPUTexture/[[format]]}}.
+                        See [[#depth-formats|depth-formats]].


As a followup, let's consider deduplicating this validation into a common place for B2T/T2B/writeTexture.

kainino0x · 2020-10-29T19:10:19Z

Merging with the agreement from the meeting

…puweb#873) This also makes the texture1d size tests avoid depth-stencil formats because they are not valid for 1D textures. Also improves the coverage of mipLevelCount validation a bit.

kvark reviewed Jun 22, 2020

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

kainino0x commented Jun 22, 2020

View reviewed changes

kvark changed the base branch from master to main June 23, 2020 13:13

litherum reviewed Jul 6, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

kvark reviewed Jul 6, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

kainino0x self-assigned this Jul 17, 2020

kainino0x marked this pull request as draft July 17, 2020 18:29

austinEng reviewed Aug 14, 2020

View reviewed changes

kainino0x mentioned this pull request Sep 17, 2020

Copying of depth24plus formats #652

Closed

kainino0x and others added 2 commits October 21, 2020 12:48

Add aspect back to GPUTextureCopyView

fb3de3f

Per discussions on gpuweb#652. It was removed in gpuweb#143.

kainino0x force-pushed the texturecopyview-aspect branch from 10a919d to 56ccdc9 Compare October 21, 2020 17:56

kainino0x marked this pull request as ready for review October 21, 2020 20:40

kdashg reviewed Oct 21, 2020

View reviewed changes

kvark approved these changes Oct 21, 2020

View reviewed changes

kainino0x commented Oct 22, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

Refer to depth-formats table for copy capabilities

1880d1f

austinEng force-pushed the texturecopyview-aspect branch from 42d31bf to 1880d1f Compare October 22, 2020 21:05

kainino0x commented Oct 23, 2020

View reviewed changes

Fix GPUTextureFormat/[[format]] -> GPUTexture/[[format]]

2b25ea2

kainino0x commented Oct 29, 2020

View reviewed changes

kainino0x merged commit e3dde36 into gpuweb:main Oct 29, 2020

kainino0x mentioned this pull request Oct 29, 2020

Consider deduplicating "must be a single aspect" validation #1192

Closed

kainino0x deleted the texturecopyview-aspect branch October 29, 2020 19:13

kainino0x assigned austinEng and unassigned kainino0x Oct 29, 2020

austinEng mentioned this pull request Sep 2, 2021

Copy Validation of Depth/Stencil textures #997

Closed

Add aspect back to GPUTextureCopyView #873

Add aspect back to GPUTextureCopyView #873

Uh oh!

Conversation

kainino0x commented Jun 20, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kainino0x Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Jun 22, 2020

Uh oh!

Uh oh!

Uh oh!

kainino0x commented Jul 6, 2020

Uh oh!

Kangz commented Aug 14, 2020

Uh oh!

austinEng left a comment

Choose a reason for hiding this comment

Uh oh!

austinEng Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Aug 14, 2020

Uh oh!

kainino0x commented Oct 1, 2020

Uh oh!

kdashg Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

austinEng Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Oct 21, 2020

Uh oh!

kvark commented Oct 21, 2020

Uh oh!

kainino0x commented Oct 21, 2020

Uh oh!

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kainino0x commented Jun 20, 2020 •

edited by pr-preview bot

Loading