Closed Bug 1437366 Opened 2 years ago Closed 2 years ago

Sound card sample rate affects mixer.com audio (FF 59 Beta)

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: lpy750, Assigned: padenot)

References

Details

(Keywords: regression)

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180208193705

Steps to reproduce:

Windows 10 64-bit

Not affected: Firefox 58.0.2
Affected: any Firefox 59 beta, Firefox 60 Nightly

In Windows, open soundcard output playback device and set sample rate to either 96000Hz or 192000Hz.

Open mixer.com, find a live stream with Low Latency enabled.



Actual results:

Although running at normal speed, as expected, audio will sound high pitched.



Expected results:

Upon a little experimenting, it appears Firefox 58 relies on "OpenH264 Video Codec provided by Cisco Systems, Inc." for both audio and video, whereas Firefox 59+ only uses it for video.
Component: Untriaged → Audio/Video
Product: Firefox → Core
Hi. Thanks for the report! Sounds like another AAC container-vs-stream samplerate issue. Does it happen when the playback device set to a lower sample rate, like 48 or 44.1 kHz?

Jean-Yves, does this match up with any existing bugs? Looks like we have a 59 regression.
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(jyavenard)
Priority: -- → P2
Hi there, at 44.1kHz or 48kHz playback is fine. At 88.2kHz or above it is high pitched/sounds just like "the chipmunks".

Tested on two devices with different hardware... a PC and tablet running Windows 10, and both did it with FF 59 but not FF 58.

As mentioned, I can easily reproduce it every time on www.mixer.com with a live stream that has "Low Latency" enabled, but websites such as YouTube aren't affected.
It could be bug 1431169, it's the only thing changing the AAC decoder, if that's what it is.

Could you try using the mozregression tool to determine what caused the problem for You?

Thanknyou
Flags: needinfo?(jyavenard) → needinfo?(lpy750)
Oh my bad, this is on Windows... Another cubeb error then.
(In reply to lpy750 from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0)
> Gecko/20100101 Firefox/59.0
> Build ID: 20180208193705
> 
> Steps to reproduce:
> 
> Windows 10 64-bit
> 
> Not affected: Firefox 58.0.2
> Affected: any Firefox 59 beta, Firefox 60 Nightly
> 
> In Windows, open soundcard output playback device and set sample rate to
> either 96000Hz or 192000Hz.
> 
> Open mixer.com, find a live stream with Low Latency enabled.
> 
> 
> 
> Actual results:
> 
> Although running at normal speed, as expected, audio will sound high pitched.
> 
> 
> 
> Expected results:
> 
> Upon a little experimenting, it appears Firefox 58 relies on "OpenH264 Video
> Codec provided by Cisco Systems, Inc." for both audio and video, whereas
> Firefox 59+ only uses it for video.
Damn, it validated too quickly 
 
> Expected results:
> 
> Upon a little experimenting, it appears Firefox 58 relies on "OpenH264 Video
> Codec provided by Cisco Systems, Inc." for both audio and video, whereas
> Firefox 59+ only uses it for video.

