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

Spec optional conversion to RGB in VideoFrame.copyTo() #754

Merged
merged 12 commits into from
Apr 19, 2024
Merged

Conversation

Djuffin
Copy link
Contributor

@Djuffin Djuffin commented Dec 7, 2023

This change should address the issue of pixel format conversion in VideoFrame.copyTo().


Preview | Diff

@Djuffin Djuffin requested review from aboba and padenot December 7, 2023 07:20
@Djuffin
Copy link
Contributor Author

Djuffin commented Dec 7, 2023

There are probably some missing details, typos and omissions.
But I'd love to get some feedback on the general approach in this change.

@Djuffin Djuffin self-assigned this Dec 7, 2023
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@Djuffin
Copy link
Contributor Author

Djuffin commented Feb 29, 2024

@padenot Could you please take a look? I'd love to merge this PR some time soon. We've talked about it at TPAC last September.

index.src.html Outdated
@@ -4179,10 +4205,36 @@
[=combined buffer layout/allocationSize=].
9. Return |combinedLayout|.

: <dfn for=VideoFrame>Convert to RGB frame</dfn> (with |format|, |rect| and |colorSpace|)
:: 1. Let |canvas| be a new {{OffscreenCanvas}} constructed with
|rect|.|width| and |rect|.|height|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't something that we can do in spec, using other specs directly like that. All the objects you use can have been overridden in the global this algorithm is running in.

We should use internal stops for canvas instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably missing something here. Could you elaborate a bit?

Isn't it what we do in the Rendering section?

referring to CanvasDrawImage drawImage()

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, that's unrelated, we reference it in your link. What happens when drawImage is called with a VideoFrame is well-defined there. Here you're using another Web Platform object from the innards of our spec.

e.g. what happens if in the document we're doing this conversion, I've maliciously done:

window.OffscreenCanvas.prototype = function() { alert("what now"); };

before anything from our spec run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This algorithm does not refer to any particular js code. It explicitly says what object needs to be created by the user agent. That's why I don't think that global overrides should concern us here. With a similar approach half of this spec can be put into question with code like

window.VideoFrame.prototype = function() { alert("what now"); };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note clarifying that UAs don't have to do it via literally creating an OffscreenCanvas as long as the result is the same. I hope this eliminates possible confusion.

If you still think that the wording leaves room for misinterpretation, I'm very much open to suggestions how to rewrite it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the links!

I've noticed that drawImage() (step 6) does not specify any details about the rendering (sampling, color conversion etc). It's only saying that "The user agent may use any filtering algorithm".
So there isn't really an algorithm to extract and call.
I will have to rewrite it in a more abstract manner, similar to the text for drawImage()

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add an export to something hand-wavy from canvas now, and merge what we want to do here, and then fix the canvas spec as well, your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked HTML spec owners about the best way to reuse canvas pixel format conversion algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've looked at the rest of the WebCodecs spec. It's pretty light on the details when it comes to format and color spaces conversions. (for example Rendering and AudioData.copyTo )

So I rewrote "Convert to RGB frame" in the similar manner.
Please take another look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@padenot gentle ping

index.src.html Outdated Show resolved Hide resolved
@Djuffin Djuffin requested a review from chrisn February 29, 2024 16:59
Copy link
Member

@chrisn chrisn left a comment

Choose a reason for hiding this comment

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

Looks good, but let's resolve the discussion on overridden globals (I don't mind if that's here or in a follow up PR).

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Show resolved Hide resolved
index.src.html Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@Djuffin Djuffin requested a review from padenot April 16, 2024 09:16
@Djuffin
Copy link
Contributor Author

Djuffin commented Apr 18, 2024

@padenot PTAL

Copy link
Collaborator

@padenot padenot left a comment

Choose a reason for hiding this comment

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

All good, thanks!

@Djuffin
Copy link
Contributor Author

Djuffin commented Apr 18, 2024

@aboba please take a look and merge if you don't have new comments.

@aboba aboba merged commit 48523cd into w3c:main Apr 19, 2024
17 checks passed
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Apr 19, 2024
SHA: 48523cd
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 10, 2024
…ayback-reviewers,smaug,padenot

This patch updates the `VideoFrameCopyToOptions` webidl to meet the
changes in WebCodecs PR 754 [1].

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213805
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 10, 2024
…viewers,padenot

This patch updates the `ParseVideoFrameCopyToOptions` to the changes
made in WebCodecs PR 754 [1]. The changes make the caller of
`ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the
given `VideoFrameCopyToOptions`'s format is invalid.

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213806
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jul 12, 2024
…ayback-reviewers,smaug,padenot

This patch updates the `VideoFrameCopyToOptions` webidl to meet the
changes in WebCodecs PR 754 [1].

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213805
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jul 12, 2024
…viewers,padenot

This patch updates the `ParseVideoFrameCopyToOptions` to the changes
made in WebCodecs PR 754 [1]. The changes make the caller of
`ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the
given `VideoFrameCopyToOptions`'s format is invalid.

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213806
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jul 13, 2024
…ayback-reviewers,smaug,padenot

This patch updates the `VideoFrameCopyToOptions` webidl to meet the
changes in WebCodecs PR 754 [1].

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213805
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jul 13, 2024
…viewers,padenot

This patch updates the `ParseVideoFrameCopyToOptions` to the changes
made in WebCodecs PR 754 [1]. The changes make the caller of
`ParseVideoFrameCopyToOptions` throw a `NotSupported` error when the
given `VideoFrameCopyToOptions`'s format is invalid.

[1] w3c/webcodecs#754

Differential Revision: https://phabricator.services.mozilla.com/D213806
@Djuffin Djuffin deleted the rgb branch September 10, 2024 23:41
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.

6 participants