Closed Bug 1436074 Opened 6 years ago Closed 6 years ago

Reduce timer for turning off camera on disable by time camera has already been on

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(1 file)

Bug 1436074 - Reduce turn-off timer by time since we turned on.
59 bytes, text/x-review-board-request
jib
: review+
Details 
We initially set this to default to 3 seconds, but after having tried in the field it does feel a bit long on the assumption that users expect it to happen as a reaction to their click, and hanging around for three seconds would make them rather impatient.

At the same time it cannot be too small, as an application could do a quick toggle on and back off in order to take a picture. This needs to light up the camera light for long enough for the user to notice it.
Rank: 17
Priority: -- → P2
So I've been playing with this some with Johann's patches in bug 1333468, and overall it works great!

That said, the in-chrome privacy indicators are delayed as well - which makes sense since they in many ways represent the same thing as the hardware light - but it feels sluggish and unresponsive.

So I have an idea:

Rather than going down to 2 seconds, can we skip the delay entirely in the common case where the camera has already been on for more than 3 seconds?

What we're trying to protect against are exploits going quickly from OFF->ON->OFF, not quickly from ON->OFF->ON.

Does that make sense? IOW subtract the time since the camera last went on from the timer.

Sorry for not coming to this realization sooner. I hadn't considered how it would feel with the UI components updating in lockstep with the light.

Sample fiddle https://jsfiddle.net/jib1/cfcoqdwz/
Discussed on slack. Comment 1 seems the way to go.
Summary: Reduce timer for turning off camera on disable → Reduce timer for turning off camera on disable by time camera has already been on
Sounds good, and should be easy enough. We add a TimeStamp denoting when the device was enabled to the DeviceState struct. Then when disabling we set the timer to `max(0, timeout - (now - enableTime))`. This way we don't have to touch existing logic.
Will need the changes in bug 1436694 in order to make this happen the simplest.
Depends on: 1436694
That seems like a great idea to me!
Comment on attachment 8951626 [details]
Bug 1436074 - Reduce turn-off timer by time since we turned on.

https://reviewboard.mozilla.org/r/220918/#review226844


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaManager.cpp:4100
(Diff revision 1)
>          aTrackID == kAudioTrack
>            ? "media.getusermedia.microphone.off_while_disabled.delay_ms"
>            : "media.getusermedia.camera.off_while_disabled.delay_ms",
>          3000));
> -    timerPromise = state.mDisableTimer->WaitFor(offDelay, __func__);
> +    const TimeDuration current = (TimeStamp::Now() - state.mDeviceEnabledTime);
> +    const TimeDuration delay = TimeDuration::Max(0, max - current);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

    const TimeDuration delay = TimeDuration::Max(0, max - current);
                                                 ^
                                                 nullptr
Comment on attachment 8951626 [details]
Bug 1436074 - Reduce turn-off timer by time since we turned on.

https://reviewboard.mozilla.org/r/220918/#review227102

Lgtm.

::: dom/media/MediaManager.cpp:4093
(Diff revision 1)
>  
>    RefPtr<MediaTimerPromise> timerPromise;
>    if (aEnable) {
>      timerPromise = MediaTimerPromise::CreateAndResolve(true, __func__);
>    } else {
> -    const TimeDuration offDelay = TimeDuration::FromMilliseconds(
> +    const TimeDuration max = TimeDuration::FromMilliseconds(

Nit: Maybe maxDelay to avoid confusion with std::max?

::: dom/media/MediaManager.cpp:4099
(Diff revision 1)
>        Preferences::GetUint(
>          aTrackID == kAudioTrack
>            ? "media.getusermedia.microphone.off_while_disabled.delay_ms"
>            : "media.getusermedia.camera.off_while_disabled.delay_ms",
>          3000));
> -    timerPromise = state.mDisableTimer->WaitFor(offDelay, __func__);
> +    const TimeDuration current = (TimeStamp::Now() - state.mDeviceEnabledTime);

Nit: redundant () or =

Also maybe s/current/timeEnabled/ ?
Attachment #8951626 - Flags: review?(jib) → review+
Comment on attachment 8951626 [details]
Bug 1436074 - Reduce turn-off timer by time since we turned on.

https://reviewboard.mozilla.org/r/220918/#review227154

::: dom/media/MediaManager.cpp:4121
(Diff revision 1)
>             aTrackID));
>  
>        state.mDeviceEnabled = aEnable;
>  
> +      if (aEnable) {
> +        state.mDeviceEnabledTime = TimeStamp::Now();

Hmm, I'm going to s/mDeviceEnabledTime/mTrackEnabledTime/ and assign it before returning the timer promise.

timerPromise->Then involves a dispatch so there's a chance we disable the track and evaluate the delay to use *before* assigning the track-enabled-time here.
Comment on attachment 8951626 [details]
Bug 1436074 - Reduce turn-off timer by time since we turned on.

https://reviewboard.mozilla.org/r/220918/#review227156


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaManager.cpp:4103
(Diff revision 2)
>            : "media.getusermedia.camera.off_while_disabled.delay_ms",
>          3000));
> -    timerPromise = state.mDisableTimer->WaitFor(offDelay, __func__);
> +    const TimeDuration durationEnabled =
> +      TimeStamp::Now() - state.mTrackEnabledTime;
> +    const TimeDuration delay =
> +      TimeDuration::Max(TimeDuration(0), maxDelay - durationEnabled);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

      TimeDuration::Max(TimeDuration(0), maxDelay - durationEnabled);
                                     ^
                                     nullptr