Not at all. Openh264 is only used for webrtc session with peer requiring h264, e.g no Web browsers
I see. My observation is, if I try a Mixer.com low latency stream with the H264 plugin disabled, it will fail to load in Firefox 58. But in Firefox 59, only the video fails to load but the audio will still work.
Flags: needinfo?(lpy750)
(In reply to lpy750 from comment #7)
> I see. My observation is, if I try a Mixer.com low latency stream with the
> H264 plugin disabled, it will fail to load in Firefox 58. But in Firefox 59,
> only the video fails to load but the audio will still work.

It must be an entire coincidence. The OpenH264 plugin can not be used to decode normal playback video
Assignee: nobody → jyavenard
mixer.com, when using low-latency mode, uses WebRTC for transport, and that uses OpenH264.

This is easy to check: open a "low-latency" stream, open "about:webrtc", see all the PeerConnection in use, with video flowing.
And why would it plays incorrectly when changing the system sampling rate, nevertheless, it's a regression
Yes. jya, do you have time to have a look, or do you want me to take it? I think the issue is in the area of the AudioConduit.
Taking as per discussion with jya.

Also, I haven't been able to test this: my windows builder is currently busted. However, this corresponds to the symptoms: wrong rate (i.e. high pitched audio) + twice less data than needed, so the audio sounds really weird.

Having the wrong rate on the track here means that the MSG will not resample its input, and consider that we're under-runing like crazy instead.
Assignee: jyavenard → padenot
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review230706
Attachment #8955228 - Flags: review?(jyavenard)
This helps, but is not good enough.
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review230712

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1970
(Diff revision 1)
>      , mMaybeTrackNeedsUnmute(true)
>    {
>      MOZ_RELEASE_ASSERT(mSource, "Must be used with a SourceMediaStream");
> +  }
> +
> +  virtual ~GenericReceiveListener()

that move seems out of scope to me
Attachment #8955228 - Flags: review+
I don't quite understand what is going on here. The patch above is valid, but then I only hear audio sporadically. It sounds like it's under-running somewhere else.
(In reply to Paul Adenot (:padenot) from comment #17)
> I don't quite understand what is going on here. The patch above is valid,
> but then I only hear audio sporadically. It sounds like it's under-running
> somewhere else.

For the record it was that the `aDesiredTime` number in mozilla::MediaPipelineReceiveAudio::PipelineListener::NotifyPullImpl was assumed to be in the right sample-rate domain, since we assumed that the graph rate was OK to use in the MediaPipeline (which isn't the case), so it was completely off.
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review230988

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1978
(Diff revision 3)
> +    MOZ_ASSERT((aRate != 0 && mTrack->AsAudioStreamTrack()) ||
> +               mTrack->AsVideoStreamTrack());

Move the assert to the top of the `->AsAudioStreamTrack()` block
Attachment #8955228 - Flags: review?(apehrson) → review+
Comment on attachment 8956085 [details]
Bug 1437366 - Teach the AudioConduit to send audio that has a non-supported sample-rate.

https://reviewboard.mozilla.org/r/225018/#review230992

r+ with these minor issues and nits fixed.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:497
(Diff revision 2)
>    {
>      MOZ_ASSERT(mConduit);
>      MOZ_COUNT_CTOR(AudioProxyThread);
>    }
>  
> +  uint32_t AppropriateSendingRateForInputRate(uint32_t aInputRate)

Could we move this to AudioSessionConduit so the definition of what is supported lives in one place?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:497
(Diff revision 2)
>    {
>      MOZ_ASSERT(mConduit);
>      MOZ_COUNT_CTOR(AudioProxyThread);
>    }
>  
> +  uint32_t AppropriateSendingRateForInputRate(uint32_t aInputRate)

Please document this so we know what it's intended to do.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:504
(Diff revision 2)
> +    if (aInputRate < 16000) {
> +      return 16000;
> +    } else if (aInputRate < 32000) {
> +      return 32000;
> +    } else if (aInputRate < 44100) {

You want these to be `<=` no?

Or an `aInputRate` of `16000` will result in 32000.

Fine that those rates are covered by `IsSamplingFreqSupported()` above, but that is not obvious here.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:515
(Diff revision 2)
> +    } else {
> +      return 48000;
> +    }
> +  }
> +
>    void InternalProcessAudioChunk(TrackRate rate,

Please document this method so we know what it's intended to do.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:517
(Diff revision 2)
> +    }
> +  }
> +
>    void InternalProcessAudioChunk(TrackRate rate,
>                                   const AudioChunk& chunk,
> -                                 bool enabled)
> +                                 bool aEnabled)

If you're fixing `aEnabled`, could you do `rate` and `chunk` too?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:545
(Diff revision 2)
> +    if (mInterleavedAudio.Length() < sampleCount) {
> +      mInterleavedAudio.SetLength(sampleCount);
> +    }

