Remote video shown before call is accepted

VERIFIED FIXED in mozilla33

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: abr, Unassigned)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
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.
Depends on: 990678
(Reporter)

Comment 2

4 years ago
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

4 years ago
Added to bug tree for desktop Client Loop.  blocks Bug 1000150 - [meta] Desktop client needs in-call control
Blocks: 1000150
Priority: -- → P1
Target Milestone: --- → mozilla33
(Reporter)

Comment 4

4 years ago
Created attachment 8434485 [details] [diff] [review]
Proof of concept patch
(Reporter)

Updated

4 years ago
Assignee: nobody → adam
Status: NEW → ASSIGNED
(Reporter)

Comment 5

4 years ago
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.
Flags: needinfo?(standard8)
(Reporter)

Comment 6

4 years ago
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.
Created attachment 8435025 [details] [diff] [review]
bug-1020451-accept-decline.patch

Simplest yet efficient implementation of an Accept/Decline view for incoming calls.
Attachment #8434485 - Attachment is obsolete: true
Attachment #8435025 - Flags: review?(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
Flags: needinfo?(standard8)
Created attachment 8435114 [details] [diff] [review]
bug-1020451-accept-decline-2.patch

Updated button l10n ids.
Attachment #8435025 - Attachment is obsolete: true
Attachment #8435025 - Flags: review?(standard8)
Attachment #8435114 - Flags: review?(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
Attachment #8435114 - Flags: review?(standard8) → review+
https://hg.mozilla.org/projects/elm/rev/059f616ab0f8

Closing for tracking purposes.
Assignee: adam → nperriault
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla33 → mozilla32
Target Milestone: mozilla32 → mozilla33
Does this need manual testing or is it sufficiently covered by automation?
Whiteboard: [qa?]
We have a smoketest covering this which has passed several times. Marking this verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.