Status: NEW → ASSIGNED
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aff350b83b8
Reduce turn-off timer by time since we turned on. r=jib
Backed out for frequent assertion failures at MediaEngineWebRTCAudio.cpp:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0b46a7a0a05b1e3f1d46147a46bbe37a7f55f5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1aff350b83b8d504dbaa3d5d8d4fe6cd99512786&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164350480&repo=mozilla-inbound&lineNumber=10939

[task 2018-02-26T15:24:16.209Z] 15:24:16     INFO - GECKO(1045) | [1101:WebRTCPD #2]: E/signaling [WebRTCPD #2|WebrtcAudioSessionConduit] AudioConduit.cpp:797: A/V sync: sync delta: 0ms, audio jitter delay 53ms, playout delay 0ms
[task 2018-02-26T15:24:16.392Z] 15:24:16     INFO - GECKO(1045) | [1101:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for (nil)
[task 2018-02-26T15:24:16.611Z] 15:24:16     INFO - GECKO(1045) | (stun/INFO) STUN-CLIENT(pgCr|IP4:172.17.0.4:36422/UDP|IP4:172.17.0.4:49302/UDP(host(IP4:172.17.0.4:36422/UDP)|candidate:0 1 UDP 2122252543 172.17.0.4 49302 typ host)): Timed out
[task 2018-02-26T15:24:16.613Z] 15:24:16     INFO - GECKO(1045) | (ice/INFO) ICE-PEER(PC:1519658652948303 (id=2147483750 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/test_peerConnection_verifyAudioAfter:default)/CAND-PAIR(pgCr): setting pair to state FAILED: pgCr|IP4:172.17.0.4:36422/UDP|IP4:172.17.0.4:49302/UDP(host(IP4:172.17.0.4:36422/UDP)|candidate:0 1 UDP 2122252543 172.17.0.4 49302 typ host)
[task 2018-02-26T15:24:16.710Z] 15:24:16     INFO - GECKO(1045) | [1101:WebRTCPD #2]: E/signaling [WebRTCPD #2|WebrtcAudioSessionConduit] AudioConduit.cpp:797: A/V sync: sync delta: 0ms, audio jitter delay 48ms, playout delay 0ms
[task 2018-02-26T15:24:17.398Z] 15:24:17     INFO - GECKO(1045) | [1101:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for (nil)
[task 2018-02-26T15:24:17.757Z] 15:24:17     INFO - GECKO(1045) | --DOMWINDOW == 11 (0x7f81840c7000) [pid = 1045] [serial = 52] [outer = (nil)] [url = chrome://browser/content/webrtcIndicator.xul]
[task 2018-02-26T15:24:17.758Z] 15:24:17     INFO - GECKO(1045) | --DOMWINDOW == 10 (0x7f8182f38000) [pid = 1045] [serial = 50] [outer = (nil)] [url = chrome://browser/content/webrtcIndicator.xul]
[task 2018-02-26T15:24:17.969Z] 15:24:17     INFO - GECKO(1045) | Assertion failure: (aGraph->IterationEnd() == 0 && mLastAppendTime == 0) || aGraph->IterationEnd() > mLastAppendTime (Iteration time didn't advance since last append), at /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:115
[task 2018-02-26T15:24:52.127Z] 15:24:52     INFO - GECKO(1045) | #01: mozilla::MediaEngineWebRTCMicrophoneSource::Pull [dom/media/webrtc/MediaEngineWebRTCAudio.cpp:767]
[task 2018-02-26T15:24:52.128Z] 15:24:52     INFO - 
[task 2018-02-26T15:24:52.129Z] 15:24:52     INFO - GECKO(1045) | #02: mozilla::MediaDevice::Pull [dom/media/MediaManager.cpp:1031]
[task 2018-02-26T15:24:52.130Z] 15:24:52     INFO - 
[task 2018-02-26T15:24:52.132Z] 15:24:52     INFO - GECKO(1045) | #03: mozilla::SourceListener::NotifyPull [dom/media/MediaManager.cpp:4251]
Flags: needinfo?(apehrson)
Flags: needinfo?(apehrson)
Depends on: 1280099
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07fad8b0b417
Reduce turn-off timer by time since we turned on. r=jib
https://hg.mozilla.org/mozilla-central/rev/07fad8b0b417
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
I tested this issue on Firefox Nightly 60.0a1(2018-03-07) using the test page from comment 1 (https://jsfiddle.net/jib1/cfcoqdwz/) and using the webcams Logitec Pro HD C920 and Microsoft LifeCam HD-3000, here are the results:

On Mac OS X 10.12  

Logitec Pro HD C920  - OK
Microsoft LifeCam HD-3000 - OK

On Ubuntu 16.04 

Logitec Pro HD C920  - OK
Microsoft LifeCam HD-3000 - OK

On Windows 10 x64

Logitec Pro HD C920  - After I click on "mute" the camera light is still on     AR: The camera light should turn off. 
Microsoft LifeCam HD-3000 - OK
(In reply to ovidiu boca[:Ovidiu] from comment #18)
> I tested this issue on Firefox Nightly 60.0a1(2018-03-07) using the test
> page from comment 1 (https://jsfiddle.net/jib1/cfcoqdwz/) and using the
> webcams Logitec Pro HD C920 and Microsoft LifeCam HD-3000, here are the
> results:
> 
> On Mac OS X 10.12  
> 
> Logitec Pro HD C920  - OK
> Microsoft LifeCam HD-3000 - OK
> 
> On Ubuntu 16.04 
> 
> Logitec Pro HD C920  - OK
> Microsoft LifeCam HD-3000 - OK
> 
> On Windows 10 x64
> 
> Logitec Pro HD C920  - After I click on "mute" the camera light is still on 
> AR: The camera light should turn off. 
> Microsoft LifeCam HD-3000 - OK

Thanks for verifying. The Windows issue with Logitech C920 is bug 1436341 which was fixed and verified in a later build.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: