Closed Bug 1213414 Opened 4 years ago Closed 2 years ago

Implement channelCount audio constraint

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox44 --- affected
firefox56 --- fixed
Blocking Flags:

People

(Reporter: jib, Assigned: achronop)

References

()

Details

Attachments

(2 files)

Spec[1]: "The number of independent channels of sound that the audio data contains, i.e. the number of audio samples per sample frame."

Where it is [2].

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#sec-track-properties
[2] https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb.c?from=cubeb_get_max_channel_count#161
Summary: Implement channelCount constraint → Implement channelCount audio constraint
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
Rank: 25 → 21
Up-prioritizing this for stereo.
Assignee: nobody → achronop
Rank: 21 → 15
Priority: P2 → P1
Comment on attachment 8878008 [details]
Bug 1213414 - Add channelCount constraint in webidl file.

https://reviewboard.mozilla.org/r/149422/#review154468

Make sure you get a DOM reviewer (e.g. smaug) on this webidl patch before landing, or it will bounce.
Attachment #8878008 - Flags: review?(jib) → review+
Comment on attachment 8878008 [details]
Bug 1213414 - Add channelCount constraint in webidl file.

https://reviewboard.mozilla.org/r/149422/#review154472
Attachment #8878008 - Flags: review?(padenot) → review+
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review154474
Attachment #8878009 - Flags: review?(padenot) → review+
Comment on attachment 8878008 [details]
Bug 1213414 - Add channelCount constraint in webidl file.

https://reviewboard.mozilla.org/r/149422/#review154476
Attachment #8878008 - Flags: review+
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review154470

Looks like the right direction! I have nits and questions as I'm not familiar with all the code.

::: dom/media/MediaManager.cpp:1801
(Diff revision 1)
>    mPrefs.mAgc          = 0;
>    mPrefs.mNoise        = 0;
>  #endif
>    mPrefs.mPlayoutDelay = 0;
>    mPrefs.mFullDuplex = false;
> +  mPrefs.mChannels = 0;

Maybe add a comment here what 0 means, e.g. like we do for mWidth and mHeight above?

::: dom/media/MediaManager.cpp:2910
(Diff revision 1)
>    GetPref(aBranch, "media.getusermedia.agc", aData, &mPrefs.mAgc);
>    GetPref(aBranch, "media.getusermedia.noise", aData, &mPrefs.mNoise);
>    GetPref(aBranch, "media.getusermedia.playout_delay", aData, &mPrefs.mPlayoutDelay);
>    GetPrefBool(aBranch, "media.getusermedia.aec_extended_filter", aData, &mPrefs.mExtendedFilter);
>    GetPrefBool(aBranch, "media.getusermedia.aec_aec_delay_agnostic", aData, &mPrefs.mDelayAgnostic);
> +  GetPref(aBranch, "media.getusermedia.channels", aData, &mPrefs.mChannels);

This looks like a new pref. Should we add the same machinery for updating it live, for parity with the others?

E.g. mirror https://dxr.mozilla.org/mozilla-central/search?q=media.getusermedia.aec&redirect=false

::: dom/media/MediaStreamGraph.h:834
(Diff revision 1)
> +  bool ResetGraphDriver(AudioDataListener *aListener);
> +
>    // XXX need a Reset API

Is this the Reset API? If so, can we remove the comment?

