Rename AudioFrame->AudioData, drop AudioBuffer, add ref counting semantics. #162

chcunningham · 2021-04-02T23:46:40Z

Partially addresses #129. I'll send a follow up shortly to do the same for VideoFrame (will land at the same time, but trying to keep the diffs in my PRs small for easy review).

Fixes #168 (Rename AudioFrame -> AudioData).
Progresses #179 (Drop dependency on AudioBuffer).
Progresses #184 (Back attributes with slots).

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 · 2021-04-05T07:20:23Z

Some context for usage of "acquire the content" in https://github.com/WebAudio/web-audio-api-v2/issues/119#issuecomment-812238655

FYI @hoch @rtoy

padenot

What we need again, but a few important questions.

index.src.html

chcunningham

Thanks @padenot @aboba

index.src.html

chcunningham · 2021-04-15T05:36:29Z

@padenot @aboba I think we at least have agreement on the API shape and its intended meaning. Would you mind approving the merge here and we can keep pushing on wording/style stuff in follow up GH issues?

chcunningham · 2021-04-19T16:48:39Z

@padenot @aboba I think we at least have agreement on the API shape and its intended meaning. Would you mind approving the merge here and we can keep pushing on wording/style stuff in follow up GH issues?

@padenot @aboba friendly ping

…me_clone_close

index.src.html

chcunningham · 2021-04-28T16:15:10Z

Broadly: we are all in agreement about having clone()/close().

The mutability of AudioBuffer was undesirable. Also, we like having mor sample formats. See discussion in #179.

chcunningham · 2021-04-30T05:12:16Z

@padenot @aboba I believe the latest commits address all outstanding comments.

padenot

A couple of types, and some real questions, but generally I think this goes in the right direction.

index.src.html

padenot · 2021-05-03T15:26:01Z

index.src.html

  readonly attribute unsigned long long timestamp;
-  readonly attribute AudioBuffer? buffer;
+
+  undefined copyFromChannel(BufferSource destination, unsigned long channelNumber);


Something that occurred to me right after the call last Friday is that this API is not very good for interleaved data, it's really wasteful: each byte is touched twice to copy the full buffer.

Yeah I would expect this to be something like copyFromPlane and interleaved data would only have one plane.

This is now copyTo(buffer, planeNumber). I've renamed to copyTo() to match direction in this thread.

padenot · 2021-05-03T15:29:17Z

index.src.html

+:: 32-bit signed integer samples with planar channel arrangement.
+
+: <dfn enum-value for=AudioSampleFormat>FLTP</dfn>
+:: 32-bit float samples with planar channel arrangement.


We'll need to explain what interleaved and planar are. Also how does those data types hold audio: for example, 16-bit is linear audio from in [-65536, 65536], but the FLT variants only use [-1.0, 1.0] in general (but it can easily go outside this). And also S32 is only expected to use 24-bits probably?

Agree about more explanation. Filed #215 to track that work.

And also S32 is only expected to use 24-bits probably?
@dalecurtis - true for ffmpeg? I'm not savvy enough to say.

index.src.html

chcunningham

Addressing feedback.

index.src.html

chcunningham · 2021-05-04T03:20:28Z

index.src.html

  readonly attribute unsigned long long timestamp;
-  readonly attribute AudioBuffer? buffer;
+
+  undefined copyFromChannel(BufferSource destination, unsigned long channelNumber);


This is now copyTo(buffer, planeNumber). I've renamed to copyTo() to match direction in this thread.

index.src.html

chcunningham · 2021-05-04T04:13:15Z

index.src.html

+:: 32-bit signed integer samples with planar channel arrangement.
+
+: <dfn enum-value for=AudioSampleFormat>FLTP</dfn>
+:: 32-bit float samples with planar channel arrangement.


Agree about more explanation. Filed #215 to track that work.

And also S32 is only expected to use 24-bits probably?
@dalecurtis - true for ffmpeg? I'm not savvy enough to say.

index.src.html

padenot · 2021-05-04T14:35:30Z

index.src.html

+        {{AudioData}}'s [=media resource=], as described by the getter steps of
+        {{AudioData/allocationSize}}.
+    3. If |allocationSize| is greater than `destination.byteLength`, throw a
+        {{TypeError}}.


Isn't that usually RangeError from ES ? It would have been IndexSizeError, but that's deprecated.

padenot · 2021-05-04T14:43:51Z

index.src.html

  readonly attribute unsigned long long timestamp;
-  readonly attribute AudioBuffer? buffer;
+
+  undefined copyTo([AllowShared] BufferSource destination, unsigned long planeNumber);


I'm thinking maybe an offset and a frame count would make it a lot easier to send data to the WASM heap, and would help to address this feedback from game developers (while not exactly that if I remember our conversation, it's really close).

