1093780 - Update OpenTok library to latest 2.4 version

Status: RESOLVED FIXED

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

Description

[song]

Attachment: webrtc-js-v2.2.9.3.tgz

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36

Comment 1

[song]

Fixes in 2.2.9.3 patch:

* AudioLevelMeter now disabled completely if audioLevelDisplayMode set to false. This was causing high CPU utilization in FirefoxOS
* streamCreated no longer fire before connectionCreated. This caused occasional null connection objects.
* Chrome no longer throw the error "Uncaught SyntaxError: Failed to construct 'AudioContext': number of hardware contexts reached maximum
* Error will no longer be thrown when there is no audio track in the stream.
* responsive dialog for IE users in plugin prompt.

Comment 2

[Maire Reavy [:mreavy]]

Let's target this for desktop for early Fx37.

Comment 3

[:shell escalante]

when would be the best time to take this update?

Comment 4

[:shell escalante]

also we have a big update coming 1/15 - is it possibly worth waiting

Comment 5

[Mark Banner (:standard8)]

I can look at this next week. Incremental updates are generally better.

Comment 6

[Maire Reavy [:mreavy]]

Moving this to P2 based on our new priority definitions.

Comment 7

[Mark Banner (:standard8)]

Attachment: Do not land - Update OpenTok library to v2.2.9.3 for Loop.

Patch to update sdk. See notes coming in a moment.

Comment 8

[Mark Banner (:standard8)]

Hi Song,

So I've tried the new sdk here, and come across a couple of issues:

- The audio level indicator is no longer hidden in audio-only mode (we have audioLevelDisplayMode set to "off" in the options, as per previous versions).

- "bugDisplayMode" seems to have been removed, but its unclear if what it disabled has also been removed.

Can you have a look please?

Comment 9

[:shell escalante]

P1 for screen sharing

Comment 10

[Mark Banner (:standard8)]

Song: ping re the issues in comment 8.

Comment 11

[Rob Hainer]

Mark: ping received. Looking for someone available to help with this.

Comment 12

[msander]

We are releasing v2.4.0 of the JS SDK this week (we skipped some versions). This includes the public screen sharing API. I'll get that to you on 1/13 or 1/14.

I'm not sure if the issue with the audio level indicator exists in v2.4.0.

The "opentok bug" ui element has been removed completely so it's no longer necessary in the new version to use "bugDisplayMode"

Comment 13

[aoberoi]

Mark,

I'm new to this issue and working with the Mozilla team on front end concerns, so please excuse any holes in my context. Song is no longer with TokBox so I will be working with you folks as much as I can.

I've looked through the v2.4.0 release that [e-mail] has mentioned. If you'd like to hide the audio level indicators, please use the following line of code *after* the element (Publisher or Subscriber) has been initialized.

   publisher.setStyle('audioLevelDisplayMode', 'off');

(publisher is a reference to the element, a subscriber could also be used)

Comment 14

[msander]

Attachment: webrtc-js-2.4.0.tgz

Here is v2.4.0 of the JS SDK. Release notes will appear on our site in the next few days.

https://tokbox.com/opentok/libraries/client/js/release-notes.html

Comment 15

[Mark Banner (:standard8)]

Attachment: WIP Update OpenTok library to v2.4.0 for Loop.

Ok, I did some testing with 2.4.0.

The previous issue with audioLevelDisplayMode not working on the initPublisher/subscribe functions is now fixed.

However, on the desktop implementation of Loop in the conversation window, the local video is now massively zoomed in, and transparent - whereas it wasn't before.

I tracked down that there were some class name changes, that have broken some of the code. I've done some changes that I think are right, but the video is still being displayed wrong. That probably means either there were more css/layout changes than I've found, or something else is broken as well.

Dan - you've looked at this recently, would you be able to take a look today?

Comment 16

[Mark Banner (:standard8)]

Attachment: WIP Update OpenTok library to v2.4.0 for Loop.

The actual WIP patch

Comment 17

[aoberoi]

