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

Add VideoFrameBufferInit.transfer #676

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Add VideoFrameBufferInit.transfer #676

merged 1 commit into from
Jun 8, 2023

Conversation

Djuffin
Copy link
Contributor

@Djuffin Djuffin commented May 18, 2023

This should allow to avoid extra copies while constructing VideoFrame from pixel data

Addressing: #104


Preview | Diff

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
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.

Looks good, the prose needs a bit of fixing.

We should also be doing the same for the audio side, not only because we've strived to make the API all symmetric (audio / video) and mirrored (encoding / decoding), but also because this will save a lot of small and very frequent allocations: while you might be doing 30-60 allocations per second for a video scenario (in addition to horribly large copies), you can be doing hundreds of allocations per second on the audio side: imagine an AudioWorkletProcessing feeding an AudioEncoder, buffers are 128-frames long, sample-rate is e.g. 48000Hz, we're looking at 375 buffer allocations (and small copies, that's less problematic) per second. We can then implement #212 (comment), and make it, so there is no allocation after some time, by naturally pooling the buffers. Authors that don't mind allocation can simply not specify the parameters and/or the transfer list.

Unrelated, please don't mix functional and non-functional (formatting / refactoring) changes in the same commit, it makes things unnecessarily hard to review and mistakes eventually slip through because of this.

In general, it's a lot better to use regular numbering in markdown, because it's not rare that a step references another step (jumps or complex conditionals), and having the number makes it easier to notice that things need to be offset when inserting or deleting steps of an algorithm. This can be fixed by adding explicit labels to jump to, but we haven't done that.

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
1. Let |resource| be a new [=media resource=] referencing pixels in |data|
without a copy.
Use {{VideoFrameBufferInit/visibleRect}} and {{VideoFrameBufferInit/layout}}
to determine where in |data| the pixels for each plane reside.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very hand-wavy, is there an algorithm to link to? Or maybe it just follows from the rest of the prose and can be omitted.

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 is the same wording that we have in the existing spec for copying.

Copy link
Contributor Author

@Djuffin Djuffin May 30, 2023

Choose a reason for hiding this comment

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

I dropped it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if implementations were allowed to copy. In general that's not observable here (unless we add buffer return), but while copying Chrome does try to minimize the coded size and that would be visible if it were not allowed on this path. It also allows us to shift the visible offset to the origin, which is more compatible; not all code is written to handle offsets (because they are rare in practice).

Chrome also expands the Y plane for odd-sized frames so that the coded size is always even. At some point in the future we may trust all of our processing paths enough to remove that, but for now we are likely to prefer to copy in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's preferable to be explicit, since it's opt-in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be specific, if one specifies a transfer list, I'd rather have it throw if it can't be transferred, rather than copied, but maybe we want it to really the same as other transfers on the platform.

If we chose to fall back to a copy, it's best to explicitly fall back to the other step, and not copy/paste a bit of prose.

I transfer succeeds, this bit of prose is not useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want to throw in edge cases like this, especially the odd-size one which I expect to go away in the future. I don't think developers should need to be aware that transfers can fail and always have to implement fallbacks; I can't imagine the fallback would ever be anything other than trying again without the transfer.

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 made transferring optional. Please take a look.

index.src.html Show resolved Hide resolved
index.src.html Outdated
1. Let |resource| be a new [=media resource=] referencing pixels in |data|
without a copy.
Use {{VideoFrameBufferInit/visibleRect}} and {{VideoFrameBufferInit/layout}}
to determine where in |data| the pixels for each plane reside.
Copy link
Contributor Author

@Djuffin Djuffin May 30, 2023

Choose a reason for hiding this comment

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

I dropped it.

index.src.html Outdated Show resolved Hide resolved
@Djuffin
Copy link
Contributor Author

Djuffin commented Jun 2, 2023

I updated the text according to the feedback and WG comments. PTAL

@Djuffin
Copy link
Contributor Author

Djuffin commented Jun 6, 2023

@youennf Could you please take another look and approve if you don't have any more comments?

@dalecurtis dalecurtis merged commit a7ea132 into w3c:main Jun 8, 2023
@Djuffin Djuffin deleted the transfer branch June 21, 2023 22:27
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