Skip to content

fix: Use the display size of VideoFrames in texture.write() - #3224

Open
RaphaelFakhri wants to merge 3 commits into
software-mansion:mainfrom
RaphaelFakhri:fix/videoframe-display-size
Open

RaphaelFakhri wants to merge 3 commits into
software-mansion:mainfrom
RaphaelFakhri:fix/videoframe-display-size

Conversation

@RaphaelFakhri

Copy link
Copy Markdown
Contributor

Fixes #3213.

texture.write(videoFrame) read the frame size from codedWidth/codedHeight. Those are the decoder's padded buffer dimensions, so a 320x240 H.264 frame can report a coded size of 320x258 and the write fails the size check against a 320x240 texture.

This change reads displayWidth/displayHeight instead, which match the region that copyExternalImageToTexture copies.

Changes

  • imageSize() in packages/typegpu/src/core/texture/textureWrite.ts uses displayWidth/displayHeight for VideoFrame sources.
  • New test in packages/typegpu/tests/texture.test.ts writes a mock frame with a 320x258 coded size and a 320x240 display size into a 320x240 texture and checks the copy size. Without the fix it throws Texture write source size 320x258 does not match target size 320x240.

Testing

  • pnpm vitest run --project=!browser packages/typegpu/tests/texture.test.ts: 54 passed.
  • oxlint --type-aware, oxfmt --check, and tsc for packages/typegpu are clean.

Copilot AI balanced review requested due to automatic review settings October 20, 2026 09:26

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@pullfrog

pullfrog Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

✅ No new issues found.

Reviewed changes

This PR fixes texture.write(videoFrame) reading the decoder's padded codedWidth/codedHeight instead of the visible displayWidth/displayHeight, which caused a spurious size-mismatch throw whenever a frame's coded size was macroblock-padded.

  • imageSize() uses the display size for VideoFrame sources — packages/typegpu/src/core/texture/textureWrite.ts:88 now returns displayWidth/displayHeight. The WebGPU spec's external-source-dimensions table defines VideoFrame source dimensions as VideoFrame.displayWidth/displayHeight, so the resolveImageWrite size check now lines up with the region copyExternalImageToTexture actually copies.
  • Regression test added — packages/typegpu/tests/texture.test.ts:775 writes a mock frame with a 320x258 coded size and a 320x240 display size into a 320x240 texture and asserts the copy size [320, 240].

I confirmed the test genuinely pins the fix: reverting imageSize() to codedWidth/codedHeight makes the new test fail with the exact error from #3213 (Texture write source size 320x258 does not match target size 320x240). The targeted suite passes (54/54) and the working tree is clean. The branch ordering in imageSize() is unaffected, since a VideoFrame exposes neither videoWidth, naturalWidth, nor width/height.

( ◦°^°◦) 🏴☠️ Aye, I read the whole diff, checked the WebGPU spec to be sure displayWidth be the right measure, ran the new test (she passes), then scuttled the fix to prove the test catches the old bug — and put it back. No issues found, matey.

Pullfrog  | View workflow run | via Pullfrog | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

This PR fixes texture.write(videoFrame) reading the decoder's padded codedWidth/codedHeight instead of the visible displayWidth/displayHeight, which caused a spurious size-mismatch throw whenever a frame's coded size was macroblock-padded.

  • imageSize() uses the display size for VideoFrame sources — packages/typegpu/src/core/texture/textureWrite.ts:88 now returns displayWidth/displayHeight. The WebGPU spec's external-source-dimensions table defines VideoFrame source dimensions as VideoFrame.displayWidth/displayHeight, so the resolveImageWrite size check now lines up with the region copyExternalImageToTexture actually copies.
  • Regression test added — packages/typegpu/tests/texture.test.ts:775 writes a mock frame with a 320x258 coded size and a 320x240 display size into a 320x240 texture and asserts the copy size [320, 240].

I confirmed the test genuinely pins the fix: reverting imageSize() to codedWidth/codedHeight makes the new test fail with the exact error from #3213 (Texture write source size 320x258 does not match target size 320x240). The targeted suite passes (54/54) and the working tree is clean. The branch ordering in imageSize() is unaffected, since a VideoFrame exposes neither videoWidth, naturalWidth, nor width/height.

( ◦°^°◦) 🏴☠️ Aye, I read the whole diff, checked the WebGPU spec to be sure displayWidth be the right measure, ran the new test (she passes), then scuttled the fix to prove the test catches the old bug — and put it back. No issues found, matey.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@vende11s vende11s left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Just a small suggestion to improve the test

Comment thread packages/typegpu/tests/texture.test.ts Outdated
.$usage('render');

const mockFrame = {
codedWidth: 320,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
codedWidth: 320,
codedWidth: 336,

Could we set codedWidth to something different from displayWidth? This would catch regressions in both dimensions.

Co-authored-by: Michał Wierzbiński <74505994+vende11s@users.noreply.github.com>
@RaphaelFakhri

Copy link
Copy Markdown
Contributor Author

@vende11s Applied the suggestion in c7fb86c: the mock frame now has a 336x258 coded size and a 320x240 display size, so the test covers both dimensions.

npx vitest run --project=!browser packages/typegpu/tests/texture.test.ts passes (54 passed). With imageSize() reverted to codedWidth/codedHeight, the test fails with Texture write source size 336x258 does not match target size 320x240. If only the height uses the display size, it also fails (336x240).

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

This run reviewed the delta since the prior pullfrog review at 0f97455: a single test-only commit that applies the human reviewer's suggestion. No production code changed, so the fix validated in the prior review stands.

  • Regression test now distinguishes both dimensions — packages/typegpu/tests/texture.test.ts:784 sets codedWidth to 336 (previously 320, equal to displayWidth), so the test now catches a regression that read the coded width as well as one that read the coded height.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
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.

texture.write(VideoFrame) uses the padded coded size instead of the visible size

3 participants