Closed Bug 1691899 Opened 4 years ago Closed 4 years ago

Logic issue in MediaTrackGraphImpl::OpenAudioInputImpl

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 1699233

People

(Reporter: sg, Unassigned)

References

(Regression)

Details

(Keywords: regression)

MediaTrackGraphImpl::OpenAudioInputImpl contains the following code at https://searchfox.org/mozilla-central/rev/7067bbd8194f4346ec59d77c33cd88f06763e090/dom/media/MediaTrackGraph.cpp#636-642:

  nsTArray<RefPtr<AudioDataListener>>& listeners =
      mInputDeviceUsers.GetOrInsert(aID);
  if (listeners.IsEmpty() && mInputDeviceUsers.Count() > 1) {
    // We don't support opening multiple input device in a graph for now.
    listeners.RemoveElement(aID);
    return;
  }

In the if body, listeners can't contain aID, since the condition requires listeners to be empty. I suspect this should be mInputDeviceUsers.Remove(aID); instead?

Flags: needinfo?(apehrson)
Flags: needinfo?(apehrson) → needinfo?(padenot)
Regressed by: 1404977
No longer regressed by: 1454998
Has Regression Range: --- → yes

Fixed the regressing bug reference, this was actually added before renaming the file in https://hg.mozilla.org/mozilla-central/annotate/507d5c269d25aee79fe081b1b02a4970ddcc1fb0/dom/media/MediaStreamGraph.cpp

If the assumption from comment 0 is correct, this could be changed to:

  // Only allow one device per MTG (hence, per document), but allow opening a
  // device multiple times
  mInputDeviceUsers.WithEntryHandle(aID, [&, hashtableWasEmpty = mInputDeviceUsers.Count() == 0](auto&& entry) {
    if (!entry) {
      // first open for this device
      mInputDeviceID = aID;
      // Switch Drivers since we're adding input (to input-only or full-duplex)
      AudioCallbackDriver* driver = new AudioCallbackDriver(
          this, CurrentDriver(), mSampleRate, AudioOutputChannelCount(),
          AudioInputChannelCount(), mOutputDeviceID, mInputDeviceID,
          AudioInputDevicePreference());
      LOG(LogLevel::Debug,
          ("%p OpenAudioInput: starting new AudioCallbackDriver(input) %p",
           this, driver));
      SwitchAtNextIteration(driver);

      entry.Insert(nsTArray<RefPtr<AudioDataListener>>{});
    }

      if (entry.Data().IsEmpty() && !hashtableWasEmpty) {
        // We don't support opening multiple input device in a graph for now.
        return;
      }

    MOZ_ASSERT(!entry.Data().Contains(aListener), "Don't add a listener twice.");

    entry.Data().AppendElement(aListener);
  });

Or my assumption from comment 0 is wrong, and actually the if condition is wrong? The comment confuses me a bit.

I guess this can be closed as a duplicate of Bug 1699233, which now has the more detailed analysis.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Flags: needinfo?(padenot)
You need to log in before you can comment on or make changes to this bug.