Skip to content

Add refcounting semantics to VideoFrame #167

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

Merged
merged 15 commits into from
May 5, 2021

Conversation

chcunningham
Copy link
Collaborator

@chcunningham chcunningham commented Apr 5, 2021

Making it symmetric w/ AudioFrame (PR #162)

Adds clone/close
Adds [[resource reference]]
Updates constructors accordingly (and fix cruft/obsolete steps).
Removes destroy (replaced by close).

Fixes #129
Notes issues #165 and #166 for follow up.


Preview | Diff

Making it symmetric w/ AudioFrame.
Adds clone/close
Adds [[resource reference]]
Updates constructors accordingly (and fix cruft/obsolete steps).
Removes destroy (replaced by close).

Fixes #129 (in comboniation w/ PR #162).
Notes issues #165 and #166 for follow up.
@chcunningham chcunningham changed the base branch from main to audioframe_clone_close April 5, 2021 06:52
@chcunningham chcunningham requested review from aboba and padenot April 5, 2021 06:54
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.

This needs clarification about ownership, and other fixes, but in general is what we need.

Copy link
Collaborator Author

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

Thanks @padenot and @aboba ! Nearly everything addressed.

@chcunningham
Copy link
Collaborator Author

@padenot , similar to the audio issue, I think we at least have agreement on the API shape and its intended meaning. Would you mind approving the merge here and doing follow up issues for any lingering wording/style concern?

@chcunningham
Copy link
Collaborator Author

@padenot , similar to the audio issue, I think we at least have agreement on the API shape and its intended meaning. Would you mind approving the merge here and doing follow up issues for any lingering wording/style concern?

@padenot friendly ping.

Replaces the ImageBitmap constructor with more generic constructor for
CanvasImageSource. VideoFrame is itself a CanvasImageSource.

Fixes #158
Copy link
Collaborator Author

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

Thanks @padenot - I'll send a fix for the nit. Otherwise, we've agreed to merge and handle remainder in follow ups.

Copy link
Collaborator Author

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

Thanks, all comments addressed

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.

Good for merging now, thanks!

@chcunningham
Copy link
Collaborator Author

This one depends on #162, so I'll merge it as soon as that one goes in.

@chcunningham chcunningham merged commit 72db49c into audioframe_clone_close May 5, 2021
@chcunningham
Copy link
Collaborator Author

I'll merge back to main manually.

@Djuffin Djuffin deleted the videoframe_clone_close branch October 27, 2023 23:21
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.

Clarify VideoFrame|AudioFrame clone/close/transfer behavior
4 participants