Closed Bug 1020451 Opened 11 years ago Closed 11 years ago

Remote video shown before call is accepted

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: abr, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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.
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
Attached patch Proof of concept patch (obsolete) (deleted) — Splinter Review
Assignee: nobody → adam
Status: NEW → ASSIGNED
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)
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.
Attached patch bug-1020451-accept-decline.patch (obsolete) (deleted) — Splinter Review
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)
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+
Assignee: adam → nperriault
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: