The default bug view has changed. See this FAQ.

Implement OfflineAudioContext

RESOLVED FIXED in mozilla24

Status

()

Core
Web Audio
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-complete})

Trunk
mozilla24
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 5 obsolete attachments)

4.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.41 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.79 KB, patch
roc
: review+
Ehsan
: checkin+
Details | Diff | Splinter Review
2.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.18 KB, patch
Details | Diff | Splinter Review
8.44 KB, patch
Details | Diff | Splinter Review
9.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.96 KB, patch
Details | Diff | Splinter Review
8.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
roc, do you know what underlying assumptions MediaStreamsGraphImpl makes about the underlying hardware, such as the number of channels, sample rate, etc.?

Thanks!
(Assignee)

Comment 1

4 years ago
Created attachment 709855 [details] [diff] [review]
Part 1: Make MediaStream aware of its owner graph
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #709855 - Flags: review?(roc)
(Assignee)

Comment 2

4 years ago
Created attachment 709857 [details] [diff] [review]
Part 2: Make MediaInputPort aware of its owner graph
Attachment #709857 - Flags: review?(roc)
(Assignee)

Comment 3

4 years ago
Created attachment 709858 [details] [diff] [review]
Part 3: Make MediaStreamGraphThreadRunnable know about its owner graph
Attachment #709858 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 709859 [details] [diff] [review]
Part 4: Make MediaStreamGraphStableStateRunnable know about its owner graph
Attachment #709859 - Flags: review?(roc)
(Assignee)

Comment 5

4 years ago
Created attachment 709860 [details] [diff] [review]
Part 5: Make MediaStreamGraphImpl::RunInStableState not assume that it's the only graph
Attachment #709860 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
Created attachment 709862 [details] [diff] [review]
Part 6: Make MediaStreamGraphImpl::AppendMessage not assume that it's the only graph
Attachment #709862 - Flags: review?(roc)
(Assignee)

Comment 7

4 years ago
I haven't finished the rest of the patches in this series yet so I won't attach them to the bug just now, but we should be able to land the first 6 parts now.
Attachment #709855 - Flags: review?(roc) → review+
Attachment #709857 - Flags: review?(roc) → review+
Attachment #709858 - Flags: review?(roc) → review+
Attachment #709859 - Flags: review?(roc) → review+
Attachment #709860 - Flags: review?(roc) → review+
Attachment #709862 - Flags: review?(roc) → review+
(Assignee)

Comment 8

4 years ago
Landed the first 6 parts:

https://hg.mozilla.org/integration/mozilla-inbound/rev/852af57de5cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/efe8cb43b6cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/718e8b7288ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c03536917a
https://hg.mozilla.org/integration/mozilla-inbound/rev/12715a3b5a08
https://hg.mozilla.org/integration/mozilla-inbound/rev/189766068d99
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/852af57de5cc
https://hg.mozilla.org/mozilla-central/rev/efe8cb43b6cd
https://hg.mozilla.org/mozilla-central/rev/718e8b7288ae
https://hg.mozilla.org/mozilla-central/rev/66c03536917a
https://hg.mozilla.org/mozilla-central/rev/12715a3b5a08
https://hg.mozilla.org/mozilla-central/rev/189766068d99
Note that bug 836850 is now fixed, so we can just use a single class here if desired.
(Assignee)

Comment 11