After demuxing properly (with all the edge cases, easier said than done!), you have quite often a decoder delay value in frames, and a padding value in frames (is WAV the only codec that doesn't have this? Certainly among the codecs available on the Web). Then imagine you're doing all your work in a custom audio engine written in C++, so the immediate thing you're doing is to copy the audio frames out to the WASM heap.

Say you're doing AAC, commonly it's 2112 frames of padding (not aligned to the common number of 1024 frames per packets), so you want to drop the two first packet entirely, and then copy to your heap with an offset of 64 frames starting on the third packet. Similarly, for the last packet, you only need to copy part of the packet, that's a function of a duration you find in the mp4 container usually (not always).

Here with the current API, you need to copy out to some intermediary buffer, and then copy from an offset to end final location, which is wasteful.

I believe adding two parameters

undefined copyTo([AllowShared] BufferSource destination, unsigned long planeNumber, optional unsigned offsetFrame, optional unsigned long frameCount);

with following meaning:

offset is an offset in frames in the plane - throw RangeError if bigger than this AudioData's numberOfFrames

frameCount is the number of frames to copy - throw RangeError if bigger than numberOfFrames - offset or destination.length. The Web Audio API clamps in a similar situation, but I'd rather be explicit in this situation. When omitted, it can be decided to mean min(numberOfFrames - offset, destination.length) if we want, that's ergonomic.

Most of the time you're not using those parameters though, hence the meaning of the default values.

I'm not a fan of APIs which take an index like `planeNumber'. It sounds instead like we should have a planes array on the AudioData object, so folks can say data.planes[i].copyTo(dest, offset, frameCount). This would provide some symmetry with the VideoFrame API as well. This also makes it clear that you're not simply indexing into a channel array.

@padenot , re offset + framecount + RangeErrors: all SGTM. Out of time tonight, but I'll send a new commit ASAP. I vote we create a AudioDataReadToOptions dict that lists these members alongside a required member for planeNumber. This also should help w/ @dalecurtis's concern that the simple argument looks like a channel index.

Re: planes VideoFrame symmetry... not sure on that. We may yet delete planes from VideoFrame ([still up in the air])(#157 (comment)), and adding a new Plane interface with a single copyTo() method feels like a lot of ceremony IMO.

WFM, thanks!

#223 opened for followup, linking to the valuable informations in this thread.

chcunningham · 2021-05-05T03:51:47Z

This PR still needs some work to address the open issues, but I'd like to go ahead and merge because its blocking 2 other VideoFrame PRs that have approval and a 3rd that is under discussion. I commit to send a new PR to address @padenot's last review tomorrow.

@chcunningham

SHA: 9f9da6d Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@chcunningham

SHA: 9f9da6d Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

chcunningham added 3 commits April 2, 2021 16:43

Make AudioFrame immutable and add reference counting semantics.

4926e78

Temporarily allow build warnings so I can split the change bewteen PRs

c574352

Relax warning for make 'ci' rule

09ef789

chcunningham mentioned this pull request Apr 5, 2021

Add refcounting semantics to VideoFrame #167

Merged

Fix typos, small style edits

1500351

padenot requested changes Apr 7, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

chcunningham commented Apr 9, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

Re-word frame-resource lifetime text

245c0a2

Merge branch 'main' of https://github.com/w3c/webcodecs into audiofra…

a5f3026

…me_clone_close

padenot reviewed Apr 28, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

chcunningham mentioned this pull request Apr 28, 2021

Audio decoder output to regular buffers and not AudioBuffer #179

Closed

chcunningham mentioned this pull request Apr 28, 2021

Clarify AudioFrame data 'snapshotting' behavior for ctor() and clone() #197

Closed

chcunningham added 2 commits April 28, 2021 19:49

Merge remote-tracking branch 'origin/main' into audioframe_clone_close

00705e6

Rename AudioFrame->AudioData. Drop dependency on AudioBuffer.

c2586e1

The mutability of AudioBuffer was undesirable. Also, we like having mor sample formats. See discussion in #179.

chcunningham changed the title ~~Make AudioFrame immutable and add reference counting semantics.~~ Rename AudioFrame->AudioData, drop AudioBuffer, add ref counting semantics. Apr 30, 2021

Relax media resource lifetime to be 'at least as' long as its references

5dd224a

Copy specific channel bytes in copyFromChannel()

59464f3

chcunningham mentioned this pull request Apr 30, 2021

Make Encoded*Chunk interfaces immutable. #174

Merged

padenot reviewed May 3, 2021

View reviewed changes

chcunningham added 2 commits May 3, 2021 20:01

Merge remote-tracking branch 'origin/main' into audioframe_clone_close

27174c4

Fix typos, rename copyFromChannel -> copyTo.

5b33d7f

chcunningham commented May 4, 2021

View reviewed changes

padenot reviewed May 4, 2021

View reviewed changes

chcunningham merged commit 9f9da6d into main May 5, 2021

github-actions bot added a commit that referenced this pull request May 5, 2021

Merge pull request #162 from w3c/audioframe_clone_close

eb013fe

SHA: 9f9da6d 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 May 5, 2021

Merge pull request #162 from w3c/audioframe_clone_close

23c1176

SHA: 9f9da6d Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Rename AudioFrame->AudioData, drop AudioBuffer, add ref counting semantics. #162

Rename AudioFrame->AudioData, drop AudioBuffer, add ref counting semantics. #162

Uh oh!

Conversation

chcunningham commented Apr 2, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chcunningham commented Apr 5, 2021

Uh oh!

padenot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chcunningham commented Apr 15, 2021

Uh oh!

chcunningham commented Apr 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chcunningham commented Apr 28, 2021

Uh oh!

chcunningham commented Apr 30, 2021

Uh oh!

padenot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

padenot May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chcunningham May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chcunningham commented May 5, 2021

Uh oh!

Uh oh!

chcunningham commented Apr 2, 2021 •

edited by pr-preview bot

Loading

padenot May 4, 2021 •

edited

Loading

chcunningham May 5, 2021 •

edited

Loading