Closed Bug 934667 Opened 11 years ago Closed 10 years ago

Setup preferences for Selecting Loopback devices in WebRTC

Categories

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

24 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: snandaku, Assigned: snandaku)

References

Details

Attachments

(1 file, 7 obsolete files)

While working on End-to-End audio performance framework in Talos, it was concluded having mozGetUserMedia() select loopback devices would be useful.
Blocks: 909524
Just from talos or from content? With a pref or constraint?
It is from content and as a preference. I have a working code that does this is dom/content/MediManager.cpp and testing it currently
We discussed this on IRC, I think what we wanted was to be able to set a pref that would indicate what devices to use for audio/video, so that the test harness could set those prefs and then getUserMedia() would simply return the chosen devices.
Assignee: nobody → snandaku
Attached file Bug934667.patch (obsolete) —
Attachment #830100 - Attachment is obsolete: true
Attachment #830100 - Attachment is patch: false
Attachment #8336939 - Attachment is obsolete: true
Attachment #8336939 - Attachment is patch: false
Attached patch Bug934667.patch (obsolete) — Splinter Review
Attachment #8336944 - Attachment is obsolete: true
Attachment #8336944 - Attachment is patch: false
Attached patch Bug934667.patch (obsolete) — Splinter Review
Attachment #8336944 - Attachment is obsolete: false
Attachment #8336944 - Attachment is patch: true
Attachment #8336944 - Attachment is obsolete: true
Attachment #8336959 - Attachment is obsolete: true
Attached patch Bug934667.patch (obsolete) — Splinter Review
Comment on attachment 8336961 [details] [diff] [review]
Bug934667.patch

Patch to add preference for choosing audio, video loopback devices for GUM.
Attachment #8336961 - Flags: review?(rjesup)
Comment on attachment 8336961 [details] [diff] [review]
Bug934667.patch

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

::: dom/media/MediaManager.cpp
@@ +735,5 @@
>      for (uint32_t len = sources.Length(), i = 0; i < len; i++) {
> +#ifdef DEBUG
> +      sources[i]->GetName(deviceName);
> +      if(media_device_name && strlen(media_device_name))  {
> +        if(deviceName.EqualsASCII(media_device_name)) {

spaces after if
strlen() > 0 or != 0
Comment why this is being done
Attachment #8336961 - Flags: review?(rjesup) → review+
Comment on attachment 8336961 [details] [diff] [review]
Bug934667.patch

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

::: dom/media/MediaManager.cpp
@@ +735,5 @@
>      for (uint32_t len = sources.Length(), i = 0; i < len; i++) {
> +#ifdef DEBUG
> +      sources[i]->GetName(deviceName);
> +      if(media_device_name && strlen(media_device_name))  {
> +        if(deviceName.EqualsASCII(media_device_name)) {

This is needed in the case, when the preference is present but no value is specified ie as in empty string. 

I will add the spacing and upload patch for final review.

Thanks Randell
Attachment #8336961 - Attachment is obsolete: true
Comment on attachment 8343216 [details] [diff] [review]
Add DEBUG only preferences for setting loopback media devices

Taking R+ forward with comments incorporated
Attachment #8343216 - Flags: review?(rjesup)
Comment on attachment 8343216 [details] [diff] [review]
Add DEBUG only preferences for setting loopback media devices

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

::: dom/media/MediaManager.cpp
@@ +1044,5 @@
> +    // Check if the preference for using loopback devices is enabled.
> +    nsresult rv;
> +    nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
> +    if (NS_SUCCEEDED(rv)) {
> +      nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(prefs);

Just noticed: this is in a Run() with NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");

But prefs cannot be safely access anywhere except main-thread.  So you need to access them on mainthread, and pass the values to here.
Attachment #8343216 - Flags: review?(rjesup) → review-
Attachment #8343216 - Attachment is obsolete: true
Attachment #8344348 - Attachment is obsolete: true
Attachment #8344348 - Attachment is patch: false
Comment on attachment 8344349 [details] [diff] [review]
Add DEBUG only preferences for setting loopback media devices

Moved preference checking to main thread. Thanks Randell for catching this.
Attachment #8344349 - Flags: review?(rjesup)
Attachment #8344349 - Flags: review?(rjesup) → review+
Comment on attachment 8344349 [details] [diff] [review]
Add DEBUG only preferences for setting loopback media devices

Taking r+ from Randell forward
Attachment #8344349 - Flags: checkin?(rjesup)
Comment on attachment 8344349 [details] [diff] [review]
Add DEBUG only preferences for setting loopback media devices

https://hg.mozilla.org/integration/mozilla-inbound/rev/622e6c1191eb

In the future, you can just use the checkin-needed bug keyword for landing. :)
Attachment #8344349 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/622e6c1191eb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Is there a reason you enabled this only for debug builds? Seems like this would be generally useful for automation.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: