Bug 1132064 - Local video is sometimes displayed in the wrong location on the standalone Loop UI. r=mikedeboer
authorMark Banner <standard8@mozilla.com>
Wed, 11 Feb 2015 20:28:11 +0000
changeset 246350 61623ff5e751555d3c5d82717c2d9624b1e8d5be
parent 246349 9e88eb0cb0d28bd6cbbf7e937bbbdbaaeae56c25
child 246351 19e0922d6d0c762fb6d275dc3a559bd5b20f8af2
push id7677
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 18:11:24 +0000
treeherdermozilla-aurora@f531d838c055 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1132064
milestone38.0a1
Bug 1132064 - Local video is sometimes displayed in the wrong location on the standalone Loop UI. r=mikedeboer
browser/components/loop/content/shared/js/mixins.js
browser/components/loop/test/shared/mixins_test.js
--- a/browser/components/loop/content/shared/js/mixins.js
+++ b/browser/components/loop/content/shared/js/mixins.js
@@ -290,25 +290,47 @@ loop.shared.mixins = (function() {
         // centered inside the video element.
         // We'll need to deal with more than one remote video stream whenever
         // that becomes something we need to support.
         if (width) {
           remoteVideoDimensions = {
             width: width,
             height: node.offsetHeight
           };
+
           var ratio = this._videoDimensionsCache.remote[videoType].aspectRatio;
+          // Leading axis is the side that has the smallest ratio.
           var leadingAxis = Math.min(ratio.width, ratio.height) === ratio.width ?
             "width" : "height";
-          var slaveSize = remoteVideoDimensions[leadingAxis] +
-            (remoteVideoDimensions[leadingAxis] * (1 - ratio[leadingAxis]));
-          remoteVideoDimensions.streamWidth = leadingAxis === "width" ?
-            remoteVideoDimensions.width : slaveSize;
-          remoteVideoDimensions.streamHeight = leadingAxis === "height" ?
-            remoteVideoDimensions.height: slaveSize;
+          var slaveAxis = leadingAxis === "height" ? "width" : "height";
+
+          // We need to work out if the leading axis of the video is full, by
+          // calculating the expected length of the leading axis based on the
+          // length of the slave axis and aspect ratio.
+          var leadingAxisFull = remoteVideoDimensions[slaveAxis] * ratio[leadingAxis] >
+            remoteVideoDimensions[leadingAxis];
+
+          if (leadingAxisFull) {
+            // If the leading axis is "full" then we need to adjust the slave axis.
+            var slaveAxisSize = remoteVideoDimensions[leadingAxis] / ratio[leadingAxis];
+
+            remoteVideoDimensions.streamWidth = leadingAxis === "width" ?
+              remoteVideoDimensions.width : slaveAxisSize;
+            remoteVideoDimensions.streamHeight = leadingAxis === "height" ?
+              remoteVideoDimensions.height: slaveAxisSize;
+          } else {
+            // If the leading axis is not "full" then we need to adjust it, based
+            // on the length of the leading axis.
+            var leadingAxisSize = remoteVideoDimensions[slaveAxis] * ratio[leadingAxis];
+
+            remoteVideoDimensions.streamWidth = leadingAxis === "height" ?
+              remoteVideoDimensions.width : leadingAxisSize;
+            remoteVideoDimensions.streamHeight = leadingAxis === "width" ?
+              remoteVideoDimensions.height: leadingAxisSize;
+          }
         }
       }, this);
 
       // Supply some sensible defaults for the remoteVideoDimensions if no remote
       // stream is connected (yet).
       if (!remoteVideoDimensions) {
         var node = this._getElement(".remote");
         var width = node.offsetWidth;
--- a/browser/components/loop/test/shared/mixins_test.js
+++ b/browser/components/loop/test/shared/mixins_test.js
@@ -190,16 +190,17 @@ describe("loop.shared.mixins", function(
         comp = TestUtils.renderIntoDocument(React.createElement(TestComp));
 
         sinon.assert.calledOnce(onDocumentHiddenStub);
       });
   });
 
   describe("loop.shared.mixins.MediaSetupMixin", function() {
     var view, TestComp, rootObject;
+    var localElement, remoteElement, screenShareElement;
 
     beforeEach(function() {
       TestComp = React.createClass({
         mixins: [loop.shared.mixins.MediaSetupMixin],
         render: function() {
           return React.DOM.div();
         }
       });
@@ -220,42 +221,161 @@ describe("loop.shared.mixins", function(
         removeEventListener: function(eventName) {
           delete this.events[eventName];
         }
       };
 
       sharedMixins.setRootObject(rootObject);
 
       view = TestUtils.renderIntoDocument(React.createElement(TestComp));
+
+      sandbox.stub(view, "getDOMNode").returns({
+        querySelector: function(classSelector) {
+          if (classSelector.contains("local")) {
+            return localElement;
+          } else if (classSelector.contains("screen")) {
+            return screenShareElement;
+          }
+          return remoteElement;
+        }
+      });
+    });
+
+    afterEach(function() {
+      localElement = null;
+      remoteElement = null;
+      screenShareElement = null;
     });
 
     describe("#getDefaultPublisherConfig", function() {
       it("should provide a default publisher configuration", function() {
         var defaultConfig = view.getDefaultPublisherConfig({publishVideo: true});
 
         expect(defaultConfig.publishVideo).eql(true);
       });
     });
 
-    describe("Events", function() {
-      var localElement, remoteElement, screenShareElement;
+    describe("#getRemoteVideoDimensions", function() {
+      var localVideoDimensions, remoteVideoDimensions;
 
       beforeEach(function() {
-        sandbox.stub(view, "getDOMNode").returns({
-          querySelector: function(classSelector) {
-            if (classSelector.contains("local")) {
-                return localElement;
-            } else if (classSelector.contains("screen")) {
-                return screenShareElement;
-            }
-            return remoteElement;
+        localVideoDimensions = {
+          camera: {
+            width: 640,
+            height: 480
           }
-        });
+        };
       });
 
+      it("should fetch the correct stream sizes for leading axis width and full",
+        function() {
+          remoteVideoDimensions = {
+            camera: {
+              width: 240,
+              height: 320
+            }
+          };
+          remoteElement = {
+            offsetWidth: 480,
+            offsetHeight: 700
+          };
+
+          view.updateVideoDimensions(localVideoDimensions, remoteVideoDimensions);
+          var result = view.getRemoteVideoDimensions();
+
+          expect(result.width).eql(remoteElement.offsetWidth);
+          expect(result.height).eql(remoteElement.offsetHeight);
+          expect(result.streamWidth).eql(remoteElement.offsetWidth);
+          // The real height of the stream accounting for the aspect ratio.
+          expect(result.streamHeight).eql(640);
+          expect(result.offsetX).eql(0);
+          // The remote element height (700) minus the stream height (640) split in 2.
+          expect(result.offsetY).eql(30);
+        });
+
+      it("should fetch the correct stream sizes for leading axis width and not full",
+        function() {
+          remoteVideoDimensions = {
+            camera: {
+              width: 240,
+              height: 320
+            }
+          };
+          remoteElement = {
+            offsetWidth: 640,
+            offsetHeight: 480
+          };
+
+          view.updateVideoDimensions(localVideoDimensions, remoteVideoDimensions);
+          var result = view.getRemoteVideoDimensions();
+
+          expect(result.width).eql(remoteElement.offsetWidth);
+          expect(result.height).eql(remoteElement.offsetHeight);
+          // Aspect ratio modified from the height.
+          expect(result.streamWidth).eql(360);
+          expect(result.streamHeight).eql(remoteElement.offsetHeight);
+          // The remote element width (640) minus the stream width (360) split in 2.
+          expect(result.offsetX).eql(140);
+          expect(result.offsetY).eql(0);
+        });
+
+      it("should fetch the correct stream sizes for leading axis height and full",
+        function() {
+          remoteVideoDimensions = {
+            camera: {
+              width: 320,
+              height: 240
+            }
+          };
+          remoteElement = {
+            offsetWidth: 700,
+            offsetHeight: 480
+          };
+
+          view.updateVideoDimensions(localVideoDimensions, remoteVideoDimensions);
+          var result = view.getRemoteVideoDimensions();
+
+          expect(result.width).eql(remoteElement.offsetWidth);
+          expect(result.height).eql(remoteElement.offsetHeight);
+          // The real width of the stream accounting for the aspect ratio.
+          expect(result.streamWidth).eql(640);
+          expect(result.streamHeight).eql(remoteElement.offsetHeight);
+          // The remote element width (700) minus the stream width (640) split in 2.
+          expect(result.offsetX).eql(30);
+          expect(result.offsetY).eql(0);
+        });
+
+      it("should fetch the correct stream sizes for leading axis height and not full",
+        function() {
+          remoteVideoDimensions = {
+            camera: {
+              width: 320,
+              height: 240
+            }
+          };
+          remoteElement = {
+            offsetWidth: 480,
+            offsetHeight: 640
+          };
+
+          view.updateVideoDimensions(localVideoDimensions, remoteVideoDimensions);
+          var result = view.getRemoteVideoDimensions();
+
+          expect(result.width).eql(remoteElement.offsetWidth);
+          expect(result.height).eql(remoteElement.offsetHeight);
+          expect(result.streamWidth).eql(remoteElement.offsetWidth);
+          // Aspect ratio modified from the width.
+          expect(result.streamHeight).eql(360);
+          expect(result.offsetX).eql(0);
+          // The remote element width (640) minus the stream width (360) split in 2.
+          expect(result.offsetY).eql(140);
+        });
+    });
+
+    describe("Events", function() {
       describe("resize", function() {
         it("should update the width on the local stream element", function() {
           localElement = {
             offsetWidth: 100,
             offsetHeight: 100,
             style: { width: "0%" }
           };
 
@@ -362,31 +482,16 @@ describe("loop.shared.mixins", function(
           expect(view._videoDimensionsCache.remote.camera.width)
             .eql(remoteVideoDimensions.camera.width);
           expect(view._videoDimensionsCache.remote.camera.height)
             .eql(remoteVideoDimensions.camera.height);
           expect(view._videoDimensionsCache.remote.camera.aspectRatio.width).eql(1);
           expect(view._videoDimensionsCache.remote.camera.aspectRatio.height)
             .eql(0.32857142857142857);
         });
-
-        it("should fetch remote video stream dimensions correctly", function() {
-          remoteElement = {
-            offsetWidth: 600,
-            offsetHeight: 320
-          };
-
-          var remoteVideoDimensions = view.getRemoteVideoDimensions();
-          expect(remoteVideoDimensions.width).eql(remoteElement.offsetWidth);
-          expect(remoteVideoDimensions.height).eql(remoteElement.offsetHeight);
-          expect(remoteVideoDimensions.streamWidth).eql(534.8571428571429);
-          expect(remoteVideoDimensions.streamHeight).eql(remoteElement.offsetHeight);
-          expect(remoteVideoDimensions.offsetX).eql(32.571428571428555);
-          expect(remoteVideoDimensions.offsetY).eql(0);
-        });
       });
     });
   });
 
   describe("loop.shared.mixins.AudioMixin", function() {
     var view, fakeAudio, TestComp;
 
     beforeEach(function() {