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

https://w3c.github.io/webcodecs/#dom-videoframe-videoframe-data-init should validate the plane layout according the coded width and height #702

Open
youennf opened this issue Jul 7, 2023 · 2 comments
Assignees

Comments

@youennf
Copy link
Contributor

youennf commented Jul 7, 2023

https://w3c.github.io/webcodecs/#dom-videoframe-videoframe-data-init step 9 is computing the plane layout according parsedRect which is computed from visibleWidth/visibleHeight.
It seems codedWidth and codedHeight should be used instead, which would be consistent with checking whether the buffer is large enough.
See also https://bugs.webkit.org/show_bug.cgi?id=256848.

@youennf
Copy link
Contributor Author

youennf commented Jul 7, 2023

webkit-commit-queue pushed a commit to youennf/WebKit that referenced this issue Jul 7, 2023
…ct is present

https://bugs.webkit.org/show_bug.cgi?id=256848
rdar://109724698

Reviewed by Eric Carlson.

Use codedWidth and codedHeight instead of parsedRect.
This is coherent with the rest of the algorithm like checking the array size and is consistent with what Chrome does.
I filed w3c/webcodecs#702 to update the spec.

* LayoutTests/http/wpt/webcodecs/videoFrame-with-stride-expected.txt: Added.
* LayoutTests/http/wpt/webcodecs/videoFrame-with-stride.html: Added.
* Source/WebCore/Modules/webcodecs/WebCodecsVideoFrame.cpp:
(WebCore::WebCodecsVideoFrame::create):

Canonical link: https://commits.webkit.org/265844@main
@sandersdan
Copy link
Contributor

sandersdan commented Jul 10, 2023

Agreed, combinedLayout is only used to run the check in step 11, which should be making sure that the buffer is large enough to contain the entire coded region of the frame. It's not necessary to compute anything related to the visible region here, but Compute Layout and Allocation Size algorithm is shared with copyTo() which does.

The complexity comes from having to support explicit and implicit layouts. The actual check just needs to ensure that the buffer contains the full planeCodedHeight * planeStride bytes for every plane.

It could be worth making this more obvious because using planeCodedWidth * planeSampleBytes for the last row instead of planeStride might be logical but would differ from the spec intention.

mnutt pushed a commit to movableink/webkit that referenced this issue Jul 18, 2023
…ct is present

https://bugs.webkit.org/show_bug.cgi?id=256848
rdar://109724698

Reviewed by Eric Carlson.

Use codedWidth and codedHeight instead of parsedRect.
This is coherent with the rest of the algorithm like checking the array size and is consistent with what Chrome does.
I filed w3c/webcodecs#702 to update the spec.

* LayoutTests/http/wpt/webcodecs/videoFrame-with-stride-expected.txt: Added.
* LayoutTests/http/wpt/webcodecs/videoFrame-with-stride.html: Added.
* Source/WebCore/Modules/webcodecs/WebCodecsVideoFrame.cpp:
(WebCore::WebCodecsVideoFrame::create):

Canonical link: https://commits.webkit.org/265844@main
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

No branches or pull requests

2 participants