Closed
Bug 1387454
Opened 7 years ago
Closed 7 years ago
Have multiple MediaStreamGraphs per process, one per audio sampling rate
Categories
(Core :: Audio/Video: MediaStreamGraph, enhancement, P2)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: achronop, Assigned: achronop)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(9 files)
(deleted),
patch
|
padenot
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
padenot
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
padenot
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
padenot
:
review+
|
Details |
Allow to have different instances of MSG running on different threads based on the sample rate. For every different sample rate a new instance of MSG will be created and will be driven by a different instance of AudioCallbackDriver. The cubeb stream will be initialized with the current MSG sample rate.
On the top of that the AudioCotext constructor will be enhanced to accept sample rate as an input. This provides the opportunity to instantiate an AudioContext with the desired sample rate instead of the default. Moreover two or more AudioContext with different sample rate on the same doc is possible. Communication between AudioContext with different sample rate is not implemented yet (it will be the subject of a following bug).
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → achronop
Rank: 15
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8893852 -
Flags: feedback?(padenot)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8893853 -
Flags: feedback?(padenot)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8893854 -
Flags: feedback?(padenot)
Updated•7 years ago
|
Attachment #8893852 -
Attachment is patch: true
Updated•7 years ago
|
Attachment #8893853 -
Attachment is patch: true
Updated•7 years ago
|
Attachment #8893854 -
Attachment is patch: true
Updated•7 years ago
|
Attachment #8893852 -
Flags: feedback?(padenot) → feedback+
Updated•7 years ago
|
Attachment #8893853 -
Flags: feedback?(padenot) → feedback+
Comment 4•7 years ago
|
||
Comment on attachment 8893854 [details] [diff] [review]
samplerate-on-msg WIP
Review of attachment 8893854 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like it would work yeah.
::: dom/locales/en-US/chrome/dom/dom.properties
@@ +108,5 @@
> # LOCALIZATION NOTE: Do not translate MediaStream and createMediaStreamSource.
> MediaStreamAudioSourceNodeCrossOrigin=The MediaStream passed to createMediaStreamSource has a cross-origin resource, the node will output silence.
> MediaLoadExhaustedCandidates=All candidate resources failed to load. Media load paused.
> MediaLoadSourceMissingSrc=<source> element has no “src” attribute. Media resource load failed.
> +MediaStreamAudioSourceNodexWrongRate=Connect nodes with different sample rate.
We'll have to fix this sentence.
::: dom/media/MediaStreamGraph.cpp
@@ +3532,5 @@
> // to the gloabl MediaStreamGraph hashtable. Effectively, this means there is
> // a graph per document and AudioChannel.
>
> + TrackRate sampleRate = aSampleRate ? aSampleRate : CubebUtils::PreferredSampleRate();
> + uint32_t hashkey = ChannelAndWindowToHash(aChannel, aWindow, sampleRate);
We'll have to rename this to be more generic.
::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp
@@ +57,5 @@
> }
>
> + if (aAudioContext.Graph() != aOptions.mMediaStream->GetPlaybackStream()->Graph()) {
> + nsCOMPtr<nsPIDOMWindowInner> pWindow = aAudioContext.GetParentObject();
> + nsIDocument* document = pWindow ? pWindow->GetExtantDoc() : nullptr;
I think we have a thing in webaudioutils.cpp that does all this.
@@ +62,5 @@
> + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> + NS_LITERAL_CSTRING("Media"),
> + document,
> + nsContentUtils::eDOM_PROPERTIES,
> + "MediaStreamAudioSourceNodexWrongRate");
why the x here?
Attachment #8893854 -
Flags: feedback?(padenot) → feedback+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #4)
> Comment on attachment 8893854 [details] [diff] [review]
> samplerate-on-msg WIP
>
> Review of attachment 8893854 [details] [diff] [review]:
> -----------------------------------------------------------------
> why the x here?
It's a typo
This is an assigned P1 bug without activity in two weeks.
If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.
Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Updated•7 years ago
|
Summary: Multi-threaded MediaStreamGraph according to sample rate → Have multiple MediaStreamGraphs per process, one per audio sampling rate
Comment 7•7 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Assignee | ||
Updated•7 years ago
|
Are there any news on the resolution of this issue? Also, could someone shed some light on how exactly this issue blocks bug 934425?
Comment 9•7 years ago
|
||
(In reply to bpantazhs from comment #8)
> Are there any news on the resolution of this issue? Also, could someone shed
> some light on how exactly this issue blocks bug 934425?
Different outputs mean different output clocks. Since we're output-driven, each output must have it's own graph -- and the fun part is dealing with data that crosses graphs (input clocked in sync with output A, run through webaudio (or not) and then routed to media element with an output to output B. The issue is that even if they're the same frequency, you need buffers (due to out-of-sync/size pulls) and to handle clock drift between A and B.
Comment 10•7 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #9)
Thank you for the reply. Is there any documentation available for this?
Comment 11•7 years ago
|
||
See https://webaudio.github.io/web-audio-api/#dom-audiocontextoptions-samplerate
(In reply to Randell Jesup [:jesup] from comment #9)
> -- and the fun part is dealing with data that crosses graphs
Note that that is outside the scope of this bug, as noted in comment 0. We plan to land a version that throws in those cases for now.
Flags: needinfo?(jib)
Assignee | ||
Comment 12•7 years ago
|
||
Back on this one. A try run on rebased patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8b9a467a525f148b43d5308acb3a9b6cdfb46c
A fiddle for basic testing:
https://fiddle.jshell.net/achronop/mdvnukyo/
Assignee | ||
Comment 13•7 years ago
|
||
Could you please review and test fiddle in the comment above. I would like to verify that it covers the basic functionality and I am not missing something.
Flags: needinfo?(padenot)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8950164 [details]
Bug 1387454 - Add AudioContextOptions in webidl.
https://reviewboard.mozilla.org/r/219426/#review225586
Attachment #8950164 -
Flags: review?(padenot) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8950164 -
Flags: review?(bugs)
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8950164 [details]
Bug 1387454 - Add AudioContextOptions in webidl.
https://reviewboard.mozilla.org/r/219426/#review225602
Attachment #8950164 -
Flags: review?(bugs) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8950165 [details]
Bug 1387454 - Set sample rate in AudioContext constructor.
https://reviewboard.mozilla.org/r/219428/#review225588
::: dom/media/webaudio/AudioContext.cpp:208
(Diff revision 1)
> nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports());
> if (!window) {
> aRv.Throw(NS_ERROR_FAILURE);
> return nullptr;
> }
>
Here, limit the rate to the sample-rate range that are valid: https://searchfox.org/mozilla-central/source/dom/media/webaudio/WebAudioUtils.h#38-39
And add a test for that.
::: dom/media/webaudio/AudioContext.cpp:212
(Diff revision 1)
> }
>
> uint32_t maxChannelCount = std::min<uint32_t>(WebAudioUtils::MaxChannelCount,
> CubebUtils::MaxNumberOfChannels());
> RefPtr<AudioContext> object =
> - new AudioContext(window, false,maxChannelCount);
> + new AudioContext(window, false,maxChannelCount,
space after `false,`.
Attachment #8950165 -
Flags: review?(padenot)
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8950167 [details]
Bug 1387454 - Mochitests for non default rate on webaudio and media recorder.
https://reviewboard.mozilla.org/r/219432/#review225584
::: dom/media/tests/mochitest/mochitest.ini:326
(Diff revision 1)
> [test_peerConnection_sender_and_receiver_stats.html]
> skip-if = (android_version == '18') # android(Bug 1189784, timeouts on 4.3 emulator)
> [test_peerConnection_verifyDescriptions.html]
> skip-if = (android_version == '18')
> [test_fingerprinting_resistance.html]
> +[test_audioContextParams_sampleRate.html]
I want tests for:
- Connecting a stream from gUM to a graph with a non-default rate
- Connecting a stream from a PC to a graph with non default rate
- Connecting a graph with non-default rate to a MediaRecorder
- Connecting a graph with non-default rate to a PeerConnection
... that kind of things.
Attachment #8950167 -
Flags: review?(padenot)
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
::: dom/locales/en-US/chrome/dom/dom.properties:107
(Diff revision 1)
> MediaElementAudioSourceNodeCrossOrigin=The HTMLMediaElement passed to createMediaElementSource has a cross-origin resource, the node will output silence.
> # LOCALIZATION NOTE: Do not translate MediaStream and createMediaStreamSource.
> MediaStreamAudioSourceNodeCrossOrigin=The MediaStream passed to createMediaStreamSource has a cross-origin resource, the node will output silence.
> MediaLoadExhaustedCandidates=All candidate resources failed to load. Media load paused.
> MediaLoadSourceMissingSrc=<source> element has no “src” attribute. Media resource load failed.
> +MediaStreamAudioSourceNodexWrongRate=Connect nodes with different sample rate.
"Connecting AudioNodes from AudioContexts with different sample-rate is currently not supported."
::: dom/locales/en-US/chrome/dom/dom.properties:107
(Diff revision 1)
> MediaElementAudioSourceNodeCrossOrigin=The HTMLMediaElement passed to createMediaElementSource has a cross-origin resource, the node will output silence.
> # LOCALIZATION NOTE: Do not translate MediaStream and createMediaStreamSource.
> MediaStreamAudioSourceNodeCrossOrigin=The MediaStream passed to createMediaStreamSource has a cross-origin resource, the node will output silence.
> MediaLoadExhaustedCandidates=All candidate resources failed to load. Media load paused.
> MediaLoadSourceMissingSrc=<source> element has no “src” attribute. Media resource load failed.
> +MediaStreamAudioSourceNodexWrongRate=Connect nodes with different sample rate.
s/xWrong/Different/
::: dom/media/webaudio/AudioDestinationNode.cpp:341
(Diff revision 1)
> nsPIDOMWindowInner* window = aContext->GetParentObject();
> MediaStreamGraph* graph =
> aIsOffline
> ? MediaStreamGraph::CreateNonRealtimeInstance(aSampleRate, window)
> : MediaStreamGraph::GetInstance(
> - MediaStreamGraph::AUDIO_THREAD_DRIVER, window);
> + MediaStreamGraph::AUDIO_THREAD_DRIVER, window, aSampleRate);
Probably not needed.
::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp:61
(Diff revision 1)
>
> if (aAudioContext.CheckClosed(aRv)) {
> return nullptr;
> }
>
> + if (aAudioContext.Graph() != aOptions.mMediaStream->GetPlaybackStream()->Graph()) {
We want to audit our code, at each call site for `MediaStreamGraph::GetInstance`, and determine if it's going to do the right thing.
::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp:68
(Diff revision 1)
> + nsIDocument* document = pWindow ? pWindow->GetExtantDoc() : nullptr;
> + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> + NS_LITERAL_CSTRING("Media"),
> + document,
> + nsContentUtils::eDOM_PROPERTIES,
> + "MediaStreamAudioSourceNodexWrongRate");
s/xWrong/Different/
Attachment #8950166 -
Flags: review?(padenot)
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
> Probably not needed.
Why not? How can we pass sample rate in MSG wihtout it?
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
> We want to audit our code, at each call site for `MediaStreamGraph::GetInstance`, and determine if it's going to do the right thing.
That's the list:
dom/html/HTMLMediaElement.cpp
3720 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStream
3753 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStreamUntilEnded
7576 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::dom::HTMLMediaElement::AudioCaptureStreamChange
dom/media/CanvasCaptureMediaStream.cpp
290 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, aWindow); // found in mozilla::dom::CanvasCaptureMediaStream::CreateSourceStream
dom/media/DOMMediaStream.cpp
562 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, ownerWindow); // found in mozilla::DOMMediaStream::Constructor
dom/media/MediaManager.cpp
1188 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::GetUserMediaStreamRunnable::Run
4154 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::SourceListener::StopSharing
dom/media/webaudio/AudioDestinationNode.cpp
340 : MediaStreamGraph::GetInstance( // found in mozilla::dom::AudioDestinationNode::AudioDestinationNode
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
2378 MediaStreamGraph* graph = MediaStreamGraph::GetInstance( // found in mozilla::PeerConnectionImpl::CreateReceiveTrack
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225990
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225996
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
> That's the list:
>
> dom/html/HTMLMediaElement.cpp
> 3720 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStream
> 3753 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStreamUntilEnded
> 7576 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::dom::HTMLMediaElement::AudioCaptureStreamChange
>
> dom/media/CanvasCaptureMediaStream.cpp
> 290 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, aWindow); // found in mozilla::dom::CanvasCaptureMediaStream::CreateSourceStream
>
> dom/media/DOMMediaStream.cpp
> 562 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, ownerWindow); // found in mozilla::DOMMediaStream::Constructor
>
> dom/media/MediaManager.cpp
> 1188 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::GetUserMediaStreamRunnable::Run
> 4154 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::SourceListener::StopSharing
>
> dom/media/webaudio/AudioDestinationNode.cpp
> 340 : MediaStreamGraph::GetInstance( // found in mozilla::dom::AudioDestinationNode::AudioDestinationNode
>
> media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> 2378 MediaStreamGraph* graph = MediaStreamGraph::GetInstance( // found in mozilla::PeerConnectionImpl::CreateReceiveTrack
About dom/html/HTMLMediaElement.cpp
3720 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStream
3753 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStreamUntilEnded
We can pair these 2 cases which have similar code flow. This does repro when we capture a stream from MediaElement. I could see a benefit to set the sample rate. If the playback stream for example is 16KHz by creating a MSG at that rate we avoid the extra resampling. Otherwise we pay the cost for resampling as we do now. If we decide to update it the biggest chalenge is that the sample rate is availlable (through `HTMLMediaElement::mMediaInfo::mAudio::mRate`) only after metadata loaded event (`HTMLMediaElement::MetadataLoaded()`), so if we get a stream capture request before that we do not really know the sample rate. In that case we should implement some special handling to wait for the event, then create the MSG and do the rest of the capture.
7576 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::dom::HTMLMediaElement::AudioCaptureStreamChange
We reach that method when we do a gUM request using `mediaSource: "audioCapture"` constraints (and we have enabled a pref). Before that MSG has already been created in MediaManager (MediaManager.cpp:1188). So we need to decide first what we will do in MediaManager case and then to make sure that we call `GetInstance()` method here using the same arguments in order to get the already created MSG.
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
> About dom/html/HTMLMediaElement.cpp
>
> 3720 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStream
> 3753 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::dom::HTMLMediaElement::MozCaptureStreamUntilEnded
>
> We can pair these 2 cases which have similar code flow. This does repro when we capture a stream from MediaElement. I could see a benefit to set the sample rate. If the playback stream for example is 16KHz by creating a MSG at that rate we avoid the extra resampling. Otherwise we pay the cost for resampling as we do now. If we decide to update it the biggest chalenge is that the sample rate is availlable (through `HTMLMediaElement::mMediaInfo::mAudio::mRate`) only after metadata loaded event (`HTMLMediaElement::MetadataLoaded()`), so if we get a stream capture request before that we do not really know the sample rate. In that case we should implement some special handling to wait for the event, then create the MSG and do the rest of the capture.
>
> 7576 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::dom::HTMLMediaElement::AudioCaptureStreamChange
>
> We reach that method when we do a gUM request using `mediaSource: "audioCapture"` constraints (and we have enabled a pref). Before that MSG has already been created in MediaManager (MediaManager.cpp:1188). So we need to decide first what we will do in MediaManager case and then to make sure that we call `GetInstance()` method here using the same arguments in order to get the already created MSG.
dom/media/CanvasCaptureMediaStream.cpp
290 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, aWindow); // found in mozilla::dom::CanvasCaptureMediaStream::CreateSourceStream
`CreateSourceStream` is a static method that comes from JS when we call `captureStream()` on a cavas HTML element. There are not many things we can do here. The sample rate is not available. Probably we have to stick with the deafault (preferred device rate).
dom/media/DOMMediaStream.cpp
562 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, ownerWindow); // found in mozilla::DOMMediaStream::Constructor
This is called when we create a new `MediaStream` object in JS. There are 3 constructors:
```
let s = new MediaStream()
let s = new MedisStream(stream);
let s = new MediaStream(track[]);
```
For the first constructor without arguments there are not many things we can do. The case is similar to the previous one.
For the other two cases the code makes use of the existing graph from the provided stream/tracks (DOMMediaStream.cpp#554) and the `GetInstance()` method is never called.
Assignee | ||
Comment 30•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review225580
> dom/media/CanvasCaptureMediaStream.cpp
> 290 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, aWindow); // found in mozilla::dom::CanvasCaptureMediaStream::CreateSourceStream
>
> `CreateSourceStream` is a static method that comes from JS when we call `captureStream()` on a cavas HTML element. There are not many things we can do here. The sample rate is not available. Probably we have to stick with the deafault (preferred device rate).
>
> dom/media/DOMMediaStream.cpp
> 562 MediaStreamGraph::GetInstance(MediaStreamGraph::SYSTEM_THREAD_DRIVER, ownerWindow); // found in mozilla::DOMMediaStream::Constructor
>
> This is called when we create a new `MediaStream` object in JS. There are 3 constructors:
> ```
> let s = new MediaStream()
> let s = new MedisStream(stream);
> let s = new MediaStream(track[]);
> ```
> For the first constructor without arguments there are not many things we can do. The case is similar to the previous one.
>
> For the other two cases the code makes use of the existing graph from the provided stream/tracks (DOMMediaStream.cpp#554) and the `GetInstance()` method is never called.
dom/media/MediaManager.cpp
1188 MediaStreamGraph::GetInstance(graphDriverType, window); // found in mozilla::GetUserMediaStreamRunnable::Run
This is where the MSG is created in a basic gUM request. The most resonable thing to do here would be to use tha sample rate from the constarint but it's not currently supported. Other than that we do not have information about sample rate at that level.
4154 MediaStreamGraph::GetInstance(MediaStreamGraph::AUDIO_THREAD_DRIVER, window); // found in mozilla::SourceListener::StopSharing
This is called at stop of AudioCapture. Since we stop an operating stream the MSG is up and running. We could get that instance from the stream (`mStream::Graph()`) and stop calling the `MediaStreamGraph::GetInstance()` method.
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8950167 [details]
Bug 1387454 - Mochitests for non default rate on webaudio and media recorder.
https://reviewboard.mozilla.org/r/219432/#review225584
> I want tests for:
> - Connecting a stream from gUM to a graph with a non-default rate
> - Connecting a stream from a PC to a graph with non default rate
> - Connecting a graph with non-default rate to a MediaRecorder
> - Connecting a graph with non-default rate to a PeerConnection
>
> ... that kind of things.
gUM and PeerConnection make use of the default rate. In all cases above we will fail to connect graphs of different sample rate. we can veiry though that we fail nicesely. Is that your intention here?
Assignee | ||
Comment 33•7 years ago
|
||
New manual tests according to the comments: https://fiddle.jshell.net/achronop/dwu7u30q/
Assignee | ||
Comment 34•7 years ago
|
||
Assignee | ||
Comment 35•7 years ago
|
||
Assignee | ||
Comment 36•7 years ago
|
||
Assignee | ||
Comment 37•7 years ago
|
||
Assignee | ||
Comment 38•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8963930 [details]
Bug 1387454 - Mochitests for interaction of non default rate with WebRTC.
https://reviewboard.mozilla.org/r/232766/#review238770
Attachment #8963930 -
Flags: review?(padenot) → review+
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8950167 [details]
Bug 1387454 - Mochitests for non default rate on webaudio and media recorder.
https://reviewboard.mozilla.org/r/219432/#review238772
Attachment #8950167 -
Flags: review?(padenot) → review+
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review238778
::: dom/media/MediaStreamGraph.h:1301
(Diff revision 2)
> };
> static const uint32_t AUDIO_CALLBACK_DRIVER_SHUTDOWN_TIMEOUT = 20*1000;
>
> // Main thread only
> - static MediaStreamGraph* GetInstanceIfExists(nsPIDOMWindowInner* aWindow);
> + static MediaStreamGraph* GetInstanceIfExists(nsPIDOMWindowInner* aWindow,
> + TrackRate aSampleRate = 0);
Remove the default argument. Create a constant that is equal to zero, means "default rate" and change all call sites so tht it's explicit everywhere and we don't make mistakes.
::: dom/media/webaudio/MediaStreamAudioSourceNode.cpp:65
(Diff revision 2)
>
> + if (aAudioContext.Graph() != aOptions.mMediaStream->GetPlaybackStream()->Graph()) {
> + nsCOMPtr<nsPIDOMWindowInner> pWindow = aAudioContext.GetParentObject();
> + nsIDocument* document = pWindow ? pWindow->GetExtantDoc() : nullptr;
> + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> + NS_LITERAL_CSTRING("Media"),
indent is off.
Attachment #8950166 -
Flags: review?(padenot)
Comment 48•7 years ago
|
||
mozreview-review |
Comment on attachment 8950165 [details]
Bug 1387454 - Set sample rate in AudioContext constructor.
https://reviewboard.mozilla.org/r/219428/#review238768
::: dom/media/webaudio/AudioContext.cpp:212
(Diff revision 2)
> }
>
> + if (aOptions.mSampleRate > 0 &&
> + (aOptions.mSampleRate - WebAudioUtils::MinSampleRate < 0.0 ||
> + WebAudioUtils::MaxSampleRate - aOptions.mSampleRate < 0.0)) {
> + aRv.Throw(NS_ERROR_INVALID_ARG);
https://webaudio.github.io/web-audio-api/#dom-audiocontextoptions-samplerate says NotSupportedError.
Attachment #8950165 -
Flags: review?(padenot)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•7 years ago
|
||
Comment 54•7 years ago
|
||
mozreview-review |
Comment on attachment 8950165 [details]
Bug 1387454 - Set sample rate in AudioContext constructor.
https://reviewboard.mozilla.org/r/219428/#review239174
::: dom/media/webaudio/AudioContext.cpp:211
(Diff revision 3)
> return nullptr;
> }
>
> + if (aOptions.mSampleRate > 0 &&
> + (aOptions.mSampleRate - WebAudioUtils::MinSampleRate < 0.0 ||
> + WebAudioUtils::MaxSampleRate - aOptions.mSampleRate < 0.0)) {
Put this behind a pref for now. We need more testing before enabling it on the web, but we want to use this in mochitests.
Attachment #8950165 -
Flags: review?(padenot)
Comment 55•7 years ago
|
||
mozreview-review |
Comment on attachment 8950166 [details]
Bug 1387454 - Create a MediaStreamGraph according to the given sample rate.
https://reviewboard.mozilla.org/r/219430/#review239176
Attachment #8950166 -
Flags: review?(padenot) → review+
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8950165 [details]
Bug 1387454 - Set sample rate in AudioContext constructor.
https://reviewboard.mozilla.org/r/219428/#review239178
Of course we'll need to adjust the tests as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 63•7 years ago
|
||
mozreview-review |
Comment on attachment 8965284 [details]
Bug 1387454 - Create preference to enable setting of sample rate in AudioContext.
https://reviewboard.mozilla.org/r/234014/#review239718
::: dom/media/tests/mochitest/head.js:424
(Diff revision 1)
> // If loopback devices are set they will be chosen instead of fakes in gecko.
> ['media.navigator.streams.fake', WANT_FAKE_AUDIO || WANT_FAKE_VIDEO],
> ['media.getusermedia.screensharing.enabled', true],
> ['media.getusermedia.audiocapture.enabled', true],
> - ['media.recorder.audio_node.enabled', true]
> + ['media.recorder.audio_node.enabled', true],
> + ['webaudio.samplerate.enable', true]
"media.webaudio.audiocontextoptions-samplerate.enabled" would be more in line with how we name our prefs.
::: dom/media/webaudio/AudioContext.cpp:210
(Diff revision 1)
> if (!window) {
> aRv.Throw(NS_ERROR_FAILURE);
> return nullptr;
> }
>
> + float sampleRate = 0.0;
Name a constant that is worth 0.0 and use it instead of this hardcoded 0.0.
We want to convey the meaning that 0.0 means "system defaults".
::: modules/libpref/init/all.js:669
(Diff revision 1)
> pref("media.audioipc.stack_size", 65536);
> #else
> pref("media.cubeb.sandbox", false);
> #endif
>
> +// Parallel MediaStreamGraph when sample rate is set in AudioContext
I don't think this comment is very useful.
Attachment #8965284 -
Flags: review?(padenot) → review+
Comment 64•7 years ago
|
||
mozreview-review |
Comment on attachment 8950165 [details]
Bug 1387454 - Set sample rate in AudioContext constructor.
https://reviewboard.mozilla.org/r/219428/#review239724
Attachment #8950165 -
Flags: review?(padenot) → review+
Comment hidden (mozreview-request) |
Comment 66•7 years ago
|
||
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b3a05d5d6216
Add AudioContextOptions in webidl. r=padenot,smaug
https://hg.mozilla.org/integration/autoland/rev/e6ab07a07c3b
Set sample rate in AudioContext constructor. r=padenot
https://hg.mozilla.org/integration/autoland/rev/6e227df35207
Create a MediaStreamGraph according to the given sample rate. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a8e2605332f3
Mochitests for non default rate on webaudio and media recorder. r=padenot
https://hg.mozilla.org/integration/autoland/rev/b33c66f1d444
Mochitests for interaction of non default rate with WebRTC. r=padenot
https://hg.mozilla.org/integration/autoland/rev/2a13706881b8
Create preference to enable setting of sample rate in AudioContext. r=padenot
Comment 67•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b3a05d5d6216
https://hg.mozilla.org/mozilla-central/rev/e6ab07a07c3b
https://hg.mozilla.org/mozilla-central/rev/6e227df35207
https://hg.mozilla.org/mozilla-central/rev/a8e2605332f3
https://hg.mozilla.org/mozilla-central/rev/b33c66f1d444
https://hg.mozilla.org/mozilla-central/rev/2a13706881b8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 68•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Comment 69•6 years ago
|
||
Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/AudioContext
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext
https://developer.mozilla.org/en-US/Firefox/Releases/61
New pages created:
https://developer.mozilla.org/en-US/docs/Web/API/AudioContextOptions
https://developer.mozilla.org/en-US/docs/Web/API/AudioContextOptions/latencyHint
https://developer.mozilla.org/en-US/docs/Web/API/AudioContextOptions/sampleRate
https://developer.mozilla.org/en-US/docs/Web/API/AudioContextLatencyCategory
Let me know if there are any problems with this, otherwise, I'll assume this is complete. Thank you!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•