1200579 - Stop copying AudioParam timelines

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Paul Adenot (:padenot)]

It's highly ineffective. Let's consider this benchmark [0], that implements a very classic toy substractive synth.

An inverse profiler call tree shows this:
5.1%: nsTArray<AudioTimelineEvent>::AppendElements
2.2%: nsTArray<AudioTimelineEvent>::ReplaceElementsAt
1.4%: nsTArray<AudioTimelineEvent>::~nsTArray

Instead, we should:
- Keep a range list of period of time where a curve is present, so we can throw synchronously on the main thread (since per spec, we have to throw if authors try to set an event during a curve). We can do other checks without knowing the state of the timeline
- Send just the single event to the MSG thread using a ControlMessage
- Append this event to the AudioNodeEngine's timeline

This should get rid of most nsTArray<AudioTimelineEvent>::* in the profiles.

[0]: https://github.com/padenot/webaudio-benchmark/blob/gh-pages/benchmarks.js#L287

Comment 1

[Paul Adenot (:padenot)]

(In reply to Paul Adenot (:padenot) from comment #0)
> - Keep a range list of period of time where a curve is present, so we can
> throw synchronously on the main thread (since per spec, we have to throw if
> authors try to set an event during a curve). We can do other checks without
> knowing the state of the timeline

I was wrong on this one, we need the full timeline because we need to check whether the previous value is zero for an exponential ramp.

Either we need to duplicate the timeline, or we need to lock. Locking is not ideal (it's conceptually against the architecture of the MSG), but looks like it would be more efficient. It also potentially has the advantage of lowering the time between the JavaScript call and reflecting it in the msg-side timeline, but I might have other plans to address that.

For now I'm going to go with the duplication. Then I'll implement the lock (I think it's easier), and do some testing/benchmarking.

Comment 2

[Karl Tomlinson (:karlt)]

Locks would mean that "immediate" changes would not be processed as a transaction with other timeline changes or other graph changes.

IIRC the main thread needs the timeline to for the value getter, which would make
duplicating the timeline necessary.  I haven't looked at the code, but perhaps more culling of old events can be done.

Comment 3

[Paul Adenot (:padenot)]

So in fact, the timeline is already present twice in memory, since we don't clear out the previous data, I didn't know the code enough when I started this, and I had wrong assumption.

This allow us to implement `BiquadFilterNode.getFrequencyResponse` properly. We could implement synchronous querying of the value (the `AudioParam.value` getter) in the same way.

And indeed culling of old events makes sense even on the main thread.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Stop copying AudioParam timelines. r=

We stop copying and just send AudioTimelineEvents to the MSG, using the same
message-passing mechanism we use for the rest of the main thread -> MSG
communication.

A couple notable points:
- Checking that an event is valid now only occurs on the main thread. When
sent, it is considered valid for insertion and is just inserted. This is ok
because the two timeline are exact copyies (except MSG's copy pruning, but we
only prune behind the AudioContext.currentTime anyways)
- We now have to send changes for the `value` attribute of the `AudioParam` and
the fact that the input of an `AudioParam` is a node through the same
`AudioTimelineEvent`. We take advantage of the fact that there cannot be both a
curve and a stream in the same event, so we can safely use an union here.
- Conversion between time in seconds (main thread) and time in MSG ticks (MSG
thread) happens right before the insertion in the MSG `AudioParamTimeline`. We
now use exclusively the getter and not the union, so it's safer. Because we
send events and not the full timeline each time we don't have to convert all the
events each time.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Changes in all the AudioNode implementations. r=

This is all the (mechanical) changes to convert the nodes to the new way of
sending timeline events to the MSG.

Comment 6

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab377e48126

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8659124 [details] [diff] [review]
Stop copying AudioParam timelines. r=

The approach looks good thanks.  A few little things.

>+    MediaStream* mStream;

Please document what ensures that mStream remains alive, or keep a reference.

> private:
>   static bool IsValid(double value)
>   {
>     return mozilla::IsFinite(value);
>   }
>+private:
>+  // This member is accessed using the `Time` method, for safety.

Gecko convention is to use "private:" only as necessary (once), rather than
for each member.

>-  void InsertEvent(const AudioTimelineEvent& aEvent, ErrorResult& aRv)
>+  bool ValidateEvent(AudioTimelineEvent& aEvent,
>+                     ErrorResult& aRv)
>   {
>+    MOZ_ASSERT(NS_IsMainThread());
>+
>+    if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) ||
>+        !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
>+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>+      return false;
>+    }
>+
>     if (!aEvent.IsValid()) {
>       aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>-      return;
>+      return false;

Is there a distinction between timeline-independent checks in ValidateEvent()
and those in IsValid()?  Could these all be on an AudioTimelineEvent method?

>+  const AudioTimelineEvent* GetPreviousEvent(double aTime) const

Can this remain private?

>-private:
>+protected:
>   // This is a sorted array of the events in the timeline.  Queries of this

Please remove the "protected:" line to leave this private.
There is no need to export these to derived classes AFAICS.

>+++ b/dom/media/webaudio/AudioNode.h
>+#include "AudioEventTimeline.h"
> 
> namespace mozilla {
> 
> namespace dom {
> 
> class AudioContext;
> class AudioBufferSourceNode;
> class AudioParam;
>@@ -218,18 +219,18 @@ private:

>+  static void SendTimelineEventToStream(AudioNode* aNode, uint32_t aIndex,
>+                                        const dom::AudioTimelineEvent& aEvent);

Forward declare instead of including in headers, where possible.
Otherwise includes get out of control.

Here, use "struct AudioTimelineEvent" like the AudioParam declaration.

Similarly in AudioNodeEngine.h, AudioNodeStream.h.

>-  virtual void SetTimelineParameter(uint32_t aIndex,
>-                                    const dom::AudioParamTimeline& aValue,
>-                                    TrackRate aSampleRate)
>+  virtual void SendTimelineEvent(uint32_t aIndex,
>+                                 dom::AudioTimelineEvent& aValue)

All the comparable AudioNodeEngine and AudioNodeStream methods are called
Set*.  "SetTimelineEvent" wouldn't make much sense, so I like
SendTimelineEvent on the AudioNodeStream, but its not so appropriate on the
engine.  Can you call this "AudioNodeEngine::RecvTimelineEvent" or similar,
please?

(Another option is "AddTimelineEvent", which could be used for both
stream and engine methods, but "Recv" does more to imply that the engine is
assuming ownership and modifying the event through the non-const reference.)

>+  // This sends a single event to the timeline, on the MSG thread side.

No comma ',' because it is the timeline that is on the MSG thread,
not the send.

>-  // We override SetValueCurveAtTime to convert the Float32Array to the wrapper
>-  // object.
>-  void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv)
>+    // We override SetValueCurveAtTime to convert the Float32Array to the wrapper
>+    // object.
>+    void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv)

Indentation.

Other methods call InsertEvent(), but SetValueCurveAtTime() doesn't.  That
would be a change in the behavior of the method.  Calling InsertEvent seems
the appropriate behavior so that the main thread can calculate the intrinsic
value.

The validate/insert/callback pattern is frequent.  A helper method would tidy
up a bit and ensure that the same thing happens in each case.

SetValue() calls InsertEvent() but CancelScheduledValues() chains up to the base
class method.  Can CancelScheduledValues follow the pattern of the others
and use the helper.

Some methods are calling DOMTimeToStreamTime() on the event time.  Others are
not.  I expect the helper can also create the event and do the conversion.

>-    if (!WebAudioUtils::IsTimeValid(aStartTime)) {
>-      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

The error code is changing to NS_ERROR_DOM_SYNTAX_ERR.
I assume that's not intentional.
Similarly elsewhere.

>+    // Remove some events on the main thread copy.
>+    AudioEventTimeline::CancelScheduledValues(DOMTimeToStreamTime(aStartTime));
>+
>+    AudioTimelineEvent event(AudioTimelineEvent::Cancel, aStartTime, 0.0f);

Missing DOMTimeToStreamTime().

>+   MOZ_ASSERT(!aSource || aSource->SampleRate() == aDest->SampleRate());
>+  aEvent.SetTimeInTicks(

Indentation.

>-   * Converts AudioParamTimeline floating point time values to tick values
>-   * with respect to a source and a destination AudioNodeStream.
>+   * Converts an AudioTimelineEvent point time values to tick values
>+   * with respect to a destination AudioNodeStream.

Please keep "floating".

Either "Converts an AudioTimelineEvent's floating point time values"
    or "Converts AudioTimelineEvent floating point time values".

>+++ b/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp

>+#include "MainThreadUtils.h"

Why this change?

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8659127 [details] [diff] [review]
Changes in all the AudioNode implementations. r=

Please fold this into the other patch because it won't compile without these
changes.

The best improvement to readability from dividing up the changes would be to
move (and make public) InsertEvent() and ValidateEvent() in a separate patch
with no other changes.

>+++ b/dom/media/webaudio/BiquadFilterNode.h
>@@ -5,16 +5,17 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #ifndef BiquadFilterNode_h_
> #define BiquadFilterNode_h_
> 
> #include "AudioNode.h"
> #include "AudioParam.h"
> #include "mozilla/dom/BiquadFilterNodeBinding.h"
>+#include "AudioEventTimeline.h"

Forward declare.

Comment 9

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #7)
> Comment on attachment 8659124 [details] [diff] [review]
> Stop copying AudioParam timelines. r=
> 
> The approach looks good thanks.  A few little things.
> 
> >+    MediaStream* mStream;
> 
> Please document what ensures that mStream remains alive, or keep a reference.

I've added a strong reference. I also had to mock MediaStream in the unit test.

> >-  void InsertEvent(const AudioTimelineEvent& aEvent, ErrorResult& aRv)
> >+  bool ValidateEvent(AudioTimelineEvent& aEvent,
> >+                     ErrorResult& aRv)
> >   {
> >+    MOZ_ASSERT(NS_IsMainThread());
> >+
> >+    if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) ||
> >+        !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
> >+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> >+      return false;
> >+    }
> >+
> >     if (!aEvent.IsValid()) {
> >       aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> >-      return;
> >+      return false;
> 
> Is there a distinction between timeline-independent checks in ValidateEvent()
> and those in IsValid()?  Could these all be on an AudioTimelineEvent method?

So, we need to return very specific DOM errors depending on certain values in the event. We can't use ErrorResult in AudioTimelineEvent because it's not templated on ErrorResult, and we don't pull in the DOM's ErrorResult in the cppunittest.

I've consolidated everything in the ValidateEvent method instead. At least, all the checks are in the sample place now.

> >+  const AudioTimelineEvent* GetPreviousEvent(double aTime) const
> 
> Can this remain private?

Yes. I'm about to remove it, but I've made it private for now.

> >-private:
> >+protected:
> >   // This is a sorted array of the events in the timeline.  Queries of this
> 
> Please remove the "protected:" line to leave this private.
> There is no need to export these to derived classes AFAICS.

Sorry, this was from a previous iteration of the patch.

> >-  virtual void SetTimelineParameter(uint32_t aIndex,
> >-                                    const dom::AudioParamTimeline& aValue,
> >-                                    TrackRate aSampleRate)
> >+  virtual void SendTimelineEvent(uint32_t aIndex,
> >+                                 dom::AudioTimelineEvent& aValue)
> 
> All the comparable AudioNodeEngine and AudioNodeStream methods are called
> Set*.  "SetTimelineEvent" wouldn't make much sense, so I like
> SendTimelineEvent on the AudioNodeStream, but its not so appropriate on the
> engine.  Can you call this "AudioNodeEngine::RecvTimelineEvent" or similar,
> please?

Yes.

> >-  // We override SetValueCurveAtTime to convert the Float32Array to the wrapper
> >-  // object.
> >-  void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv)
> >+    // We override SetValueCurveAtTime to convert the Float32Array to the wrapper
> >+    // object.
> >+    void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv)
> 
> Indentation.
> 
> Other methods call InsertEvent(), but SetValueCurveAtTime() doesn't.  That
> would be a change in the behavior of the method.  Calling InsertEvent seems
> the appropriate behavior so that the main thread can calculate the intrinsic
> value.

Yes, that's a mistake.

> The validate/insert/callback pattern is frequent.  A helper method would tidy
> up a bit and ensure that the same thing happens in each case.

I've done that. `CancelScheduledValue()` and `SetValue()` should not insert an event on the same thread. Instead, they should modify the timeline on the main thread and then send the event to the MSG thread.

> SetValue() calls InsertEvent() but CancelScheduledValues() chains up to the
> base
> class method.  Can CancelScheduledValues follow the pattern of the others
> and use the helper.

This is a mistake, per above.

> Some methods are calling DOMTimeToStreamTime() on the event time.  Others are
> not.  I expect the helper can also create the event and do the conversion.

Done. The methods that are creating a special event (SetValue, CancelScheduledValue, Stream), have not been moved to the helper so that the method itself is simple.

> 
> >-    if (!WebAudioUtils::IsTimeValid(aStartTime)) {
> >-      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> 
> The error code is changing to NS_ERROR_DOM_SYNTAX_ERR.
> I assume that's not intentional.
> Similarly elsewhere.

This is all about to change (in bug 1204883). For, I mostly care about the tests passing.

> >+++ b/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp
> 
> >+#include "MainThreadUtils.h"
> 
> Why this change?

The test does not compile anymore otherwise, I've added a number of threading assertion using `NS_IsMainThread()` in `AudioEventTimeline.h` in `AudioTimelineEvent::Time()`.

Comment 10

[Paul Adenot (:padenot)]

Attachment: interdiff

Here is an interdiff for the first patch.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Stop copying AudioParam timelines. r=

This is everything folded into one patch.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8661231 [details] [diff] [review]
Stop copying AudioParam timelines. r=

Please don't move GetPreviousEvent().
That is adding noise to the patch and making it harder to review.

I haven't yet reviewed the InsertEvent() code moved in this version.

>* * *
>Bug 1200579 - Changes in all the AudioNode implementations. r=
>
>This is all the changes to convert the nodes to the new way of sending timeline
>event to the MSG.

This can be removed.

>   AudioTimelineEvent(const AudioTimelineEvent& rhs)
>   {
>     PodCopy(this, &rhs, 1);
>     if (rhs.mType == AudioTimelineEvent::SetValueCurve) {
>       SetCurveParams(rhs.mCurve, rhs.mCurveLength);
>     }
>   }
> 
>   ~AudioTimelineEvent()
>   {
>     if (mType == AudioTimelineEvent::SetValueCurve) {
>       delete[] mCurve;
>     }
>   }

These are not dealing with mStream appropriately when mType == Stream.
mStream's destructor needs to be run explicitly when appropriate.

> template <>
> inline double AudioTimelineEvent::Time<double>() const
> {
>   MOZ_ASSERT(!mTimeIsInTicks);
>+  MOZ_ASSERT(NS_IsMainThread());
>   return mTime;

Used off main thread from ConvertAudioTimelineEventToTicks().

>+    // Validate the event itself
>+    if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) ||
>+        !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
>+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>+      return false;
>+    }

>+    bool timeValueValid = IsValid(aEvent.template Time<double>()) &&
>+                          IsValid(aEvent.mValue) &&
>+                          IsValid(aEvent.mTimeConstant) &&
>+                          IsValid(aEvent.mDuration);

WebAudioUtils::IsTimeValid() is stricter than IsValid(), so no need to test
Time() and mTimeConstant again.

>+++ b/dom/media/webaudio/AudioNodeEngine.h
>@@ -5,16 +5,17 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> #ifndef MOZILLA_AUDIONODEENGINE_H_
> #define MOZILLA_AUDIONODEENGINE_H_
> 
> #include "AudioSegment.h"
> #include "mozilla/dom/AudioNode.h"
> #include "mozilla/MemoryReporting.h"
> #include "mozilla/Mutex.h"
>+#include "AudioEventTimeline.h"

Forward declare.

>+++ b/dom/media/webaudio/AudioParam.h
>@@ -10,28 +10,29 @@
> #include "AudioParamTimeline.h"
> #include "nsWrapperCache.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsAutoPtr.h"
> #include "AudioNode.h"
> #include "mozilla/dom/TypedArray.h"
> #include "WebAudioUtils.h"
> #include "js/TypeDecls.h"
>+#include "AudioEventTimeline.h"

Already included through AudioParamTimeline.h.

>@@ -44,87 +45,103 @@ public:
> 
>   double DOMTimeToStreamTime(double aTime) const
>   {
>     return mNode->Context()->DOMTimeToStreamTime(aTime);
>   }
> 
>   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> 
>+  void EventInsertionHelper(ErrorResult& aRv,

private or protected.  (This class is final.)

>+    InsertEvent<double>(event);

AudioParamTimeline::InsertEvent to skip the unnecessary Cancel/SetValue/Stream
handling.

(In reply to Paul Adenot (:padenot) from comment #9)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #7)

> > >+#include "MainThreadUtils.h"
> > 
> > Why this change?
>
> The test does not compile anymore otherwise, I've added a number of threading
> assertion using `NS_IsMainThread()` in `AudioEventTimeline.h` in
> `AudioTimelineEvent::Time()`.

That should go in AudioTimelineEvent.h then.
It's counter-intuitive for callers to need to include headers for their
callees.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8661231 [details] [diff] [review]
Stop copying AudioParam timelines. r=

>+    // Remove some events on the main thread copy.
>+    AudioEventTimeline::CancelScheduledValues(DOMTimeToStreamTime(aStartTime));
>+
>+    AudioTimelineEvent event(AudioTimelineEvent::Cancel,
>+                             DOMTimeToStreamTime(aStartTime), 0.0f);

DOMTimeToStreamTime() is non-trivial so call the method only once.
Either read from the event or use a local variable.

Comment 14

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7750079af88d

Comment 15

[Karl Tomlinson (:karlt)]

In the copy constructor, need to skip the PodCopy when mType is Stream and use placement new instead of mStream.operator=.

http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=556
may be helpful.

Comment 16

[Karl Tomlinson (:karlt)]

Actually, I guess the PodCopy is ok with placement new after the copy.

Comment 17

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c111743fe83

Comment 18

[Karl Tomlinson (:karlt)]

Perhaps it is easier manually refcount a MediaStream* AudioTimelineEvent::mStream than using placement new on a nsRefPtr<MediaStream>.

Comment 19

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19155c090395

Comment 20

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa7010a7240c

Comment 21

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/try/rev/9a1c72aad2da has a placement new solution, so that mType is not used uninitialized in the copy constructor,
but it seems that the MSVC version doesn't have support for these unions.
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e50d1d9a9e

Comment 22

[Paul Adenot (:padenot)]

I think I'll use inheritance to have tighter instances for the `AudioTimelineEvent` and compile everywhere. We can save some bytes, and this is the kind of class that gets instantiated a lot and where each instance is duplicated, on top of that.

I've got green tries with patches resembling yours in comment 21, but refactoring this so that it's tighter and looks better is probably not hard so I'll do it before re-requesting review, in a separate patch.

Comment 23

[Paul Adenot (:padenot)]

In fact we would need to change a bunch of things that we are changing in bug 1184057, and I expect to get to it soon, so I'll make the event smaller later later, and put mStream out of the union for now.

I've taken you patch in comment 21, it's better than what I had.

Comment 24

[Paul Adenot (:padenot)]

Attachment: Stop copying AudioParam timelines. r=

This has your patch to the copy constructor, and all the comments addressed.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8664346 [details] [diff] [review]
Stop copying AudioParam timelines. r=

(In reply to Paul Adenot (:padenot) from comment #23)
> In fact we would need to change a bunch of things that we are changing in
> bug 1184057, and I expect to get to it soon, so I'll make the event smaller
> later later, and put mStream out of the union for now.

Moving it out of the union makes sense for now, but see below for one issue.

Inheritance works well for individually heap allocated classes, but
AudioTimelineEvent has many stack/member allocations.  I think it would be
best to avoid allocating each event on the heap individually, and so I suspect
a union with manual ref-counting of bare pointers will provide the best long
term solution.

>   ~AudioTimelineEvent()
>   {
>     if (mType == AudioTimelineEvent::SetValueCurve) {
>       delete[] mCurve;
>+    } else if (mType == AudioTimelineEvent::Stream) {
>+      mStream.~nsRefPtr<MediaStream>();
>     }
>   }

Don't add the new code here.  Now that mStream is not part of a union, this is
not necessary, and I think it will cause problems because mStream's destructor
should run after the AudioTimelineEvent destructor.  Running the destructor
twice will release the reference twice.

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> >* * *
> >Bug 1200579 - Changes in all the AudioNode implementations. r=
> >
> >This is all the changes to convert the nodes to the new way of sending timeline
> >event to the MSG.
> 
> This can be removed.

To be clear, I wasn't asking for the whole commit message to be removed, just
the quoted part left over from the fold.  However, I guess much of the comment
may be obsolete now anyway.

>+    // Validate the event itself
>+    if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) ||
>+        !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
>+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>+      return false;
>+    }

>-    if (!WebAudioUtils::IsTimeValid(aStartTime)) {
>-      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

There are two issues with these changes:

1) AudioEventTimeline has these double times after DOMTimeToStreamTime
   conversion and so they may be negative.

   This change makes the testcase in bug 864171 throw.

   I don't like these DOMTimeToStreamTime conversions anyway, so if you are
   happy with the fix that I'll upload to that bug, then we can ignore this.

2) browser/devtools/webaudioeditor/test/browser_callwatcher-02.js is failing.
   I assume that is due to these changes.

   https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e50d1d9a9e

For now, I would leave the checks in AudioParam.h and make minimal changes to
the checks in AudioEventTimeline.h.

Comment 26

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1e3774c416d

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f47605f2babe

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f47605f2babe