1203529 - Bad display when starting an audio-only call to a contact

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

When you start an audio-only call to a contact, you end up with the display as two halves - the left half has the audio mute avatar for the contact, the right half has the local section, but there's a spinner that never goes away.

What it should be is the remote avatar is full size of the window, and the local avatar is inset.

This regressed in 42 - as a result of bug 1180179.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bad display

Comment 2

[Mark Banner (:standard8)]

Attachment: Bad display when starting an audio-only call to a contact.

This fixes this issue, and in doing so also fixes bug 1171969.

The issue is that the views were only getting streams when video is enabled. This confuses them as to whether or not the video enabled. For some reason, this is especially worse when starting off with an audio-only call.

Given that the streams we're getting include audio, the aim of this patch is to always tell the stores/views about the stream for its lifetime, regardless of video/audio status. It then separates out the toggle status of video into a separate action.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8659297 [details] [diff] [review]
Bad display when starting an audio-only call to a contact.

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

Looks good; r=dmose with the mentioned issues addressed.

::: browser/components/loop/content/shared/js/actions.js
@@ +232,5 @@
> +     */
> +    MediaStreamCreated: Action.define("mediaStreamCreated", {
> +      hasVideo: Boolean,
> +      isLocal: Boolean,
> +      srcVideoObject: Object

Global rename this to srcStreamObject for readability?

@@ -254,5 @@
> -    /**
> -     * Video from the local camera has been enabled.
> -     *
> -     * XXX we should implement a LocalVideoDisabled action to cleanly prevent
> -     * leakage; see bug 1171978 for details

Review and consider closing this bug, since this should fix the leak.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +664,5 @@
> +        return;
> +      }
> +
> +      this.setStoreState({
> +        remoteSrcVideoObject: null

Please consider renaming these as well.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +599,5 @@
> +        hasVideo: sdkSubscriberObject.stream[STREAM_PROPERTIES.HAS_VIDEO],
> +        isLocal: false,
> +        srcVideoObject: sdkSubscriberVideo
> +      }));
> +

Let's put this below the "on"s to prevent future races.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +475,5 @@
>  
>      it("should dispatch a DataChannelsAvailable action with available = false", function() {
>        driver.disconnectSession();
>  
> +      sinon.assert.calledThrice(dispatcher.dispatch);

Let's just change this to called, so that it's not dependent on other random implemntation details.,   Similarly the tests below.

@@ +992,5 @@
>              srcVideoObject: videoElement
>            }));
>        });
>  
> +      it("should ispatch MediaStreamCreated after subscribe is complete with audio-only indication", function() {

s/ispatch/dispatch/, and with hasVideo set to false.

Comment 4

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #3)
> ::: browser/components/loop/content/shared/js/actions.js
> @@ +232,5 @@
> > +     */
> > +    MediaStreamCreated: Action.define("mediaStreamCreated", {
> > +      hasVideo: Boolean,
> > +      isLocal: Boolean,
> > +      srcVideoObject: Object
> 
> Global rename this to srcStreamObject for readability?

I've moved this out to bug 1203850 to keep this patch smaller as it'll need uplifting.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bad display when starting an audio-only call to a contact.

Updated for review comments.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c2d62280bd5c

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8659753 [details] [diff] [review]
Bad display when starting an audio-only call to a contact.

Approval Request Comment
[Feature/regressing bug #]: Bug 1180179 - media layout rework for Hello
[User impact if declined]: Users starting audio-only calls, or if they mute their videos at both ends will see a bad layout.
[Describe test coverage new/current, TreeHerder]: Covered by unit tests run in tree header. We also have functional tests run separately that cover some of the control flows.
[Risks and why]: Low risk. Although its a slightly larger patch that does rework how our streams are handled, it is also one of our main setup flows - its used for every conversation/direct call. Hence any problems should quickly be found.
[String/UUID change made/needed]: None

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c2d62280bd5c

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8659753 [details] [diff] [review]
Bad display when starting an audio-only call to a contact.

OK, not really happy about the size of the patch... but we still have time to find/fix the potential regression.

Comment 10

[Carsten Book [:Tomcat]]

hi, this cause problems when uplifting to aurora. 

merging browser/components/loop/ui/ui-showcase.js failed!                                                                                                                                                                                     
merging browser/components/loop/ui/ui-showcase.jsx                                                                                                                                                                                            
3 files to edit
merging browser/components/loop/ui/ui-showcase.jsx failed!                                                                                                                                                                                    

Mark could you take a look thanks!

Comment 11

[Mark Banner (:standard8)]

Fixed it up, tested and pushed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d896fd0ad02

Comment 12

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using an old Nightly (2015-08-11), verified that the issue is fixed using latest DevEdition 43.0a2 and Firefox 42beta3 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).