::: dom/media/MediaStreamGraph.cpp:3182
(Diff revision 1)
> +bool
> +SourceMediaStream::ResetGraphDriver(AudioDataListener * aListener)
> +{
> +  AudioCallbackDriver* nextDriver = new AudioCallbackDriver(GraphImpl());
> +  nextDriver->SetInputListener(aListener);
> +  {
> +    MonitorAutoLock lock(GraphImpl()->GetMonitor());
> +    GraphImpl()->CurrentDriver()->SwitchAtNextIteration(nextDriver);

I'll rely on Paul to review this part, though it looks good to me. I assume the context here makes it ok to always switch in an AudioCallbackDriver?

I see other places where we do this [1] where we're checking state more, even using other drivers. Should we at least assert some state here maybe?

[1] https://dxr.mozilla.org/mozilla-central/rev/75be6742abb94d79f3bcb731ab7fafa3d42ac4da/dom/media/MediaStreamGraph.cpp#546

::: dom/media/webrtc/MediaEngineWebRTC.h:172
(Diff revision 1)
> -  explicit AudioInputCubeb(webrtc::VoiceEngine* aVoiceEngine, int aIndex = 0) :
> -    AudioInput(aVoiceEngine), mSelectedDevice(aIndex), mInUseCount(0)
> +  explicit AudioInputCubeb(webrtc::VoiceEngine* aVoiceEngine, int aIndex = 0)
> +  : AudioInput(aVoiceEngine)
> +  , mSelectedDevice(aIndex)
> +  , mInUseCount(0)

Generally we want to avoid pure format changes in unrelated code; makes blame harder to follow.

::: dom/media/webrtc/MediaEngineWebRTC.h:278
(Diff revision 1)
> +    if (sUserChannelCount == 0) {
> -    return GetDeviceMaxChannels(aDeviceIndex, aChannels);
> +      return GetDeviceMaxChannels(aDeviceIndex, aChannels);
> -  }
> +    }
>  
> +    if (GetDeviceMaxChannels(aDeviceIndex, aChannels)) {
> +      return 1; // error
> +    }
> +
> +    if (sUserChannelCount < aChannels) {
> +      aChannels = sUserChannelCount;
> +    }
> +
> +    return 0;

Looks like this boils down to:

    if (GetDeviceMaxChannels(aDeviceIndex, aChannels)) {
      return 1; // error
    }

    if (sUserChannelCount && sUserChannelCount < aChannels) {
      aChannels = sUserChannelCount;
    }
    return 0;

::: dom/media/webrtc/MediaEngineWebRTC.h:307
(Diff revision 1)
> +  int SetUserChannelCount(uint32_t aChannels)
> +  {
> +    sUserChannelCount = aChannels;
> +    return 0;
> +  }

Any reason not to have this function return void?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:292
(Diff revision 1)
>  
>    MediaEnginePrefs prefs = aPrefs;
>    prefs.mAecOn = c.mEchoCancellation.Get(prefs.mAecOn);
>    prefs.mAgcOn = c.mAutoGainControl.Get(prefs.mAgcOn);
>    prefs.mNoiseOn = c.mNoiseSuppression.Get(prefs.mNoiseOn);
> +  prefs.mChannels = c.mChannelCount.Get(prefs.mChannels);

So one gotcha here is the impedance mismatch between the internal overload of the value 0 with the special meaning of "device max" and the constraints algorithm.

Users are allowed to constrain numerals using min-max ranges, so if my microphone supports stereo but nothing more, then the real default to input into the constraints algorithm would be 2 here, not 0. E.g. if a site constrains my stereo mic like this without an ideal value:

    track.applyConstraints({ channelCount: { min: 1 }});

then the algorithm should produce 2 (since min and max are basically range-clamps), whereas here it looks like it will produce 1, which is not right.

This'll come up again once we add throwing of OverconstrainedError (which we discussed doing in a second patch), since a 0 would throw OverconstrainedError above.

Is there a way to know the actual device max here?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:319
(Diff revision 1)
>        }
> -      if (!AllocChannel()) {
> +      if (mAudioInput->SetRecordingDevice(mCapIndex)) {
> -        LOG(("Audio device is not initalized"));
> -        return NS_ERROR_FAILURE;
> +         return NS_ERROR_FAILURE;
>        }
> -      if (mAudioInput->SetRecordingDevice(mCapIndex)) {
> +      MOZ_ASSERT(prefs.mChannels >= 0);

Another gotcha is the mChannelCount.Get method is just a plain range clamper, so we could actually see negative values here. We probably want to clamp values here, rather than assert, otherwise the following would crash a debug build:

    track.applyConstraints({ channelCount: { max: -1 }});

I also see some code duplication of this below. Could we hoist it out of the switch?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:320
(Diff revision 1)
> +      if (mAudioInput->SetUserChannelCount(prefs.mChannels)) {
> +         return NS_ERROR_FAILURE;

Can this ever fail? Applies throughout.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:330
(Diff revision 1)
> +        LOG(("Audio device is not initalized"));
>          return NS_ERROR_FAILURE;
>        }
>        LOG(("Audio device %d allocated", mCapIndex));
> +      {
> +        // Applied channelCount might not be the requested,

the requested what? I'm not following. I'm a bit confused by the code that follows. Seems like we're just reading back the value we just set above with SetUserChannelCount. When would this not be the case? Seems redundant, unless I'm missing something.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:334
(Diff revision 1)
> +      {
> +        // Applied channelCount might not be the requested,
> +        // this will be stored later in settings.
> +        uint32_t channelCount = 0;
> +        if (mAudioInput->GetChannelCount(mCapIndex, channelCount) == 0) {
> +          MOZ_ASSERT(channelCount > 0 && channelCount <= 2);

Can channelCount never be higher than 2? What prevents this assert from hitting if a site passes in { channelCount: 3 } ?

If it's an internal limitation, should this assert be inside GetChannelCount() instead?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:347
(Diff revision 1)
>          return NS_OK;
>        }
> +
> +      if (prefs.mChannels != mLastPrefs.mChannels) {
> +        MOZ_ASSERT(mSources.Length() > 0);
> +        SourceMediaStream * source = mSources.LastElement().get();

Keep our head down in the war on raw pointers:

    auto& source = mSources.LastElement();

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:352
(Diff revision 1)
> +        webrtc::CodecInst codec;
> +        strcpy(codec.plname, ENCODING);
> +        codec.channels = CHANNELS;

CHANNELS is 1? When exactly would this default kick in?

https://dxr.mozilla.org/mozilla-central/rev/fe809f57bf2287bb937c3422ed03a63740b3448b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#19
Attachment #8878009 - Flags: review?(jib) → review-
> Is this the Reset API? If so, can we remove the comment?

It is not, just an unfortunate name collision. Maybe we should rename the function that is being added instead, because it's just moving to a new driver, not really resetting stuff..
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review154470

> This looks like a new pref. Should we add the same machinery for updating it live, for parity with the others?
> 
> E.g. mirror https://dxr.mozilla.org/mozilla-central/search?q=media.getusermedia.aec&redirect=false

We can make it but I'm not sure if we want it, maybe Paul has a better opinion about it.

> Is this the Reset API? If so, can we remove the comment?

It replaced with name `OpenNewAudioCallbackDriver`

> So one gotcha here is the impedance mismatch between the internal overload of the value 0 with the special meaning of "device max" and the constraints algorithm.
> 
> Users are allowed to constrain numerals using min-max ranges, so if my microphone supports stereo but nothing more, then the real default to input into the constraints algorithm would be 2 here, not 0. E.g. if a site constrains my stereo mic like this without an ideal value:
> 
>     track.applyConstraints({ channelCount: { min: 1 }});
> 
> then the algorithm should produce 2 (since min and max are basically range-clamps), whereas here it looks like it will produce 1, which is not right.
> 
> This'll come up again once we add throwing of OverconstrainedError (which we discussed doing in a second patch), since a 0 would throw OverconstrainedError above.
> 
> Is there a way to know the actual device max here?

I fixed it but please double check it.

> Another gotcha is the mChannelCount.Get method is just a plain range clamper, so we could actually see negative values here. We probably want to clamp values here, rather than assert, otherwise the following would crash a debug build:
> 
>     track.applyConstraints({ channelCount: { max: -1 }});
> 
> I also see some code duplication of this below. Could we hoist it out of the switch?

Fixed but please double check.

> the requested what? I'm not following. I'm a bit confused by the code that follows. Seems like we're just reading back the value we just set above with SetUserChannelCount. When would this not be the case? Seems redundant, unless I'm missing something.

I want to explain the fact that the requested constraint value night not be the applied value if it is not supported by device capabilities. For example if we request channelCount : 3 on a device that support up to stereo the applied channelCount will be 2.

The code that follows provides the actual applied value. The `SetUserChannelCount` calls the `AudioInputCubeb::GetUserChannelCount` which provide either the value just set above or if it is supported by the device capabilities the max number of channels that is supported.

The comment updated.

> Can channelCount never be higher than 2? What prevents this assert from hitting if a site passes in { channelCount: 3 } ?
> 
> If it's an internal limitation, should this assert be inside GetChannelCount() instead?

In theory this value can be higher than 2 is the device supports it and user request it. I will remove the upper limit.

> CHANNELS is 1? When exactly would this default kick in?
> 
> https://dxr.mozilla.org/mozilla-central/rev/fe809f57bf2287bb937c3422ed03a63740b3448b/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#19

Yeah, that was wrong. That codec channels need to be the same with the number of channels in the driver. I updated the code.
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review155540
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review154470

> We can make it but I'm not sure if we want it, maybe Paul has a better opinion about it.

Couldn't hurt I think. Should be simple cut'n'paste job.
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review154470

> I want to explain the fact that the requested constraint value night not be the applied value if it is not supported by device capabilities. For example if we request channelCount : 3 on a device that support up to stereo the applied channelCount will be 2.
> 
> The code that follows provides the actual applied value. The `SetUserChannelCount` calls the `AudioInputCubeb::GetUserChannelCount` which provide either the value just set above or if it is supported by the device capabilities the max number of channels that is supported.
> 
> The comment updated.

Why not we clamp the input value to be valid on set? Is there any value in keeping around an sUserChannelCount that is too high?
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review155902

The input value validation needs another look. I've suggested some code.

::: commit-message-aa8ca:2
(Diff revisions 1 - 2)
> +* * *
> +[mq]: review

clean up commit msg.

::: dom/media/MediaStreamGraph.cpp:3185
(Diff revisions 1 - 2)
> +  typedef MediaStreamGraphImpl::LifecycleState state;
> +  MOZ_ASSERT(GraphImpl()->mLifecycleState == state::LIFECYCLE_RUNNING);

Nit: s/state/State/ (avoid all-lowercase types)

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:292
(Diff revisions 1 - 2)
> -  prefs.mChannels = c.mChannelCount.Get(prefs.mChannels);
> +  uint32_t maxChannels = 0, channels = 0;
> +  if (AudioInputCubeb::GetDeviceMaxChannels(mCapIndex, maxChannels) == 0) {
> +    channels = c.mChannelCount.Get(maxChannels);
> +  } else {
> +    channels = c.mChannelCount.Get(prefs.mChannels);
> +  }
> +  prefs.mChannels = channels < 0 ? 0 : channels;

This seems to ignore prefs.mChannels. We should also disallow 0 as a JS-observable setting.

How about:

    uint32_t maxChannels = 1;
    AudioInputCubeb::GetDeviceMaxChannels(mCapIndex, maxChannels);
    prefs.mChannels = c.mChannelCount.Get(std::max(1, std::min(prefs.mChannels, maxChannels)));

This should also simplify downstream code (SetUserChannelCount and GetUserChannelCount) since we'd never set an invalid value (unless the cubeb config can change on us. Can it?)
Attachment #8878009 - Flags: review?(jib) → review-
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review155902

> This seems to ignore prefs.mChannels. We should also disallow 0 as a JS-observable setting.
> 
> How about:
> 
>     uint32_t maxChannels = 1;
>     AudioInputCubeb::GetDeviceMaxChannels(mCapIndex, maxChannels);
>     prefs.mChannels = c.mChannelCount.Get(std::max(1, std::min(prefs.mChannels, maxChannels)));
> 
> This should also simplify downstream code (SetUserChannelCount and GetUserChannelCount) since we'd never set an invalid value (unless the cubeb config can change on us. Can it?)

This is indeed a better way to do it. I updated the code to set the `prefs.mChannels`. I am not sure if we should remove the call of `GetUserChannelCount`. It's not general and if things change we loose control.
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review157338

Good, though we still need to clamp values after the constraints algorithm, and implement OverconstrainedError.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:296
(Diff revisions 2 - 3)
> -  if (AudioInputCubeb::GetDeviceMaxChannels(mCapIndex, maxChannels) == 0) {
> -    channels = c.mChannelCount.Get(maxChannels);
> +  if (mAudioInput->GetMaxAvailableChannels(maxChannels) != 0) {
> +    return NS_ERROR_FAILURE;
> -  } else {
> -    channels = c.mChannelCount.Get(prefs.mChannels);
>    }
> -  prefs.mChannels = channels < 0 ? 0 : channels;
> +  int32_t maxAvailableChannels = maxChannels;

Is this just to deal with the sign?

I forget if this causes a lint warning or not on one of the builders. May want to add an explicit cast. Is it green on try?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:297
(Diff revisions 2 - 3)
> -    channels = c.mChannelCount.Get(maxChannels);
> +    return NS_ERROR_FAILURE;
> -  } else {
> -    channels = c.mChannelCount.Get(prefs.mChannels);
>    }
> -  prefs.mChannels = channels < 0 ? 0 : channels;
> +  int32_t maxAvailableChannels = maxChannels;
> +  prefs.mChannels = c.mChannelCount.Get(std::min(prefs.mChannels, maxAvailableChannels));

I forgot we still need to handle 0 meaning max, so I think this needs to be:

    if (prefs.mChannels <= 0) {
      prefs.mChannels = maxAvailableChannels;
    }
    prefs.mChannels = c.mChannelCount.Get(std::min(prefs.mChannels, maxAvailableChannels));

Then we have a valid input into the constraints algorithm.

But prefs.mChannels may still be out of bounds after the algorithm. E.g. any of these will do it:

    track.applyConstraints({ channelCount: {min: 3}});
    track.applyConstraints({ channelCount: {max: 0}});
    track.applyConstraints({ channelCount: {exact: 3}});
    track.applyConstraints({ channelCount: {ideal: 3}});
    track.applyConstraints({ channelCount: 3};
    track.applyConstraints({ channelCount: 0};

So we need to clamp after the algorithm as well.

In addition, the first three above are required constraints that should produce OverConstrainedError. We need to implement these manually since FlattenedConstraints only handles OverConstrainedError when multiple requests are incompatible. It does not know about our inherent limits (1-maxAvailableChannels).

I can think of two ways to implement this:
  A. Check c.mChannelCount.mMin, mMax and mExact explicitly, or
  B. Create a competing FlattenedConstraints based on (1-maxAvailableChannels) and merge [1]

A is probably simplest. To produce OverConstrained error, just do:

    *aOutBadConstraint = "channelCount";

[1] https://dxr.mozilla.org/mozilla-central/rev/2f10d697c307a09a286faa1f11831644c40f778d/dom/media/webrtc/MediaEngine.h#406-424

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:353
(Diff revisions 2 - 3)
>          auto& source = mSources.LastElement();
>          mAudioInput->SetUserChannelCount(prefs.mChannels);
> -        webrtc::CodecInst codec;
> -        strcpy(codec.plname, ENCODING);
> -        uint32_t channels = 0;
> -        if (mAudioInput->GetChannelCount(mCapIndex, channels)) {
> +        // Get validated number of channel
> +        uint32_t channelCount = 0;
> +        mAudioInput->GetChannelCount(channelCount);
> +        uint32_t last_pref_channels = mLastPrefs.mChannels;

last_pref_channels seems redundant. Can we just use mLastPrefs.mChannels directly below?
Attachment #8878009 - Flags: review?(jib) → review-
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review157338

> Is this just to deal with the sign?
> 
> I forget if this causes a lint warning or not on one of the builders. May want to add an explicit cast. Is it green on try?

Yeah it's for the warning. I have not tested with try but I have a warning locally. I will try to cats it.

> last_pref_channels seems redundant. Can we just use mLastPrefs.mChannels directly below?

We get again a warning locally, I will try to cast here as well.
Comment on attachment 8878009 [details]
Bug 1213414 - Implement channelCount audio constraint.

https://reviewboard.mozilla.org/r/149424/#review158344

Lgtm.
Attachment #8878009 - Flags: review?(jib) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f2e7a3ca954a -d 5aa966279356: rebasing 404661:f2e7a3ca954a "Bug 1213414 - Add channelCount constraint in webidl file. r=jib,padenot,smaug"
rebasing 404662:7c725d19f816 "Bug 1213414 - Implement channelCount audio constraint. r=jib,padenot" (tip)
merging dom/media/GraphDriver.cpp
merging dom/media/MediaManager.cpp
merging dom/media/MediaStreamGraph.cpp
merging dom/media/MediaStreamGraph.h
merging dom/media/webrtc/MediaEngineWebRTC.cpp
merging dom/media/webrtc/MediaEngineWebRTC.h
merging modules/libpref/init/all.js
warning: conflicts while merging modules/libpref/init/all.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d696d4dde21c
Add channelCount constraint in webidl file. r=jib,padenot,smaug
https://hg.mozilla.org/integration/autoland/rev/999d51686767
Implement channelCount audio constraint. r=jib,padenot
https://hg.mozilla.org/mozilla-central/rev/d696d4dde21c
https://hg.mozilla.org/mozilla-central/rev/999d51686767
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 971528
Depends on: 1392837
No longer depends on: 1392837
See Also: → 1393401
You need to log in before you can comment on or make changes to this bug.