Closed
Bug 1020451
Opened 11 years ago
Closed 11 years ago
Remote video shown before call is accepted
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla33
People
(Reporter: abr, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 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•11 years ago
|
||
Added to bug tree for desktop Client Loop. blocks Bug 1000150 - [meta] Desktop client needs in-call control
Reporter | ||
Comment 4•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → adam
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•11 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•11 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.
Assignee | ||
Comment 7•11 years ago
|
||
Simplest yet efficient implementation of an Accept/Decline view for incoming calls.
Attachment #8434485 -
Attachment is obsolete: true
Attachment #8435025 -
Flags: review?(standard8)
Comment 8•11 years ago
|
||
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
Updated•11 years ago
|
Flags: needinfo?(standard8)
Assignee | ||
Comment 9•11 years ago
|
||
Updated button l10n ids.
Attachment #8435025 -
Attachment is obsolete: true
Attachment #8435025 -
Flags: review?(standard8)
Attachment #8435114 -
Flags: review?(standard8)
Comment 10•11 years ago
|
||
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+
Comment 11•11 years ago
|
||
https://hg.mozilla.org/projects/elm/rev/059f616ab0f8
Closing for tracking purposes.
Assignee: adam → nperriault
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla33 → mozilla32
Comment 12•10 years ago
|
||
Updated•10 years ago
|
Target Milestone: mozilla32 → mozilla33
Comment 13•10 years ago
|
||
Does this need manual testing or is it sufficiently covered by automation?
Whiteboard: [qa?]
Comment 14•10 years ago
|
||
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.
Description
•