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

Consider putting most of the writeTexture validation on the device timeline. #2680

Closed
Kangz opened this issue Mar 22, 2022 · 5 comments
Closed
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.)
Projects
Milestone

Comments

@Kangz
Copy link
Contributor

Kangz commented Mar 22, 2022

Right now GPUQueue.writeTexture runs validating linear layout on the content timeline and raises an exception if it fails. There doesn't seem to be a strong reason why that's the case, and this is duplicating validation code in browsers to be both in the GPU process and the content process. We should run that validation on the device timeline instead.

@Kangz Kangz added this to the V1.0 milestone Mar 22, 2022
@kainino0x kainino0x added this to Needs Discussion in Main Mar 29, 2022
@kainino0x
Copy link
Contributor

FTR, this was changed in #832.

It's technically possible to move this validation to the device timeline, but in actual practice the content timeline (content process) must always do similar computations in order to figure out how much data (from dataBytes) to send to the device timeline (GPU process). Spec-wise, we can either:

  • leave it as is (editorially, we need to actually write down the fact that it's computing the number of bytes to copy)
  • write the spec as if it copies the entire source buffer to the device timeline, and then the device timeline does validation and extracts the right part. But non-normatively, actual implementations will have to diverge from this because they can't just copy the whole source buffer (which may be the WASM heap), so must compute either the exact number of bytes needed for the copy, or conservatively overestimate it.

The specific reason people run into this is that the dataLayout parameter contains an offset into the BufferSource which allows using this entry point without creating a view with the correct offset. If we didn't have that, maybe we could either:

  • get away with copying the entire BufferSource (issuing a warning if it's way too large), because users would have to create subarrays anyway and could specify a length.
  • add a required dataLength parameter which tells us how much to copy.

cc @Richard-Yunchao who recently did work on this

@Kangz
Copy link
Contributor Author

Kangz commented Mar 30, 2022

IMHO it's weird that the spec has somewhat complex content-side validation for just this method and not the others. It doesn't help implementations (since there needs to be duplicated validation), nor developers (this one call is different and can just throw, they get the error quicker but that's not that much more of an improvement). For the spec we don't need to say much since under the as-if rule, browsers are free to optimize this.

IMHO again, there is no perfect solution for handling the offset of writeTexture, so leaving it as is is ok.

For reference this pseudo code should compute exactly the data needed for valid calls to writeTexture. It is some computations but fairly straightforward.

BlockSizeData blockData = GetBlockSizeDataFor(dest.format, aspect);
if (!blockData.valid) {return 0;} // No need to send data, it is a validation error anyway

GPUExtent3D sizeInBlocks = copySize / blockData.blockSize;
// Maybe sizeInBlocks = max(sizeInBlocks, {1, 1, 1});

size_t blocksPerRow = dataLayout.bytesPerRow ?
      bytesPerRow / blockSize.bytesPerBlock :
      sizeInBlocks.width;
size_t rowsPerImage = dataLayout.rowsPerImage ?? sizeInBlocks.height;

// No need for overflow checks! That's done in the device timeline
size_t blockCount =
    sizeInBlocks.width +
    blocksPerRow * (
        sizeInBlocks.height - 1 +
        rowsPerImage * (sizeInBlocks.depth - 1));

return blockCount * blockData.bytesPerBlock;

@kainino0x
Copy link
Contributor

OK, I think moving it to the device timeline is fine.

I think your pseudocode needs to get slightly more complex, since it needs to handle all valid cases correctly including zero-sized copies (which are generally valid but we shouldn't no-op because other validation is still supposed to run).

@kainino0x kainino0x added the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label Apr 6, 2022
@Kangz Kangz moved this from Needs Discussion to Needs Specification in Main Apr 19, 2022
@Kangz Kangz removed the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label Apr 19, 2022
@Kangz
Copy link
Contributor Author

Kangz commented Apr 19, 2022

There's been agreement to proceed with this. Moving to needs-spec.

@kainino0x kainino0x added the copyediting Pure editorial stuff (copyediting, bikeshed, etc.) label Apr 25, 2022
@kainino0x
Copy link
Contributor

kainino0x commented Jul 14, 2022

This ended up getting fixed in #2702 (and #2847).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.)
Projects
No open projects
Main
Needs Specification
Development

No branches or pull requests

2 participants