1598114 - implement AudioWorkletNode.parameters

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P2)

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Use char16_t for AudioParam names, since they can now be exposed to js and contain non-ascii values. r?karlt

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Pass the graph time to ProcessBlockOnPorts. r?karlt

This is necessary to then compute AudioParam values for the right time.

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Implement `AudioWorkletNode.parameters`. r?karlt

This covers only the main thread part of this attribute.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Receive AudioParamTimeline events to the render thread. r?karlt

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Initialize the javascript objects necessary to pass the third parameter to `process`. r?karlt

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Compute the values for each AudioParamTimeline directly into the Float32Arrays and pass the object to `process`. r?karlt

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Update WPTs expectations after implementation of AudioWorkletNode parameters. r?karlt

Comment 8

[Steve Fink [:sfink] [:s:]]

With respect to the rooting hazard at AudioWorkletNode.cpp:471: the difficulty is that you've declared that nothing will GC (via AutoCheckCannotGC at line 462), which is good because dest might be stored directly within a GC thing and therefore be invalidated by the GC. I'll note that the scope of that AutoCheckCannotGC is too large; it prevents any GC from that point in the function to the end, which seems difficult to enforce. You'll probably want to move it within the for loop body. It won't do anything at all in a non-DEBUG build, so it's not a performance concern.

But the actual problem is that GetValuesAtTime() is eager to expire all events older than the queried one (!?), which freaks out the analysis because it sees ~AudioTimelineEvent calling ~AudioNodeStream calling ~MediaStream calling all kinds of crazy stuff. I'm not entirely sure why it thinks that call chain is possible -- it says ~AudioTimelineEvent calls ~RefPtr<AudioNodeStream>, which seems odd since an AudioTimelineEvent has a RefPtr<AudioNodeTrack> not RefPtr<AudioNodeStream>, but whatever. I think you can safely say that the refcount will not drop to zero and so this case will not trigger a GC. The most straightforward way to do that would be to make a small scope around the CleanupEventsOlderThan call:

   {
     // Removing events will not trigger the (stream? track?) refcount to drop to zero because all of the events will
     // be for the same (stream? track?) and we will leave at least one behind, so we won't trigger GC from here.
     JS::AutoSuppressGCAnalysis nogc;
     CleanupEventsOlderThan(aTime);
    }

...except make the comment say something that makes sense and is true. :-)

Then again, I'm not confident I'm following this code correctly. Because it seems weird that something called GetValuesAtTime would be destructive, and in fact could trigger a seemingly O(n^2) destruction process (the cleanup scans oldest first, and if it finds something to expire does an O(n) RemoveElementAt(0) call.) But maybe you just never build up stale stuff? Still, it seems weird that the loop in the original code would call GetValuesAtTime() for every element, thus guaranteeing that you're discarding all but one? I'm probably just misreading things.

Comment 9

[Karl Tomlinson (:karlt)]

Thank you for having a look, Steve.

safely say that the refcount will not drop to zero and so this case will not trigger a GC.

The refcount can drop to zero, so I assume the choice is one of:

1. We need to be sure that ~MediaStream can't trigger GC,
2. We copy from a temporary intermediate buffer,
3. We use a different kind of array buffer with client-provided contents so that the buffer can be accessed via means other than JS_GetFloat32ArrayData(), or
4. Redesign GetValuesAtTime() to avoid side effects, and use a different method to purge old events.

Comment 10

[Paul Adenot (:padenot)]

For the rooting hazard issue, we can also never destroy AudioEventTimeline containing mTrack on the render thread, this is rather easy and has the right property, but this property cannot be checked statically so we'd have to use the thing Steve mentioned above. AudioTimelineEvent doesn't need to keep a reference to the AudioNodeTrack, it's only to pass the track to the AudioParamTimeline. Another way would be to stop overloading AudioTimelineEvent to pass the stream (it's not really an event on the AudioParam timeline, it was just convenient to do it like that at the time). This would have the benefit of silencing the analysis.

Generally the AudioParam code is slow, incomplete/wrong and complicated, so I'm not really willing to do a fix that is too invasive, and we're not going to rewrite it now, because we're trying to ship AudioWorklet, but we should.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #10)

AudioTimelineEvent doesn't need to keep a reference to the AudioNodeTrack, it's only to pass the track to the AudioParamTimeline.

Ah, good point. AudioParamTimeline::InsertEvent() does not actually save the event when it contains a track

So the RemoveElementAt() in CleanupEventsOlderThan() could follow an assert !mEvents[0].mTrack and be wrapped in AutoSuppressGCAnalysis.

Another way would be to stop overloading AudioTimelineEvent to pass the stream (it's not really an event on the AudioParam timeline, it was just convenient to do it like that at the time). This would have the benefit of silencing the analysis.

Yes, that would work too and sounds good, but I haven't looked into what that would involve.

Comment 12

[Paul Adenot (:padenot)]

Attachment: Bug 1598114 - Never destroy `AudioTimelineEvent` from the render thread when it contains a valid pointer to an `AudioNodeTrack`, and assert it. r?karlt

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be37fac7bc9e
Use char16_t for AudioParam names, since they can now be exposed to js and contain non-ascii values. r=karlt
https://hg.mozilla.org/integration/autoland/rev/8cb2c6c40eac
Pass the graph time to ProcessBlockOnPorts. r=karlt
https://hg.mozilla.org/integration/autoland/rev/0561d9bf849f
Implement `AudioWorkletNode.parameters`. r=karlt
https://hg.mozilla.org/integration/autoland/rev/95fddf6af53d
Receive AudioParamTimeline events to the render thread. r=karlt
https://hg.mozilla.org/integration/autoland/rev/96e097707886
Initialize the javascript objects necessary to pass the third parameter to `process`. r=karlt
https://hg.mozilla.org/integration/autoland/rev/3f06a2523af9
Compute the values for each AudioParamTimeline directly into the Float32Arrays and pass the object to `process`. r=karlt
https://hg.mozilla.org/integration/autoland/rev/03bb577930c8
Update WPTs expectations after implementation of AudioWorkletNode parameters. r=karlt
https://hg.mozilla.org/integration/autoland/rev/2e184c82e740
Never destroy `AudioTimelineEvent` from the render thread when it contains a valid pointer to an `AudioNodeTrack`, and assert it. r=karlt

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/be37fac7bc9e
https://hg.mozilla.org/mozilla-central/rev/8cb2c6c40eac
https://hg.mozilla.org/mozilla-central/rev/0561d9bf849f
https://hg.mozilla.org/mozilla-central/rev/95fddf6af53d
https://hg.mozilla.org/mozilla-central/rev/96e097707886
https://hg.mozilla.org/mozilla-central/rev/3f06a2523af9
https://hg.mozilla.org/mozilla-central/rev/03bb577930c8
https://hg.mozilla.org/mozilla-central/rev/2e184c82e740

Comment 15

[Karl Tomlinson (:karlt)]

Thank you for landing this, Paul.

I wonder whether the last changeset was the version that you intended to land given further changes described in https://phabricator.services.mozilla.com/D64759#1991643 ?

Comment 16

[Paul Adenot (:padenot)]

Indeed. It's functionally equivalent however, I'll fix that later.