1020451 - Remote video shown before call is accepted

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Adam Roach [:abr]]

When an incoming call arrives, the remote party's audio and video is rendered before the callee accepts the call. This is very startling from a user perspective; rendering of remote video should be prevented until the callee assents to the call, even in the MLP use case of leveraging camera permissions to indicate assent.

Comment 1

[Mark Banner (:standard8)]

The problem here is the way that the SDK is set up and initiates the call is difficult to work around. I was thinking that we'd fix this at the same time as or just after bug 990678 implements the explicit accept/reject.

Comment 2

[Adam Roach [:abr]]

I think this should be pretty easy with the API that the toolkit gives us. In a nutshell: when an incoming call shows up, set up the publisher but *not* the subscriber. Make sure to hook the "accessAllowed" event on the publisher (see http://tokbox.com/opentok/libraries/client/js/reference/Publisher.html#event:accessAllowed). When this event fires, *then* you add the subscriber.

Alternately, if you want to get the ICE exchange out of the way, you set up the subscriber at the same place we do now, but set subscribeToAudio(false) and subscribeToVideo(false). Once you see the accessAllowed event on the publisher, flip them both to 'true.'

In both cases, I think this is like an hour of work, tops.

Comment 3

[:shell escalante]

Added to bug tree for desktop Client Loop.  blocks Bug 1000150 - [meta] Desktop client needs in-call control

Comment 4

[Adam Roach [:abr]]

Attachment: Proof of concept patch

Comment 5

[Adam Roach [:abr]]

Mark -- The attached patch shows how this needs to hang together, but there seems to be some race in the way things are laid out: if you take to long to "answer," the the remote video seems to be behind an opaque pane of some kind (it flashes briefly when you hang up the call). If you have time to figure out what's going on here, I'd really appreciate it.

Comment 6

[Adam Roach [:abr]]

Current thinking, based on the difficulty getting this to work gracefully, is that we'll implement a portion of the accept/decline functionality instead. This is our long-term solution anyway.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1020451-accept-decline.patch

Simplest yet efficient implementation of an Accept/Decline view for incoming calls.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8435025 [details] [diff] [review]
bug-1020451-accept-decline.patch

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

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +11,5 @@
>  call_has_ended=Your call has ended.
>  close_window=Close this window
>  do_not_disturb=Do not disturb
> +incoming_call=Incoming call
> +accept=Accept

This should be a bit more descriptive, e.g. accept_button

@@ +12,5 @@
>  close_window=Close this window
>  do_not_disturb=Do not disturb
> +incoming_call=Incoming call
> +accept=Accept
> +decline=Decline

Ditto, I would suggest decline_button

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-1020451-accept-decline-2.patch

Updated button l10n ids.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8435114 [details] [diff] [review]
bug-1020451-accept-decline-2.patch

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

Looks good, r=Standard8

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/059f616ab0f8

Closing for tracking purposes.

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/059f616ab0f8

Comment 13

[u279076]

Does this need manual testing or is it sufficiently covered by automation?

Comment 14

[u279076]

We have a smoketest covering this which has passed several times. Marking this verified fixed.