@Mark and @Dan. I wanted to follow up with you guys about the UI customizations pains you guys explained you felt a couple weeks ago. We have some new API in 2.4.0 that I think you guys will like.

1. On initialization, Publishers and Subscribers now have a new property called `'showControls'` which can be set to `false` in order to turn off all controls on the video that we draw be default (no audioLevels, no name, no mute/mic buttons). This only works on initialization so you would pass it in to `OT.initPublisher(target, properties, completionHandler)` and `session.subscribe(stream, target, properties, completionHandler)` as a key/value pair in the properties parameter.

2. There is new API for the crop/zoom behavior of the actual video element within the Publisher and Subscriber DOM elements. Similar to above, a new property called `'fitMode'` has been added. The possible values are `'cover'` and `'contain'`. These are inspired by CSS object-fit (https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit). From what I recall, you guys were looking for '`contain`'.

This should simplify the modifications needed to make the possibly broken CSS work, by refactoring into less "hack" code. Let me know how that works out.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

aberoi: very cool; thanks for getting us something so quickly.  We'll poke at as part of this patch, I think.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Standard8: I took your patch, and turned on fitMode: "cover" in all the various places where you had removed bugDisplayMode.  That fixed the zooming problem, but it looked like thing had been scaled (horizontally squished) in a way that didn't preserve the aspect ratio.  Need to play around a bit more to figure out what's going on.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Mark, I need to head out early today, and won't get a chance to play with this again until Tuesday, since Monday is a holiday here.  I'm interested in doing the work (and if you'd like that, feel free to reassign to me), but if you can't wait that long, I'd say just go ahead without me.

Comment 21

[Mark Banner (:standard8)]

Attachment: WIP v2 Update OpenTok library to v2.4.0 for Loop.

Updated patch that incorporates some of the recent changes to the codebase. Also an experimental cover/contain selection for fitMode.

I tried out the new modes a bit, and came across a few issues:

- For local video, using fitMode="contain" seems to be what we want - it'll always display the whole video (and hence, the remote users sees no more than what the local user sess).
- For remote video, using fitMode="cover" seems to be ideal as we'll get the maximum display possible - however, it doesn't work properly.

Using fitMode="cover" seems to have (on my display) two sizes of video element, anything in-between is anchored at the top-right corner of the display, and you loose the bottom/right parts of the video if you dynamicially resize - it seems to be fixed sizes.

It could be our styles, but I think I removed them and it was fine - I'll take another look in the morning.

Comment 22

[Mark Banner (:standard8)]

I had a bit more of a play around today, but couldn't improve quickly, therefore I'm going to let Dan take a look later.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

After spending some time on this too, there's definitely something odd about "cover".  Like Mark, I'm not 100% sure it's an SDK problem, but it kinda looks that way:

If I use the inspector on the standalone side to disable the forced width & height rules applied by the SDK directly inline on the video element itself, as well as the "width: 100%" rule for .OT_subscriber .OT_video_element the displayed content looks more complete, though the size can still be cropped or scaled wrong.

I suspect the easiest way to debug this is likely to be one of Standard8, Niko, or me pairing with Ankhur over Hangouts, Skype, or Vidyo.  Standard8 & Niko are on European time, and I'm on Pacific time, so we should be able to find some overlap for any of those conversations.  Ankur, is something you could make work?

Comment 24

[aoberoi]

Thanks for the summary, Dan. I'm going to spend a little time today to dig into this and some other layout funkiness I've been hearing about as well. They might be one in the same issue. I'll update this ticket with my findings by the end of the day.

If I don't come up with a solution by then, lets set up a time to pair. I'm on US Eastern time today and tomorrow but I'll be on US Central time on Friday.

Which IRC channel might be the best to find you all on?

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

#loop on irc.mozilla.org .  I'll be busy for a few hours starting now, but should be available in Pacific afternoon, otherwise tomorrow should be fine.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

also, thanks for the quick turnaround on looking at this, Ankur!

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

So, looking at the jsdoc inside the SDK itself, I see:

*               <code>"cover"</code> &mdash; The video is cropped if its dimensions do not match
*               those of the DOM element. This is the default setting for screen-sharing videos
*               (for Stream objects with the <code>videoType</code> property set to
*               <code>"screen"</code>).

Which means that at least some of the problems I was describing in comment 23 were actually my misunderstanding of how cover was supposed to work.

Mark, I'm not terribly convinced that we want to use cover.  In comment 21, when you said "maximum display possible", did you mean "least blank space"?

The main guidance from UX that I've seen on how to handle video sizing is in bug 1046789 (discussion for that was in bug 1008935).  There, the conclusion was that we want to neither crop nor letterbox, but instead want to scale.  I'm not actually convinced that that's what we really want, and I've added comment 2 there asking folks to dig into it a bit more.

My suspicion is that we want to actually letterbox/pillarbox.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

That said, we're effectively doing something like "cover" for remote video today in that we're cropping the remote video as soon as things resize too small, so if we can get cover working well, it shouldn't be perceived as a big regression, and we can leave behavior changes for other bugs.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

There are a couple of big responsiveness problems with the popped out window in the current patch (using cover mode): 

** pop out, resize until the remote video gets really big, pop back in, video is not scaled down and cropped:
https://www.evernote.com/shard/s3/sh/db403939-a3a2-47f7-bf5b-b0ef2549c1ad/d879806ebc1153af5fdf66c3f4a38db5

** tall narrow window: video in top-left corner with lots of empty space between it and local video bottom right
https://www.evernote.com/shard/s3/sh/60b05655-50a3-4f6e-acb2-fcf61fe9bea8/597dcf76dc1476df4dc26f991862e67c

** nearly square window: same problem as above
https://www.evernote.com/shard/s3/sh/9ea66302-6604-4399-a1bc-f036e263a451/d50a0069e68f1a66ac243a8ad4aa8987

** big remote video on left side with lots of empty space between it and remote video bottom right, another variant of above
https://www.evernote.com/shard/s3/sh/89ce2d81-960c-4c5e-b679-6e2a287e73a1/5519493394dca2417ff7c31f1bfb120e

I'm tracking them in https://etherpad.mozilla.org/hello-opentok240-css

Since the popped out window is so painful to debug, I'm gonna flip over to the standalone stuff now, where at least the devtools inspector works.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

I can easily reproduce the problem with the three variants in the popout (remote and local video in diagonally opposite corners with a bunch of space in between) in standalone too.  I've started poking at it more, and it appears to be leading towards the CSS props that are forced onto the DOM element in the SDK's fixFitModeCover.  I think the next step is to see what progress Ankur has made on the thing he was looking at, and then perhaps to pair.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Given my new understanding of bug 1046789, that we really do just want letter/pillarboxing, I spent some time with the remote video set to contain instead of cover.  So far, I haven't ended up with something that uses the maximum space when the standalone room view is resized large (which is what I'm using to debug, as it's the easiest).  That said, there's some chance we could get something that looks reasonable, even though it might not use as much space as it could.

Between our own CSS and the SDKs CSS, there are 9 layers of <div> wrapping the video element, and every one of them has a variety of rules, so debugging is a bit like playing Jenga.

My guess is that if it's straightforward to fix "cover", that's likely to be the lower risk pask to getting the SDK landed sooner rather than later.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

s/pask/path/

Comment 33

[Mark Banner (:standard8)]

Mike and I took a look at this today.

Currently, "cover" is most similar to what the production standalone currently uses; even if not what we want to end up with, I think this would be the quickest way to update to 2.4.0 and unblock the Screen Sharing work - however, the cover mode isn't working in the same way as previous versions did, and this is obviously broken.

Using the letter/pillarboxing "contain" mode does seem to be what we want. Part of the issues at the moment, are that there's a fixed maximum width on the entire main body, and the video sizes are expressed as percentages of the page, and don't take into account the aspect-ratio of the video.

The desktop side looks reasonable with the "contain" mode without any chances apart from to make the background black when letterboxing.

