Skip to content

Conversation

chcunningham
Copy link
Collaborator

@chcunningham chcunningham commented Aug 25, 2021

Fixes #232.

Also, I've added validation of copyTo options to both allocationSize() and
copyTo() (previously only the latter), such that any call to
allocationSize() that succeeds should also succeed with copyTo().


Preview | Diff

Fixes #232.

Also, I've added validation of copyTo options to both allocationSize() and
copyTo() (previously only the latter), such that any call to
allocationSize() that succeeds should also succeed with copyTo().
@chcunningham chcunningham requested review from padenot and aboba August 25, 2021 06:33
@chcunningham
Copy link
Collaborator Author

@tguilbert-google @dalecurtis FYI

@chcunningham
Copy link
Collaborator Author

@padenot please give priority to reviewing the API shape / behavior, as we'd like to launch with this in Chrome v1.

In a follow up, I imagine we'll want to say more about how conversion takes place. Looking at Chrome's impl for just the float32 conversion (e.g. here's planaru8 to float32), this seems like a potentially dense matrix of info. Lets chat about to spec this consicely.

@chcunningham
Copy link
Collaborator Author

Editors Call: Paul to take a closer look, generally looks right. Follow up PR for conversion can simply use min/max/zero from AudioSampleFormat paramaterized steps.

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 overall. I'm not sure if only mandating a single conversion is worth it, it's simple enough to do all of them.

We need to follow up with less hand-wavy conversion steps. As mentioned, the min/max/bias are already specced so there's not a lot to do.

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!

Re: support mandate, Chrome's support is currently limited to just conversion to f32 planar. Basically a detail of the longstanding audio pipeline in chrome, which does all mixing in f32. I agree we could implement more. Do you think conversions to other formats beyond f32 planar will useful?

@chcunningham
Copy link
Collaborator Author

Thanks!

Re: support mandate, Chrome's support is currently limited to just conversion to f32 planar. Basically a detail of the longstanding audio pipeline in chrome, which does all mixing in f32. I agree we could implement more. Do you think conversions to other formats beyond f32 planar will useful?

Lets go ahead and land this and follow up in #352

@chcunningham chcunningham merged commit 9dc3359 into main Sep 8, 2021
@chcunningham chcunningham deleted the audio_copyto_convert branch September 8, 2021 00:01
github-actions bot added a commit that referenced this pull request Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

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 Sep 8, 2021
SHA: 9dc3359
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Converting AudioSampleFormats for rendering
2 participants