See also SetCapacity

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:584
(Diff revision 2)
> +    if (framesProcessed != 0) {
> +      processedAudio = inputAudio;
> +    } else {
> +      // In place conversion not possible, use a buffer.

IMO this gets easier to read if you put the block that the conditional is for (checking framesProcessed against 0) first.

The bonus of avoiding negations helps too.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:601
(Diff revision 2)
> +                     transmissionRate,
> +                     outputChannels,
> +                     framesProcessed);
> +  }
>  
> -    // Check if the rate or the number of channels has changed since the last
> +  void PacketizeAndSend(const int16_t* aAudioData,

Can you document this method so we know what it's intended to do?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:604
(Diff revision 2)
> +  }
>  
> -    // Check if the rate or the number of channels has changed since the last
> -    // time we came through. I realize it may be overkill to check if the rate
> -    // has changed, but I believe it is possible (e.g. if we change sources) and
> -    // it costs us very little to handle this case.
> +  void PacketizeAndSend(const int16_t* aAudioData,
> +                        uint32_t aRate,
> +                        uint32_t aChannels,
> +                        uint32_t aFrameCount) {

Opening bracket on new line

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:613
(Diff revision 2)
>  
> -    uint32_t audio_10ms = rate / 100;
> +    uint32_t audio_10ms = aRate / 100;
>  
>      if (!mPacketizer || mPacketizer->PacketSize() != audio_10ms ||
> -        mPacketizer->Channels() != outputChannels) {
> +        mPacketizer->Channels() != aChannels) {
>        // It's ok to drop the audio still in the packetizer here.

Normally I'd ask you to clarify *why* this is ok, but since this line is not touched I'll consider it optional.
Attachment #8956085 - Flags: review?(apehrson) → review+
Comment on attachment 8956086 [details]
Bug 1437366 - Add a way to force the sample-rate used for a MediaStreamGraph.

https://reviewboard.mozilla.org/r/225020/#review231004

Looks good, but please make sure to get the behavior around resetting the pref, and `PreferredSampleRate()` right.

::: dom/media/CubebUtils.cpp:37
(Diff revision 2)
>  #define PREF_VOLUME_SCALE "media.volume_scale"
>  #define PREF_CUBEB_BACKEND "media.cubeb.backend"
>  #define PREF_CUBEB_LATENCY_PLAYBACK "media.cubeb_latency_playback_ms"
>  #define PREF_CUBEB_LATENCY_MSG "media.cubeb_latency_msg_frames"
> +// Allows to get something non-default for the preferred sample-rate, to allow
> +// troubleshotting in the field and testing.

s/shotting/shooting/

::: dom/media/CubebUtils.cpp:128
(Diff revision 2)
> +// If sCubebForcedSampleRate is non-zero, PreferredSampleRate will return the
> +// preferred sample-rate for the audio backend in use. Otherwise, it will be
> +// used as the preferred sample-rate.

I think you need to flip the cases around here, no?

> If it's non-zero it will be used as the preferred rate. Otherwise, PreferredSampleRate() will return the rate for the backend in use.

::: dom/media/CubebUtils.cpp:247
(Diff revision 2)
> +  } else if (strcmp(aPref, PREF_CUBEB_FORCE_SAMPLE_RATE) == 0) {
> +    if (Preferences::HasUserValue(aPref)) {
> +      StaticMutexAutoLock lock(sMutex);
> +      sCubebForcedSampleRate = Preferences::GetUint(aPref, 48000);
> +    }

What does it mean if it doesn't have user value -- shouldn't we reset the forced rate to 0?

It seems sound to me to rely on Preferences::GetUint() to fall back to 0 in case of an error (like there not being a user value).

Any reason for defaulting to 48k instead of 0 here?

::: dom/media/CubebUtils.cpp:336
(Diff revision 2)
>  uint32_t PreferredSampleRate()
>  {
>    if (!InitPreferredSampleRate()) {
>      return 44100;
>    }
>    MOZ_ASSERT(sPreferredSampleRate);
> +  if (sCubebForcedSampleRate) {
> +    return sCubebForcedSampleRate;
> +  }

It seems logical to me that the forced rate should be returned even if InitPreferredSampleRate() fails.

::: dom/media/tests/mochitest/test_forceSampleRate.html:10
(Diff revision 2)
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +</div>

Needed?

::: dom/media/tests/mochitest/test_forceSampleRate.html:23
(Diff revision 2)
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [
> +  ["media.cubeb.force_sample_rate", WEIRD_SAMPLE_RATE]
> +]}).then(function() {
> +  var ac = new AudioContext();
> +  is(ac.sampleRate, WEIRD_SAMPLE_RATE, "Forced sample-rate set successfuly.");

s/successfuly/successfully/
Attachment #8956086 - Flags: review?(apehrson) → review+
Comment on attachment 8956087 [details]
Bug 1437366 - Test that sending and receiving audio when using a graph with a sample-rate that is not supported by the MediaPipeline code works.

https://reviewboard.mozilla.org/r/225022/#review231010

r- for finishing the test early.

::: dom/media/tests/mochitest/mochitest.ini:30
(Diff revision 2)
>    !/dom/media/test/gizmo.mp4
>  
>  [test_a_noOp.html]
>  [test_dataChannel_basicAudio.html]
> +[test_peerConnection_basicAudio_forcedSampleRate.html]
>  skip-if = (android_version == '18') # Bug 962984 for debug, bug 963244 for opt

The `skip-if` applies to the line above does it not?

Please double check this with the try results.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forcedSampleRate.html:16
(Diff revision 2)
> +    title: "Basic audio-only peer connection, with the MSG running at a rate not supported by the MediaPipeline"
> +  });
> +
> +// The MediaPipeline supports 16, 32, 44.1 and 48kHz, test something below and
> +// above those numbers;
> +[96000, 24000].forEach(function(forcedSampleRate) {

When we've run this function for the first element (96k), the test framework will consider the test done and move on to the next, no?

I always had to override the finish() call when doing something like this before. One example is [1] where we do `test.chain.execute()` instead of `test.run()` to avoid finishing early.


[1] https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_capturedVideo.html

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forcedSampleRate.html:25
(Diff revision 2)
> +    ]
> +  }).then(function () {
> +    var test;
> +    runNetworkTest(function (options) {
> +      test = new PeerConnectionTest(options);
> +      test.audioContext = new AudioContext();

do you need to have the audio context as an attribute on `test`?

It should be kept alive through the MediaStream and the stream destination node, no?

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forcedSampleRate.html:26
(Diff revision 2)
> +  }).then(function () {
> +    var test;
> +    runNetworkTest(function (options) {
> +      test = new PeerConnectionTest(options);
> +      test.audioContext = new AudioContext();
> +      test.setMediaConstraints([{ audio: true }], [{ audio: true }]);

This will cause the test framework to set up one gUM on pcLocal (which you replace) and one gUM on pcRemote.

Make the second array empty to make the test case a bit smaller.
Attachment #8956087 - Flags: review?(apehrson) → review-
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review231314

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1978
(Diff revision 3)
> +    MOZ_ASSERT((aRate != 0 && mTrack->AsAudioStreamTrack()) ||
> +               mTrack->AsVideoStreamTrack());

!? this is clearly an invariant for the whole method.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1978
(Diff revision 3)
> +    MOZ_ASSERT((aRate != 0 && mTrack->AsAudioStreamTrack()) ||
> +               mTrack->AsVideoStreamTrack());

!? this is clearly an invariant for the whole method.
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review231314

> !? this is clearly an invariant for the whole method.

Sure, but you only assert the rate if it's audio, and we assert the track type further down in the method.
Comment on attachment 8956085 [details]
Bug 1437366 - Teach the AudioConduit to send audio that has a non-supported sample-rate.

https://reviewboard.mozilla.org/r/225018/#review230992

> Could we move this to AudioSessionConduit so the definition of what is supported lives in one place?

AudioSessionConduit does not want to know about this.

This is an implementation concern of the MediaPipeline. AudioSessionConduit still only supports a few rates.

> You want these to be `<=` no?
> 
> Or an `aInputRate` of `16000` will result in 32000.
> 
> Fine that those rates are covered by `IsSamplingFreqSupported()` above, but that is not obvious here.

It's now clear with the documentations.

> See also SetCapacity

?

> Normally I'd ask you to clarify *why* this is ok, but since this line is not touched I'll consider it optional.

Fixed regardless.
Comment on attachment 8956085 [details]
Bug 1437366 - Teach the AudioConduit to send audio that has a non-supported sample-rate.

https://reviewboard.mozilla.org/r/225018/#review231336
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review231314

> !? this is clearly an invariant for the whole method.

You're right, removed the other one. I prefer the invariants stated in one go at the top.
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

https://reviewboard.mozilla.org/r/224388/#review231314

> You're right, removed the other one. I prefer the invariants stated in one go at the top.

Personally I prefer to make the asserts as individual as possible. Makes it easier to debug when they fail. Could you make one checking the track type, and then one `MOZ_ASSERT_IF(isAudio, aRate != 0)`?
Comment on attachment 8956087 [details]
Bug 1437366 - Test that sending and receiving audio when using a graph with a sample-rate that is not supported by the MediaPipeline code works.

https://reviewboard.mozilla.org/r/225022/#review231346

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forcedSampleRate.html:16
(Diff revision 2)
> +    title: "Basic audio-only peer connection, with the MSG running at a rate not supported by the MediaPipeline"
> +  });
> +
> +// The MediaPipeline supports 16, 32, 44.1 and 48kHz, test something below and
> +// above those numbers;
> +[96000, 24000].forEach(function(forcedSampleRate) {

It occured to me last night that this is a bit more complicated. It's best to do both tests in two documents, because we have an MSG per documents (for now), and I don't want to innadvertently run into a sitution where an AudioContext with a certain rate runs on an MSG with a different rate. That would be very bad.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Rank: 15
Component: Audio/Video: Playback → Audio/Video: MediaStreamGraph
Comment on attachment 8956087 [details]
Bug 1437366 - Test that sending and receiving audio when using a graph with a sample-rate that is not supported by the MediaPipeline code works.

https://reviewboard.mozilla.org/r/225022/#review231700

::: dom/media/tests/mochitest/peerconnection_audio_forced_sample_rate.js:8
(Diff revision 3)
> +// the MediaPipeline can work with.
> +// It is in a separate file because we have an MSG per document, and we want to
> +// test multiple sample-rates, so we include it in multiple HTML mochitest
> +// files.
> +function test_peerconnection_audio_forced_sample_rate(forcedSampleRate) {
> +  SpecialPowers.pushPrefEnv({

SimpleTest might not yet be available.

https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/media/tests/mochitest/pc.js#2099

And I'd suggest using pushPrefs instead, https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/media/tests/mochitest/head.js#378

::: dom/media/tests/mochitest/peerconnection_audio_forced_sample_rate.js:13
(Diff revision 3)
> +  SpecialPowers.pushPrefEnv({
> +    "set": [
> +      ["media.cubeb.force_sample_rate", forcedSampleRate]
> +    ]
> +  }).then(function () {
> +    var test;

Unnecessary

::: dom/media/tests/mochitest/peerconnection_audio_forced_sample_rate.js:16
(Diff revision 3)
> +    ]
> +  }).then(function () {
> +    var test;
> +    runNetworkTest(function (options) {
> +      test = new PeerConnectionTest(options);
> +      var ac = new AudioContext();

Use `let` instead of `var`? Applies throughout

::: dom/media/tests/mochitest/peerconnection_audio_forced_sample_rate.js:31
(Diff revision 3)
> +          test.pcLocal.attachLocalStream(dest.stream);
> +        }
> +      ]);
> +      test.chain.append([
> +        function CHECK_REMOTE_AUDIO_FLOW(test) {
> +          return test.pcRemote.checkReceivingToneFrom(new AudioContext(), test.pcLocal);

You could pass in `ac` here.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forced_lower_rate.html:15
(Diff revision 3)
> +  createHTML({
> +    bug: "1437366",
> +    title: "Basic audio-only peer connection, with the MSG running at a rate not supported by the MediaPipeline (24000Hz)"
> +  });
> +
> +test_peerconnection_audio_forced_sample_rate(24000);

Indentation
Attachment #8956087 - Flags: review?(apehrson) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/41bb4a676575
Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation. r=jya,pehrsons
https://hg.mozilla.org/integration/autoland/rev/abca0eb36d33
Teach the AudioConduit to send audio that has a non-supported sample-rate. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/a26852df062b
Add a way to force the sample-rate used for a MediaStreamGraph. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/57826a5b03e8
Test that sending and receiving audio when using a graph with a sample-rate that is not supported by the MediaPipeline code works. r=pehrsons
Comment on attachment 8960259 [details]
Bug 1437366 - Lower the sample rate so that the test don't underrun on Windows on try.

https://reviewboard.mozilla.org/r/229018/#review234828

Sadness. I wonder how long/well this will hold. But for the short term it should be all right.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio_forced_higher_rate.html:12
(Diff revision 1)
>  <body>
>  <pre id="test">
>  <script type="application/javascript">
>    createHTML({
>      bug: "1437366",
>      title: "Basic audio-only peer connection, with the MSG running at a rate not supported by the MediaPipeline (96000Hz)"

Also here, s/96/49/
Attachment #8960259 - Flags: review?(apehrson) → review+
Comment on attachment 8960260 [details]
Bug 1437366 - Disable the test on Android, all the others are, there are now ways this one can pass.

https://reviewboard.mozilla.org/r/229020/#review234836
Attachment #8960260 - Flags: review?(apehrson) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/408c4c89868d
Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation. r=jya,pehrsons
https://hg.mozilla.org/integration/autoland/rev/66aa57290ce6
Teach the AudioConduit to send audio that has a non-supported sample-rate. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/dfabe184b367
Add a way to force the sample-rate used for a MediaStreamGraph. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/b36537851ae1
Test that sending and receiving audio when using a graph with a sample-rate that is not supported by the MediaPipeline code works. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/1bc2b82918bd
Lower the sample rate so that the test don't underrun on Windows on try. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/f1ec577cb636
Disable the test on Android, all the others are, there are now ways this one can pass. r=pehrsons
clearing ni.
Flags: needinfo?(padenot)
Duplicate of this bug: 1428528
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

Approval Request Comment
[Feature/Bug causing the regression]: 
[User impact if declined]: Sites using WebRTC with inbound audio or outbound audio (or both), most of them don't work if the sample rate set on the input or output devices is not in [16k, 32k, 44.1k, 48k]. This appears to be a non-trivial number of desktop users, and some OSX users (the built-in sound card on OSX can be set to higher rates).

[Is this code covered by automated tests?]: Yes, and we've had multiple independent reports of things working as expected on multiple web apps.

[Has the fix been verified in Nightly?]: Yes, baked for a while no intermittents

[Needs manual test from QE? If yes, steps to reproduce]: I don't think it is necessary because it's properly unit tested, but here is how to do it:

Find a machine that can have a default sample-rate (input and output audio devices) of more than 48000Hz (96000Hz would be the next usual rate). Set this to be the default sample-rate. This can be a Windows desktop or laptop with an external sound card, or simply an Apple computer (any will do). It's also possible to set the sample-rate on Linux, regardless of the device.

Run Firefox. In the devtools console, verify that `(new AudioContext).sampleRate` returns the rate set in the previous step.

Make a WebRTC call using any service. This used to produce terrible glitchy audio, and with this patch set, the audio is clear.

[List of other uplifts needed for the feature/fix]: None, but all the patches in this bug (covering respectively inbound audio, outbound audio, and tests).

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It's been working well for everybody involved, is straightforward code and well tested, so I don't think so.

[String changes made/needed]: None.

This uplift request is motivated by the fact that 60 is an ESR release, and that we've had external request of having this scenario work well for the upcoming ESR cycle.
Attachment #8955228 - Flags: approval-mozilla-beta?
Comment on attachment 8955228 [details]
Bug 1437366 - Set the correct (possibly clamped) rate on the MediaStreamTrack when the MSG runs at a rate not compatible with the webrtc.org code, and fix interval calculation.

webrtc fix, approved for 60.0b14 (all patches in this bug)
Attachment #8955228 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1456159
You need to log in before you can comment on or make changes to this bug.