Mike is currently looking at prototyping something up which uses the aspect-ratios of the video and calculates the widths and heights in a more dynamic way - to maximise the screen area of the video and to keep the local video overlapped to the remote.

This is likely to take a few days to get right, but it'll hopefully be closer to where we want to end up and provide a better layout.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

OK, jaws and I did some pairing here and have good and bad news.

The good news is that there's a bug in the SDK "cover" mode that's easy to fix: changing the code at line 12988 to:

          var width = changeSet.width ? parseInt(changeSet.width[1], 10) : container.offsetWidth,
              height = changeSet.height ? parseInt(changeSet.height[1], 10) : container.offsetHeight;

avoids trying do computations on a string containing "px", which makes things somewhat better.

The bad news is that after substantial poking, we're back to a game of Jenga, and it appears that the new "cover" mode is different enough from what was there before that even using it is likely to entail non-trivial changes with non-trivial risk.

The one obvious idea we came up with to mitigate the risk (and it's not clear how practical this is) would be if the SDK could grow a third fitMode called something like "tokbox22" that would preserve the existing behavior to give us time to decouple the various changes that we're trying to make.

So failing that, it seems like a reasonable step to simply move forward with the "contain" prototype that Mike's working on.

Comment 35

[aoberoi]

hey guys, sorry for my lack of time to devote to this over the past couple days. had fires to fight.

there's been a lot of conversation so i just want to clarify my understanding of the current situation. do we all agree with the following?

1) the remote view should be fitMode: "contain"
2) the local view could also be fitMode: "contain" as long as the element was dynamically sized to have the same aspect ratio as the underlying video dimensions. in js with opentok, this is attainable from publisher.stream.videoDimensions{.width|.height}.
3) there is currently some bugginess around dynamically resizing publisher/subscriber elements who have fitMode: "contain". (the specifics are yet to be characterized but i think dmose is on the trail with comment 34)

open question: how does screensharing interact with all of this? does that feature invalidate any of the above? perhaps, there is no local video display for screensharing (to avoid the hall of mirrors effect)?

Comment 36

[aoberoi]

ah, i need to amend some statements.

2) as long as the element was dynamically sized to have the same aspect ratio as the underlying video dimensions, the local view could be either fitMode: "contain" or "cover". in js with opentok, the dimensions are attainable from publisher.stream.videoDimensions{.width|.height}.

3) there is currently some bugginess around dynamically resizing publisher/subscriber elements who have fitMode: "cover" (the specifics are yet to be characterized but i think dmose is on the trail with comment 34).

Comment 37

[Mark Banner (:standard8)]

In the 2.4.0 package, I notice there is now a dependencies.js file. However, neither the sdk, nor the documents reference this file.

Is this something we should be including in the standalone/desktop (what's the purpose of it?) or was it included by mistake?

Comment 38

[Mark Banner (:standard8)]

Attachment: Part 1 - Update OpenTok library to v2.4.0 for Loop.

Basic update to 2.4.0, includes changes to the functional test (note: depends on bug 1126199), and initial css modifications. The cover vs contain stuff gets fixed in part 2.

Comment 39

[Mark Banner (:standard8)]

Attachment: Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

This is the patch to get us working with contain mode.

Comment 40

[msander]

(In reply to Mark Banner (:standard8) from comment #37)
> In the 2.4.0 package, I notice there is now a dependencies.js file. However,
> neither the sdk, nor the documents reference this file.
> 
> Is this something we should be including in the standalone/desktop (what's
> the purpose of it?) or was it included by mistake?

I believe that is an artifact of some refactoring that is in progress, and does not need to be included. Rolly, could you comment on this?

Also, we are going to have a patch for v2.4 later this week or early next to fix some layout issues that occur when resizing the video element.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8555266 [details] [diff] [review]
Part 1 - Update OpenTok library to v2.4.0 for Loop.

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

Looks good, r=dmose with any appropriate nits addressed, and on the assumption that this will be landing together with the fit-mode contain patch.

::: browser/components/loop/content/shared/css/conversation.css
@@ +509,5 @@
>   * about the generated OT markup, any change will break it
>   */
>  .local-stream.local-stream-audio,
>  .standalone .OT_subscriber .OT_video-poster,
> +.fx-embedded .OT_subscriber .OT_video-poster,

The old selector here would have matched both subscriber (local) and publisher (remote).  Was this intentionally narrowed to only match subscriber?

If not, I assume we want this to actually be:

.fx-embedded .OT_widget-container .OT_video-poster.	

If so, it'd be helpful to add a comment describing why the two cases are treated differently.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +48,5 @@
>  
>        // At this state we init the publisher, even though we might be waiting for
>        // the initial connect of the session. This saves time when setting up
>        // the media.
> +      this.publisherConfig.fitMode = "contain";

Would it be worth DRYing up the code by dropping this (it's already handled by getDefaultPublisherConfig, I think).  Mostly since we intend to land this together with the next patch, which drops the later use of "cover" entirely.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8555267 [details] [diff] [review]
Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

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

So far so good.  Here are some initial questions/suggestions that I'd love to hear your thoughts on. I'm looking forward to seeing how the unit tests shake out as well.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +481,5 @@
>        // do a full reset here.
>        this.setStoreState(this.getInitialStoreState());
> +    },
> +
> +    videoDimensionsChanged: function(actionData) {

JSDoc might make this function easier to read, I think.  Less abstract variable names might help too.

@@ +487,5 @@
> +      //       we'll need to make this support multiple remotes as well. Good
> +      //       starting point for video tiling.
> +      var storeProp = (actionData.isLocal ? "local" : "remote") + "VideoDimensions";
> +      var nextState = {};
> +      nextState[storeProp] = this._storeState[storeProp];

Why reach inside _storeStore rather than use getStoreState?

::: browser/components/loop/content/shared/js/mixins.js
@@ +240,5 @@
> +     * @param  {Object} dimensions Object containing 'width' and 'height' properties
> +     * @return {Object}            Contains the indexed aspect ratio for 'width'
> +     *                             and 'height' assigned to the corresponding
> +     *                             properties.
> +     */

I've not run across this terminology and way of handling aspect ratios before.  The documentation helps, but I'm finding the terminology here to make things more difficult to reason about, since there are multiple terms used in (as far as I know) new or unusual ways.  

What's the motivation for this conceptual model?  Is there information that's implicitly overloaded into this model on top of what's expressed in the more typical way that aspect ratio is handled (eg for 640 x 480, one might more typically see 4:3).

@@ +258,5 @@
>       * display area changes.
> +     *
> +     * Buffer the calls to this function to make sure we don't overflow the stack
> +     * with update calls when many 'resize' event are fired, to prevent blocking
> +     * the event loop.

This method is large and it jumps across multiple layers of abstractions.  In order to make it easier to both reason about and get reasonable test coverage for, I'd strongly suggest chunking it up a bunch, probably by at least extracting the lambdas and the various low-level calculations into their own functions.

Reusing a standard debounce function (eg like the thing we have built-in from lodash) might be a win too.

Comment 43

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #41)
> ::: browser/components/loop/content/shared/css/conversation.css
> @@ +509,5 @@
> >   * about the generated OT markup, any change will break it
> >   */
> >  .local-stream.local-stream-audio,
> >  .standalone .OT_subscriber .OT_video-poster,
> > +.fx-embedded .OT_subscriber .OT_video-poster,
> 
> The old selector here would have matched both subscriber (local) and
> publisher (remote).  Was this intentionally narrowed to only match
> subscriber?

The local stream is handled on the following line (for both standalone & desktop):

.local-stream-audio .OT_publisher .OT_video-poster

So I don't think we need to do anything.

> ::: browser/components/loop/content/shared/js/otSdkDriver.js
> @@ +48,5 @@
> >  
> >        // At this state we init the publisher, even though we might be waiting for
> >        // the initial connect of the session. This saves time when setting up
> >        // the media.
> > +      this.publisherConfig.fitMode = "contain";
> 
> Would it be worth DRYing up the code by dropping this (it's already handled
> by getDefaultPublisherConfig, I think).  Mostly since we intend to land this
> together with the next patch, which drops the later use of "cover" entirely.

Oops, that was an oversight and I thought it was removed in the part 2, it wasn't, so I'll just remove that and the cover one from this patch.

Comment 44

[Mark Banner (:standard8)]

Attachment: Part 1 - Update OpenTok library to v2.4.0 for Loop.

Update patch to fix comments and bitrot.

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #43)
> (In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment
> #41)
> > ::: browser/components/loop/content/shared/css/conversation.css
> > @@ +509,5 @@
> > >   * about the generated OT markup, any change will break it
> > >   */
> > >  .local-stream.local-stream-audio,
> > >  .standalone .OT_subscriber .OT_video-poster,
> > > +.fx-embedded .OT_subscriber .OT_video-poster,
> > 
> > The old selector here would have matched both subscriber (local) and
> > publisher (remote).  Was this intentionally narrowed to only match
> > subscriber?
> 
> The local stream is handled on the following line (for both standalone &
> desktop):
> 
> .local-stream-audio .OT_publisher .OT_video-poster
> 
> So I don't think we need to do anything.

Are you sure?  Skimming the code suggests to me that that selector is usually added to the classset ONLY if video is disabled.  That said, the actual design here is that we don't need want to display avatar while the local camera is being acquired on audio-video calls (which would seem like a reasonable choice to me), could you please add a comment briefly describing the design that the CSS is attempting to implement?  Maybe a comment is worth adding in any case...

Comment 46

[Mike de Boer [:mikedeboer]]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #42)
> JSDoc might make this function easier to read, I think.  Less abstract
> variable names might help too.

Will add docs. Regarding the variable names; I'm copying the pattern as seen in the rest of the store. I'll see what I can do here.

> Why reach inside _storeStore rather than use getStoreState?

Like above, I'm following what seems to be the convention _inside_ a store itself. If I we're accessing the store state outside of this file, I'd use `getStoreState()`.

> I've not run across this terminology and way of handling aspect ratios
> before.  The documentation helps, but I'm finding the terminology here to
> make things more difficult to reason about, since there are multiple terms
> used in (as far as I know) new or unusual ways.  
> 
> What's the motivation for this conceptual model?  Is there information
> that's implicitly overloaded into this model on top of what's expressed in
> the more typical way that aspect ratio is handled (eg for 640 x 480, one
> might more typically see 4:3).

In fact, this is exactly the same as 4:3 like notations, which are merely human-readable forms of their fractional counterparts. 4:3 === 1:0.75 and 16:9 === 1:0.5625. So I'm using the aspect ratios in their original form, because they're easier to do calculus with. I'll mention this in the comment as well.

> 
> @@ +258,5 @@
> >       * display area changes.
> > +     *
> > +     * Buffer the calls to this function to make sure we don't overflow the stack
> > +     * with update calls when many 'resize' event are fired, to prevent blocking
> > +     * the event loop.
> 
> This method is large and it jumps across multiple layers of abstractions. 
> In order to make it easier to both reason about and get reasonable test
> coverage for, I'd strongly suggest chunking it up a bunch, probably by at
> least extracting the lambdas and the various low-level calculations into
> their own functions.
> 
> Reusing a standard debounce function (eg like the thing we have built-in
> from lodash) might be a win too.

I'll use the debounce method here, sure. As a side-note: I'm personally a bit wary about using libraries to solve simple problems like this, since their implementation is usually overly generic. Same goes for stores and event emitters; they're so simple that it's not worth dragging a generic library - like Backbone - in. I'm not willing to start a discussion over this, though, so I'll simply use this. Just food for thought, perhaps?

Comment 47

[Mike de Boer [:mikedeboer]]

Attachment: Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

I left the setTimeout() code in there, because we can't use the `_.debounce()` function: `updateVideoContainer()` is called from multiple places, like 'orientationchange', 'resize' event handlers and `updateVideoDimensions` (directly).
We'd have to add work around to keep a reference to the function that `debounce()` returns and call that in the three distinct places. Or something like that. Therefore I decided to keep this approach.

Comment 48

[Mike de Boer [:mikedeboer]]

Attachment: Part 3 - add tests for contain mode functionality in the MediaSetup mixin

Comment 49

[Mike de Boer [:mikedeboer]]

Attachment: Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

Forgot to remove the max-width: 1000 rule for standalone and the now jarring dark-gray background.

Comment 50

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8556457 [details] [diff] [review]
Part 3 - add tests for contain mode functionality in the MediaSetup mixin

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

Moving review req to Std8 as discussed in this today's standup.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8556463 [details] [diff] [review]
Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

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

Moving review req to Std8 as discussed in this today's standup.

Comment 52

[Mark Banner (:standard8)]

Attachment: Part 4 - Fix the audio-only display of avatars for the new sdk.

I took a look at Dan's comments in a bit more detail with only part 1 applied (it was more obvious there).

Looking at the old version, it appears it only really worked as the background was being applied to two elements, one of which was opacity 0.25. That's why the css was complicated on multiple elements with no obvious reason.

Additionally, in the old version, I think the subscribed stream video element was still taking effect, so the avatar background still appeared dull.

Given that it wasn't obvious, and was broken in the new version, I've re-worked it. We now use more generic ways of detecting audio-only and applying the background. I've also turned off the display of the actual video elements, as they tend to cause artifacts with the audio-only mode.

I've left the remote video stream as "dull" if it is audio-only, as it looked better than having it bright.

I've also added explanations for all the bits of css so that we know in future what they are actually trying to do and why.

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 8556463 [details] [diff] [review]
Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

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

Looks good, r=Standard8 with the comments addressed.

::: browser/components/loop/content/shared/js/mixins.js
@@ +186,5 @@
> +      var cache = this._videoDimensionsCache[which];
> +      var cacheKeys = Object.keys(cache);
> +      var changed = false;
> +      Object.keys(newDimensions).forEach(function(videoType) {
> +        if (cacheKeys.indexOf(videoType) == -1) {

nit: I think we should use === and !== to be consistent with the rest of the content code. If we want to change the style we should agree this elsewhere.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +104,5 @@
>       * Disconnects the sdk session.
>       */
>      disconnectSession: function() {
>        if (this.session) {
> +        this.session.off("streamCreated streamDestroyed connectionDestroyed sessionDisconnected");

I think we need to add "streamPropertyChanged" in here. Would be nice to split the line as well.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +296,5 @@
> +    updateLocalCameraPosition: function(ratio) {
> +      var node = this._getElement(".local");
> +      var parent = node.offsetParent || this._getElement(".media");
> +      // The local camera view should be a sixth of the size of its offset parent
> +      // and positioned to overlap with the remote stream at a third of its width.

Change third to a quarter.

@@ +309,5 @@
> +        // Now position the local camera view correctly with respect to the remote
> +        // video stream.
> +        var remoteVideoDimensions = this.getRemoteVideoDimensions();
> +        var offsetX = (remoteVideoDimensions.streamWidth + remoteVideoDimensions.offsetX);
> +        node.style.left = (offsetX - ((targetWidth * ratio.height) / 4)) + "px";

Please add a comment why this is ratio.height and not width.

Comment 54

[Mark Banner (:standard8)]

Comment on attachment 8556457 [details] [diff] [review]
Part 3 - add tests for contain mode functionality in the MediaSetup mixin

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

Please can you add some tests for the additions to otSdkDriver to check we're dispatching the new actions at the right time. There's already some event based examples in the test file.

::: browser/components/loop/test/shared/mixins_test.js
@@ +271,5 @@
>            };
>  
>            rootObject.events.resize();
>  
> +          window.setTimeout(function() {

rather than doing this, in the first beforeEach, please add:

sandbox.useFakeTimers();

You can then drop the async nature of the tests and use:

sandbox.clock.tick(<delay>);

to advance a specific time in ms.

Comment 55

[Mike de Boer [:mikedeboer]]

Comment on attachment 8557043 [details] [diff] [review]
Part 4 - Fix the audio-only display of avatars for the new sdk.

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

LGTM, nice cleanup and I mostly appreciate the excellent comments! ;)

::: browser/components/loop/content/shared/css/conversation.css
@@ +523,5 @@
> +  opacity: 1
> +}
> +
> +/*
> + * In audio-only mode, don't display the video element, doing so interfers

nit: interferes

Comment 56

[Mike de Boer [:mikedeboer]]

Attachment: Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

Addressed review comments. Carrying over r=Standard.

Comment 57

[Mike de Boer [:mikedeboer]]

Attachment: Part 3 - add tests for contain mode functionality in the MediaSetup mixin

Comment 58

[Mike de Boer [:mikedeboer]]

Attachment: Part 2 - Add support for using 'contain' mode for all video streams Loop publishes and resize/ position the elements based on their aspect ratio

Fixes a massive oversight from my end, which was easy to miss, I admit. So this was discovered while writing unit tests; proving their use yet again.

Carrying over r=Standard8

Comment 59

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #52)
>
> Given that it wasn't obvious, and was broken in the new version, I've
> re-worked it. [...]
>
> I've also added explanations for all the bits of css so that we know in
> future what they are actually trying to do and why.

Awesome; thanks so much!

Comment 60

[Mark Banner (:standard8)]

Comment on attachment 8557106 [details] [diff] [review]
Part 3 - add tests for contain mode functionality in the MediaSetup mixin

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

There was one minor issue in roomsViews_test.js with some of the changes in activeRoomStore. I've fixed it locally, and will land with it. The first part of bug 1122032 should make changing it less necessary in future.

Comment 61

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/dcf6ab90ff97
https://hg.mozilla.org/integration/fx-team/rev/9e204cdb756c
https://hg.mozilla.org/integration/fx-team/rev/e8201b76639d
https://hg.mozilla.org/integration/fx-team/rev/2197b32a55ed

Comment 62

[Mark Banner (:standard8)]

We pushed a follow-up test fix for an intermittent failure that this showed up - I don't think its directly related to this bug, but more just timing has changed and hence was exposed a bit more:

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

Comment 63

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dcf6ab90ff97
https://hg.mozilla.org/mozilla-central/rev/9e204cdb756c
https://hg.mozilla.org/mozilla-central/rev/e8201b76639d
https://hg.mozilla.org/mozilla-central/rev/2197b32a55ed
https://hg.mozilla.org/mozilla-central/rev/f59defcc1b36

Comment 64

[rolly]

(In reply to msander from comment #40)
> (In reply to Mark Banner (:standard8) from comment #37)
> > In the 2.4.0 package, I notice there is now a dependencies.js file. However,
> > neither the sdk, nor the documents reference this file.
> > 
> > Is this something we should be including in the standalone/desktop (what's
> > the purpose of it?) or was it included by mistake?
> 
> I believe that is an artifact of some refactoring that is in progress, and
> does not need to be included. Rolly, could you comment on this?
> 
> Also, we are going to have a patch for v2.4 later this week or early next to
> fix some layout issues that occur when resizing the video element.

That's correct, it's just a build artifact. The next version won't even emit it, but you can safely ignore it in this version as the entire contents of that file has already been included in the main JS file.

Comment 65

[Mark Banner (:standard8)]

(In reply to rolly from comment #64)
> That's correct, it's just a build artifact. The next version won't even emit
> it, but you can safely ignore it in this version as the entire contents of
> that file has already been included in the main JS file.

Great, thanks for the confirmation.

Comment 66

[Mark Banner (:standard8)]

Shell, why the change of iteration? This was definitely completed in 38.2 as per comment 61 and comment 63.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Might be an issue with the queries used to set those flags? This bug had activity after it was resolved which might be throwing things off.

Comment 68

[:shell escalante]

This was me - i misunderstood when it was completed based on comments and changed the flag when doing clean-up.  a lot of bug have older iterations when closed (if they ran over to the next iterations).  Sorry about that one.  Usually I just look at check-in dates - but missed on this one.