Sitelet https://github.com/gpuweb/gpuweb/pull/519
Skip to content

imageHeight -> imagePitch - #519

Closed
kainino0x wants to merge 1 commit into
gpuweb:masterfrom
kainino0x:zPitch
Closed

kainino0x wants to merge 1 commit into
gpuweb:masterfrom
kainino0x:zPitch

Conversation

@kainino0x

@kainino0x kainino0x commented Dec 14, 2019 •

Copy link
Copy Markdown
Contributor

Fixes #148


Preview | Diff

Comment thread spec/index.bs Outdated
GPUBufferSize offset = 0;
required unsigned long rowPitch;
required unsigned long imageHeight;
required unsigned long zPitch; // in units of rowPitch bytes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Basically imagePitchInBytes is zPitch * rowPitch? I think imageHeight is more clear, but maybe because I'm more used to it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. My problem is 'imageHeight' sounds like the height of some image, and it really isn't at all.

@kvark kvark Dec 16, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could be more to the point, like rowsPerDepthSlice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a little imprecise because not all of those rows are necessarily in the depth slice. But it does use "depth" instead of "z" which matches elsewhere (copySize.depth) better.

@kainino0x kainino0x Dec 16, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... to go in the opposite direction, maybe it would be better to have copySize have z and layer be separate concepts (and either "depth" or "layerCount" must be 1).

I don't have any clue whether 3d array textures would ever be a thing, but maybe we should design the api to allow them and totally separate those two "depth" concepts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most APIs have features to convert 2D array layers to 3D texture views and vice-versa so I think it is safe to assume that depth and layers are the same thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kainino0x
3d array textures might sound unreal, but there definitely are cube arrays

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How should cubemaps be specified? Does the 6 go into layer count or depth?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right now, it's the layer count.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see also #520.

@kainino0x

Copy link
Copy Markdown
Contributor Author

Alternate proposal from #148 that I'd be happy to use: imagePitch (i.e. pitch between "images" where images are either z-slices of a 3d texture or layers of a 2d array texture).

@kainino0x

Copy link
Copy Markdown
Contributor Author

The "image" naming links it better to the way other APIs name this concept.

@Kangz

Kangz commented Jan 6, 2020

Copy link
Copy Markdown
Contributor

This was discussed in the 2020-01-06 meeting

@kainino0x

Copy link
Copy Markdown
Contributor Author

I've updated to imagePitch in hopes it's something people can agree on.

@JusSn JusSn left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pitch: TECHNICAL
"the distance between successive corresponding points or lines, e.g., between the teeth of a cogwheel."

Especially if we are already using "rowPitch", "imagePitch" makes sense to me. I was under the impression the underlying APIs shared some terminology, but nope.

@kainino0x kainino0x changed the title imageHeight -> zPitch imageHeight -> imagePitch Feb 3, 2020
@kainino0x

kainino0x commented Feb 3, 2020 •

Copy link
Copy Markdown
Contributor Author

We'll talk about this at the F2F. I think we're fairly happy with the names. However, we'd like to figure out what the units should be:

  • bytes?
  • texels?
  • row pitches?
row pitch image pitch
Metal sourceBytesPerRow
Bytes
Zero probably invalid
Must be a multiple of the texel size
Must be <= 32767 texels
sourceBytesPerImage
Bytes
Zero probably invalid
Must be a multiple of row pitches
Must be >= 512 bytes? (#537)
Vulkan bufferRowLength
Texels
Zero means tightly packed
bufferImageHeight
Texels
Zero means tightly packed
D3D12 CopyTextureRegion
(D3D12_SUBRESOURCE_FOOTPRINT)
Footprint.RowPitch
Bytes
Zero not valid (must be >= size of row)
Must be a multiple of 256 bytes
Footprint.Height
Row pitches
Therefore must be a multiple of row pitches
D3D11 Requires draw or compute call? -
GLES ROW_LENGTH
Texels
Zero means tightly packed
IMAGE_HEIGHT
Texels
Zero means tightly packed

We should be sure to understand how this works with compressed textures.

@kainino0x

kainino0x commented Feb 12, 2020 •

Copy link
Copy Markdown
Contributor Author

Discussed at F2F. To be resolved by spec editors based on the input there.

@JusSn

JusSn commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

new preference: "bytesPerRow" and "rowsPerImage"

@kainino0x

Copy link
Copy Markdown
Contributor Author

And, per F2F decisions, the field becomes optional / used only for 3d. I think I'll open a new PR for all of it together.

@kainino0x kainino0x closed this Mar 9, 2020
@kainino0x
kainino0x deleted the zPitch branch March 9, 2020 22:23
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* depth related tests

* fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider renaming BufferCopyView.imageHeight

5 participants