4 years ago
(In reply to Boris Zbarsky (:bz) from comment #10)
> Note that bug 836850 is now fixed, so we can just use a single class here if
> desired.

Yes, thanks!
Depends on: 836850
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(Assignee)

Updated

4 years ago
Depends on: 859600
(Assignee)

Comment 12

4 years ago
Created attachment 746745 [details] [diff] [review]
Part 7: Implement a non-realtime API in MediaStreamGraph

This currently passes try.  I'd like to land this part sooner.  I'm working on the rest.
Attachment #746745 - Flags: review?(roc)
Comment on attachment 746745 [details] [diff] [review]
Part 7: Implement a non-realtime API in MediaStreamGraph

Review of attachment 746745 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #746745 - Flags: review?(roc) → review+
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(Assignee)

Comment 14

4 years ago
Comment on attachment 746745 [details] [diff] [review]
Part 7: Implement a non-realtime API in MediaStreamGraph

https://hg.mozilla.org/integration/mozilla-inbound/rev/c97a67b4ec08
Attachment #746745 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c97a67b4ec08
(Assignee)

Updated

4 years ago
Depends on: 871226
(Assignee)

Updated

4 years ago
Depends on: 872635
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(Assignee)

Comment 16

4 years ago
Created attachment 749983 [details] [diff] [review]
Part 8: Add a non-realtime media graph API to produce a given number of ticks
Attachment #749983 - Flags: review?(roc)
(Assignee)

Comment 17

4 years ago
Created attachment 749984 [details] [diff] [review]
Part 9: Implement the DOM binding for OfflineAudioContext
Attachment #749984 - Flags: review?(roc)
(Assignee)

Comment 18

4 years ago
Created attachment 749986 [details] [diff] [review]
Part 10: Use the non-realtime MediaStreamGraph API and a custom destination node engine for OfflineAudioContext
Attachment #749986 - Flags: review?(roc)
(Assignee)

Comment 19

4 years ago
Created attachment 749987 [details] [diff] [review]
Part 11: Implement the processing of OfflineAudioContext
Attachment #749987 - Flags: review?(roc)
(Assignee)

Comment 20

4 years ago
Created attachment 749988 [details] [diff] [review]
Part 12: Fix the lifetime management of the Web Audio graph in presence of OfflineAudioContexts
Attachment #749988 - Flags: review?(roc)
(Assignee)

Comment 21

4 years ago
Created attachment 749989 [details] [diff] [review]
Part 13: Add a unit test for OfflineAudioContext
Attachment #749989 - Flags: review?(roc)
(Assignee)

Comment 22

4 years ago
Created attachment 749990 [details] [diff] [review]
Part 14: Run most of the existing Web Audio tests using OfflineAudioContext as well as AudioContext
Attachment #749990 - Flags: review?(roc)
(Assignee)

Comment 23

4 years ago
Created attachment 749991 [details] [diff] [review]
(diff -w of part 14)
(Assignee)

Updated

4 years ago
Blocks: 872394
(Assignee)

Comment 24

4 years ago
Try is happy with this stuff: https://tbpl.mozilla.org/?tree=Try&rev=b4da97aa2e0f
Attachment #749984 - Flags: review?(roc) → review+
Comment on attachment 749983 [details] [diff] [review]
Part 8: Add a non-realtime media graph API to produce a given number of ticks

Review of attachment 749983 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaStreamGraphImpl.h
@@ +531,5 @@
>     */
>    bool mRealtime;
> +  /**
> +   * True when a non-realtime MediaStreamGraph has started to process input.  This
> +   * value is always set to true on the main thread.

"only accessed on the main thread"
Attachment #749983 - Flags: review?(roc) → review+
Attachment #749986 - Flags: review?(roc) → review+
Comment on attachment 749987 [details] [diff] [review]
Part 11: Implement the processing of OfflineAudioContext

Review of attachment 749987 [details] [diff] [review]:
-----------------------------------------------------------------

I actually think we should run the entire graph at the OfflineAudioContext's rate. Sorry, I know that's a big change and maybe difficult, but it seems like the right thing to do.

If you want to take a shortcut and only support OfflineAudioContext with the IdealAudioRate, we can do that for now to get tests working better.
Attachment #749990 - Flags: review?(roc) → review+
Attachment #749989 - Flags: review?(roc) → review+
Comment on attachment 749988 [details] [diff] [review]
Part 12: Fix the lifetime management of the Web Audio graph in presence of OfflineAudioContexts

Review of attachment 749988 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +253,5 @@
>    mStream = graph->CreateAudioNodeStream(engine, MediaStreamGraph::EXTERNAL_STREAM);
>  }
>  
> +void
> +AudioDestinationNode::DestroyGraphIfNeeded()

This seems like a poor name, since you unconditionally destroy the graph.
(Assignee)

Comment 28

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 749987 [details] [diff] [review]
> Part 11: Implement the processing of OfflineAudioContext
> 
> Review of attachment 749987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I actually think we should run the entire graph at the OfflineAudioContext's
> rate. Sorry, I know that's a big change and maybe difficult, but it seems
> like the right thing to do.

Hmm, why do you think that we should do that?  That pretty much breaks everything that depends on the current assumption that the graph runs at 48KHz...

> If you want to take a shortcut and only support OfflineAudioContext with the
> IdealAudioRate, we can do that for now to get tests working better.

No matter what we decide to do eventually, I definitely don't want to make that big of a change here, since we need this sooner rather than later...  We can always do that later, and it should be transparent as far as content is concerned.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> > Comment on attachment 749987 [details] [diff] [review]
> > Part 11: Implement the processing of OfflineAudioContext
> > 
> > Review of attachment 749987 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I actually think we should run the entire graph at the OfflineAudioContext's
> > rate. Sorry, I know that's a big change and maybe difficult, but it seems
> > like the right thing to do.
> 
> Hmm, why do you think that we should do that?  That pretty much breaks
> everything that depends on the current assumption that the graph runs at
> 48KHz...

Why? Using OfflineAudioContext you can specify that the rate is 48KHz, which is actually *more* reliable than assuming AudioContext's rate is 48KHz!

That's one reason I think we should do that; if someone wants processing to happen at a certain rate, we can and should support that in OfflineAudioContext.

> > If you want to take a shortcut and only support OfflineAudioContext with the
> > IdealAudioRate, we can do that for now to get tests working better.
> 
> No matter what we decide to do eventually, I definitely don't want to make
> that big of a change here, since we need this sooner rather than later... 
> We can always do that later, and it should be transparent as far as content
> is concerned.

OK.
(Assignee)

Comment 30

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #28)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> > > Comment on attachment 749987 [details] [diff] [review]
> > > Part 11: Implement the processing of OfflineAudioContext
> > > 
> > > Review of attachment 749987 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I actually think we should run the entire graph at the OfflineAudioContext's
> > > rate. Sorry, I know that's a big change and maybe difficult, but it seems
> > > like the right thing to do.
> > 
> > Hmm, why do you think that we should do that?  That pretty much breaks
> > everything that depends on the current assumption that the graph runs at
> > 48KHz...
> 
> Why? Using OfflineAudioContext you can specify that the rate is 48KHz, which
> is actually *more* reliable than assuming AudioContext's rate is 48KHz!

Well, we make assumptions about IdealAudioRate() all around the code base.  If we want to change that, all of the existing code must be audited to make sure we don't break things.  (At least we should change all callers of IdealAudioRate.)

> That's one reason I think we should do that; if someone wants processing to
> happen at a certain rate, we can and should support that in
> OfflineAudioContext.

Hmm, I guess there's actually a content visible difference for a-rate AudioParams...  I was not paying attention to that. :(

> > > If you want to take a shortcut and only support OfflineAudioContext with the
> > > IdealAudioRate, we can do that for now to get tests working better.
> > 
> > No matter what we decide to do eventually, I definitely don't want to make
> > that big of a change here, since we need this sooner rather than later... 
> > We can always do that later, and it should be transparent as far as content
> > is concerned.
> 
> OK.

To confirm my understanding, do you mean throwing when you try to create an OfflineAudioContext with a different rate for now, or leave the patch as is?  (I slightly prefer to leave it as is)
(Assignee)

Comment 31

4 years ago
Created attachment 750185 [details] [diff] [review]
Part 8: Add a non-realtime media graph API to produce a given number of ticks

(with the nit addressed)
Attachment #749983 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 750186 [details] [diff] [review]
Part 12: Fix the lifetime management of the Web Audio graph in presence of OfflineAudioContexts
Attachment #749988 - Attachment is obsolete: true
Attachment #749988 - Flags: review?(roc)
Attachment #750186 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #30)
> To confirm my understanding, do you mean throwing when you try to create an
> OfflineAudioContext with a different rate for now, or leave the patch as is?
> (I slightly prefer to leave it as is)

Throw, I think.
(Assignee)

Comment 34

4 years ago
(In reply to comment #33)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #30)
> > To confirm my understanding, do you mean throwing when you try to create an
> > OfflineAudioContext with a different rate for now, or leave the patch as is?
> > (I slightly prefer to leave it as is)
> 
> Throw, I think.

OK, I'll do that tomorrow though.  Not feeling well right now...
Attachment #750186 - Flags: review?(roc) → review+
(Assignee)

Comment 35

4 years ago
Created attachment 750399 [details] [diff] [review]
Part 9: Implement the DOM binding for OfflineAudioContext
Attachment #749984 - Attachment is obsolete: true
Attachment #750399 - Flags: review?(roc)
(Assignee)

Comment 36

4 years ago
Created attachment 750400 [details] [diff] [review]
Part 10: Use the non-realtime MediaStreamGraph API and a custom destination node engine for OfflineAudioContext
Attachment #749986 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Created attachment 750401 [details] [diff] [review]
Part 11: Implement the processing of OfflineAudioContext
Attachment #749987 - Attachment is obsolete: true
Attachment #749987 - Flags: review?(roc)
Attachment #750401 - Flags: review?(roc)
Attachment #750399 - Flags: review?(roc) → review+
Comment on attachment 750401 [details] [diff] [review]
Part 11: Implement the processing of OfflineAudioContext

Review of attachment 750401 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +31,5 @@
> +    // These allocations might fail if content provides a huge number of
> +    // channels or size, but it's OK since we'll deal with the failure
> +    // gracefully.
> +    if (mInputChannels.SetLength(aNumberOfChannels)) {
> +      static const fallible_t fallible = fallible_t();

Factoring this out seems pointless. Just use new (fallible_t()).

@@ +108,5 @@
> +      {
> +        // If it's not safe to run scripts right now, schedule this to run later
> +        if (!nsContentUtils::IsSafeToRunScript()) {
> +          nsContentUtils::AddScriptRunner(this);
> +          return NS_OK;

Unnecessary. It's always safe to run script in an nsRunnable dispatched to the main thread.

@@ +165,1 @@
>    uint32_t mLength;

Please document the meaning of mWriteIndex and mLength. And mInputChannels.
Attachment #750401 - Flags: review?(roc) → review+
(Assignee)

Comment 39

4 years ago
(In reply to comment #38)
> @@ +108,5 @@
> > +      {
> > +        // If it's not safe to run scripts right now, schedule this to run later
> > +        if (!nsContentUtils::IsSafeToRunScript()) {
> > +          nsContentUtils::AddScriptRunner(this);
> > +          return NS_OK;
> 
> Unnecessary. It's always safe to run script in an nsRunnable dispatched to the
> main thread.

See bug 865233 comment 8.  :-)
Gah. We should have NS_DispatchToMainThreadWhenSafeToRunScript or something like that. OK.
(Assignee)

Comment 41

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Comment on attachment 750401 [details] [diff] [review]
> Part 11: Implement the processing of OfflineAudioContext
> 
> Review of attachment 750401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/AudioDestinationNode.cpp
> @@ +31,5 @@
> > +    // These allocations might fail if content provides a huge number of
> > +    // channels or size, but it's OK since we'll deal with the failure
> > +    // gracefully.
> > +    if (mInputChannels.SetLength(aNumberOfChannels)) {
> > +      static const fallible_t fallible = fallible_t();
> 
> Factoring this out seems pointless. Just use new (fallible_t()).

That seems to confuse the compiler, since it doesn't know whether we mean to allocate a fallible_t or not. :(
(Assignee)

Comment 42

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Gah. We should have NS_DispatchToMainThreadWhenSafeToRunScript or something
> like that. OK.

Agreed! Filed bug 873292.
(Assignee)

Comment 43

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eccb855886ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67a43c241c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5529434311
https://hg.mozilla.org/integration/mozilla-inbound/rev/35cedd1dd27a
https://hg.mozilla.org/integration/mozilla-inbound/rev/894831c54271
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4f7f3be361
https://hg.mozilla.org/integration/mozilla-inbound/rev/825f0424a603
https://hg.mozilla.org/mozilla-central/rev/eccb855886ad
https://hg.mozilla.org/mozilla-central/rev/b67a43c241c5
https://hg.mozilla.org/mozilla-central/rev/3f5529434311
https://hg.mozilla.org/mozilla-central/rev/35cedd1dd27a
https://hg.mozilla.org/mozilla-central/rev/894831c54271
https://hg.mozilla.org/mozilla-central/rev/7c4f7f3be361
https://hg.mozilla.org/mozilla-central/rev/825f0424a603
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
Depends on: 873553
(Assignee)

Comment 45

4 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Depends on: 910171
Documented. https://developer.mozilla.org/en-US/docs/Web/API/OfflineAudioContext
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.