Closed Bug 1422875 Opened 7 years ago Closed 7 years ago

fake:true constraint should not affect screen sharing (needed for testing screenshare+audio)

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(2 files)

Bug 1422875 - Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android)
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1422875 - Fix fake constraint to not apply to screensharing (on all platforms but android).
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
This is needed for automated testing of screenshare+audio gUM requests. Problem revealed with existing tests on Windows when trying to bug 1397793.
"Trying to land"
Comment on attachment 8934208 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android) https://reviewboard.mozilla.org/r/205144/#review210664 r+ with nits considered. Up to you whether to fix them but the comments reflect the troubles I went through when trying to understand this code. ::: dom/media/MediaManager.cpp:1705 (Diff revision 1) > - bool hasVideo = aVideoType != MediaSourceEnum::Other; > + bool hasVideo = aVideoType != MediaSourceEnum::Other; > - bool hasAudio = aAudioType != MediaSourceEnum::Other; > + bool hasAudio = aAudioType != MediaSourceEnum::Other; > - bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > + bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > - bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool getReal = (!fakeCams && hasVideo) || (!fakeMics && hasAudio); It might just be me but I think this whole section would be easier to read if you had the variables `hasVideo` - video was requested `hasAudio` - audio was requested `fakeVideo` - the video requested is fake `fakeAudio` - the audio requested is fake (all unchanged from today) This pairs up the names `fakeVideo` and `fakeAudio` to match "video" and "audio" in `hasVideo` and `hasAudio`. When reading the code where these are used today (`fakeCams`, `fakeMics`) I have to check whether there's some special case to warrant the difference in naming. Turns out they just tell us whether the video or audio device should be from the fake backend. `realDevices` is also not so self-telling IMO. With it's presence I assumed it was a blanket statement saying we should enumerate all real devices. That was not the case, enumerating video is still hinged on `fakeCams`, and vice-versa for audio. I think we can inline it as `!fakeVideo || !fakeAudio`. It reads simply enough to allow being copied in two places IMHO. ::: dom/media/MediaManager.cpp:1709 (Diff revision 1) > - // Only enumerate what's asked for, and only fake cams and mics. > - bool hasVideo = aVideoType != MediaSourceEnum::Other; > + bool hasVideo = aVideoType != MediaSourceEnum::Other; > - bool hasAudio = aAudioType != MediaSourceEnum::Other; > + bool hasAudio = aAudioType != MediaSourceEnum::Other; > - bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > + bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > - bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool getReal = (!fakeCams && hasVideo) || (!fakeMics && hasAudio); s/getReal/realDevices/ ::: dom/media/MediaManager.cpp:1711 (Diff revision 1) > + RefPtr<Runnable> task = NewTaskFrom([id, aWindowId, audioLoopDev, > + videoLoopDev, aVideoType, > + aAudioType, hasVideo, hasAudio, > + fakeCams, fakeMics, getReal]() mutable { Does this need to be mutable? We seem to only pass primitives and I doubt we write to them. ::: dom/media/MediaManager.cpp:1763 (Diff revision 1) > - if (!aFake && > - (aVideoType == MediaSourceEnum::Camera || > + // Always request permission for one or more real devices (unless preffed off) > + if (getReal && > - aAudioType == MediaSourceEnum::Microphone) && > Preferences::GetBool("media.navigator.permission.device", false)) { "Always" but only if `getReal` is true? Do you mean "given that `getReal` is true *and* the pref is on, we request permission? I prefer to put comments regarding `if` conditionals inside the respective branch's block, since they otherwise have to cover all the branches and easily get confusing and ambiguous. E.g., are they commenting on the need for the `if` or what we're doing inside it?
Attachment #8934208 - Flags: review?(apehrson) → review+
Comment on attachment 8934209 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing (on all platforms but android). https://reviewboard.mozilla.org/r/205146/#review210678 ::: dom/media/MediaManager.cpp:2515 (Diff revision 1) > + bool hasVideo = videoType != MediaSourceEnum::Other; > + bool hasAudio = audioType != MediaSourceEnum::Other; > + bool fakeCams = fake && videoType == MediaSourceEnum::Camera; > + bool fakeMics = fake && audioType == MediaSourceEnum::Microphone; > + bool realDevices = (!fakeCams && hasVideo) || (!fakeMics && hasAudio); > + > bool askPermission = > (!privileged || Preferences::GetBool("media.navigator.permission.force")) && > - (!fake || Preferences::GetBool("media.navigator.permission.fake")); > + (realDevices || Preferences::GetBool("media.navigator.permission.fake")); Please consider the suggestions I had for the other patch.
Attachment #8934209 - Flags: review?(apehrson) → review+
Comment on attachment 8934208 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android) https://reviewboard.mozilla.org/r/205144/#review210684 ::: dom/media/MediaManager.cpp:1705 (Diff revision 1) > - bool hasVideo = aVideoType != MediaSourceEnum::Other; > + bool hasVideo = aVideoType != MediaSourceEnum::Other; > - bool hasAudio = aAudioType != MediaSourceEnum::Other; > + bool hasAudio = aAudioType != MediaSourceEnum::Other; > - bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > + bool fakeCams = aFake && aVideoType == MediaSourceEnum::Camera; > - bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool fakeMics = aFake && aAudioType == MediaSourceEnum::Microphone; > + bool getReal = (!fakeCams && hasVideo) || (!fakeMics && hasAudio); I think the existing names are clearer as fakeCams is always camera, and fakeMics is always mic. "video" can be either camera or screensharing "audio" can be either microphone or audioCapture. We never want to fake screensharing or audioCapture. realDevices means we'll be enumerating real devices (which require permission), *independent* of whether we'll also be enumerating fake devices. In the case of {fake: true, audio: true, video: {mediaSource: "screen"}} we'll be enumerating both.
Comment on attachment 8934208 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android) https://reviewboard.mozilla.org/r/205144/#review210664 > It might just be me but I think this whole section would be easier to read if you had the variables > `hasVideo` - video was requested > `hasAudio` - audio was requested > `fakeVideo` - the video requested is fake > `fakeAudio` - the audio requested is fake > (all unchanged from today) > > This pairs up the names `fakeVideo` and `fakeAudio` to match "video" and "audio" in `hasVideo` and `hasAudio`. > > When reading the code where these are used today (`fakeCams`, `fakeMics`) I have to check whether there's some special case to warrant the difference in naming. > > Turns out they just tell us whether the video or audio device should be from the fake backend. > > `realDevices` is also not so self-telling IMO. With it's presence I assumed it was a blanket statement saying we should enumerate all real devices. That was not the case, enumerating video is still hinged on `fakeCams`, and vice-versa for audio. > > I think we can inline it as `!fakeVideo || !fakeAudio`. It reads simply enough to allow being copied in two places IMHO. I think it's important that the logic in the two places don't fall out of lock-sync, as any mistake here could lead to a sec bug (allowing real access without permission), so I'd rather not inline for that reason.
Comment on attachment 8934208 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android) https://reviewboard.mozilla.org/r/205144/#review210664 > "Always" but only if `getReal` is true? > > Do you mean "given that `getReal` is true *and* the pref is on, we request permission? > > I prefer to put comments regarding `if` conditionals inside the respective branch's block, since they otherwise have to cover all the branches and easily get confusing and ambiguous. E.g., are they commenting on the need for the `if` or what we're doing inside it? I added the comment before realizing how it worked. Good catch. Removed.
Comment on attachment 8934209 [details] Bug 1422875 - Fix fake constraint to not apply to screensharing (on all platforms but android). https://reviewboard.mozilla.org/r/205146/#review210718 ::: dom/media/MediaManager.cpp:2515 (Diff revision 1) > + bool hasVideo = videoType != MediaSourceEnum::Other; > + bool hasAudio = audioType != MediaSourceEnum::Other; > + bool fakeCams = fake && videoType == MediaSourceEnum::Camera; > + bool fakeMics = fake && audioType == MediaSourceEnum::Microphone; > + bool realDevices = (!fakeCams && hasVideo) || (!fakeMics && hasAudio); > + > bool askPermission = > (!privileged || Preferences::GetBool("media.navigator.permission.force")) && > - (!fake || Preferences::GetBool("media.navigator.permission.fake")); > + (realDevices || Preferences::GetBool("media.navigator.permission.fake")); I s/realDevices/realDevicesPresent/ to underscore that this boolean represents the presence of real devices rather than representing all or nothing. Hope that helps.
Opened a new bug on the one try failure, which I've convinced myself is unrelated since it uses {fake: false} explicitly.
Pushed by jbruaroey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ba4cd2e93fec Fix fake constraint to not apply to screensharing for media.navigator.permission.device codepath (aka Android) r=pehrsons https://hg.mozilla.org/integration/autoland/rev/525f2f162385 Fix fake constraint to not apply to screensharing (on all platforms but android). r=pehrsons
Rank: 8
https://hg.mozilla.org/mozilla-central/rev/ba4cd2e93fec https://hg.mozilla.org/mozilla-central/rev/525f2f162385
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: