Closed Bug 1242061 Opened 8 years ago Closed 8 years ago

Update audio device enumeration to notice device removals and additions

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(2 files)

re-enumerate audio devices in full_duplex via cubeb when getUserMedia is called
9.58 KB, patch
jib
: review+
Details | Diff | Splinter Review
Interdiffs from review NOT FOR CHECKIN directly
4.82 KB, patch
Details | Diff | Splinter Review 
Initial full_duplex code did not update the list of devices after the first enumeration.

The best way would be to watch for device change notifications, but a simpler way for now is to re-enumerate each time we're asked for an enumeration (i.e. when gUM is called).  

The tricky part is keeping the indexes used by the webrtc.org code intact; we do this with a translation array and a cache of what entry (index) was what physical device, and maintain that mapping for the run of the browser.
Comment on attachment 8711196 [details] [diff] [review]
re-enumerate audio devices in full_duplex via cubeb when getUserMedia is called

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

Found nothing wrong, but can we refactor the goto?

::: dom/media/webrtc/MediaEngineWebRTC.cpp
@@ +324,5 @@
>  
>      if (uniqueId[0] == '\0') {
>        // Mac and Linux don't set uniqueId!
>        MOZ_ASSERT(sizeof(deviceName) == sizeof(uniqueId)); // total paranoia
> +      strcpy(uniqueId, deviceName); // safe given assert and initialization/error-check

whitespace-only change?

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +170,5 @@
>        mDevices = nullptr;
>      }
> +    mDeviceIndexes.Clear();
> +    for (uint32_t j = 0; j < mDeviceNames.Length(); j++) {
> +      delete mDeviceNames[j];

Why not i? or better: for (auto& deviceName : mDeviceNames)

@@ +265,5 @@
> +    }
> +
> +    for (uint32_t i = 0; i < mDeviceIndexes.Length(); i++) {
> +      mDeviceIndexes[i] = -1; // unmapped
> +    }

for ( : )

@@ +279,5 @@
> +      {
> +        for (uint32_t j = 0; j < mDeviceIndexes.Length(); j++) {
> +          // See if the device still exists (or exists again)
> +          if (strcmp(mDeviceNames[j], devices->device[i]->device_id) == 0) {
> +            // match! update the mapping

Not a fan of the goto out of the inner for-loop. If we use nsTArray<nsCString> for mDeviceNames, then we can use mDeviceNames.IndexOf() instead of the inner loop, and use continue instead of goto.

Also, the old code seemed to tolerate identical device names, whereas the new code will remove them. Do we need to handle that better?

@@ +281,5 @@
> +          // See if the device still exists (or exists again)
> +          if (strcmp(mDeviceNames[j], devices->device[i]->device_id) == 0) {
> +            // match! update the mapping
> +            fprintf(stderr, "remapped device %d (%s) to devices[%d]\n",
> +                    j, devices->device[i]->device_id, i);

I hope you plan to remove the fprintfs?

@@ +300,5 @@
> +    // swap state
> +    if (mDevices) {
> +      cubeb_device_collection_destroy(mDevices);
> +    }
> +    mDevices = devices;

This function didn't touch the old mDevices AFAICT, so is the safe overlap needed?

@@ +313,5 @@
>    int mSelectedDevice;
> +  bool mInUse; // for assertions about listener lifetime
> +
> +  static nsTArray<int> mDeviceIndexes;
> +  static nsTArray<char*> mDeviceNames;

I feel nsCString would be safer and just as efficient, and help the earlier code.
Attachment #8711196 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Comment on attachment 8711196 [details] [diff] [review]
> re-enumerate audio devices in full_duplex via cubeb when getUserMedia is
> called
> 
> Review of attachment 8711196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Found nothing wrong, but can we refactor the goto?

It's the classic issue with languages, one of the few places goto makes sense for exiting multiple loops (really it's "break N").  (in other cases, jumping to an abort-and-exit point.)  That said, I can add more code and avoid it.

> 
> ::: dom/media/webrtc/MediaEngineWebRTC.cpp
> @@ +324,5 @@
> >  
> >      if (uniqueId[0] == '\0') {
> >        // Mac and Linux don't set uniqueId!
> >        MOZ_ASSERT(sizeof(deviceName) == sizeof(uniqueId)); // total paranoia
> > +      strcpy(uniqueId, deviceName); // safe given assert and initialization/error-check
> 
> whitespace-only change?

yes

> ::: dom/media/webrtc/MediaEngineWebRTC.h
> @@ +170,5 @@
> >        mDevices = nullptr;
> >      }
> > +    mDeviceIndexes.Clear();
> > +    for (uint32_t j = 0; j < mDeviceNames.Length(); j++) {
> > +      delete mDeviceNames[j];
> 
> Why not i? or better: for (auto& deviceName : mDeviceNames)

Ok.

> @@ +265,5 @@
> > +    }
> > +
> > +    for (uint32_t i = 0; i < mDeviceIndexes.Length(); i++) {
> > +      mDeviceIndexes[i] = -1; // unmapped
> > +    }
> 
> for ( : )

So long as I don't need to use (naked) i in the loop, sure - but I do.

> @@ +279,5 @@
> > +      {
> > +        for (uint32_t j = 0; j < mDeviceIndexes.Length(); j++) {
> > +          // See if the device still exists (or exists again)
> > +          if (strcmp(mDeviceNames[j], devices->device[i]->device_id) == 0) {
> > +            // match! update the mapping
> 
> Not a fan of the goto out of the inner for-loop. If we use
> nsTArray<nsCString> for mDeviceNames, then we can use mDeviceNames.IndexOf()
> instead of the inner loop, and use continue instead of goto.

yeah, that would work I guess - though I'm wary of anything that might add more static initializers (I'm already thinking I might need to do something about the static nsTArrays - but then nsCString might be fine)

> Also, the old code seemed to tolerate identical device names, whereas the
> new code will remove them. Do we need to handle that better?

They inherently can't be identical or they're the same device.

> @@ +281,5 @@
> > +          // See if the device still exists (or exists again)
> > +          if (strcmp(mDeviceNames[j], devices->device[i]->device_id) == 0) {
> > +            // match! update the mapping
> > +            fprintf(stderr, "remapped device %d (%s) to devices[%d]\n",
> > +                    j, devices->device[i]->device_id, i);
> 
> I hope you plan to remove the fprintfs?

Ooops, of course :-)

> 
> @@ +300,5 @@
> > +    // swap state
> > +    if (mDevices) {
> > +      cubeb_device_collection_destroy(mDevices);
> > +    }
> > +    mDevices = devices;
> 
> This function didn't touch the old mDevices AFAICT, so is the safe overlap
> needed?

Yes.  Once you destroy mDevices, all the devids you're using become freed memory.  And devids are opaque pointers that you can't copy.

> @@ +313,5 @@
> >    int mSelectedDevice;
> > +  bool mInUse; // for assertions about listener lifetime
> > +
> > +  static nsTArray<int> mDeviceIndexes;
> > +  static nsTArray<char*> mDeviceNames;
> 
> I feel nsCString would be safer and just as efficient, and help the earlier
> code.

Yes; I'd thought of that but was working on getting the logic right.
(In reply to Randell Jesup [:jesup] from comment #3)

> > @@ +265,5 @@
> > > +    }
> > > +
> > > +    for (uint32_t i = 0; i < mDeviceIndexes.Length(); i++) {
> > > +      mDeviceIndexes[i] = -1; // unmapped
> > > +    }
> > 
> > for ( : )
> 
> So long as I don't need to use (naked) i in the loop, sure - but I do.

Wrong loop - it's fine there.  will do.
Comment on attachment 8711268 [details] [diff] [review]
Interdiffs from review NOT FOR CHECKIN directly

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

::: dom/media/webrtc/MediaEngineWebRTC.h
@@ +280,5 @@
> +          mDeviceIndexes[j] = i;
> +        } else {
> +          // new device, add to the array
> +          mDeviceIndexes.AppendElement(i);
> +          mDeviceNames.AppendElement(strdup(devices->device[i]->device_id));

Lose the strdup. Wont this leak?
Flags: needinfo?(rjesup)
Yup (small).  Though our leak detection probably won't see it
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: