1132064 - Local video is sometimes displayed in the wrong location on the standalone

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

Attachment: Local video in wrong position

Sometimes the local video is displayed in the wrong location on the standalone window.

This is most obvious when you're screen sharing with a wide, short window on a fairly large screen, e.g. see the attached screenshot.

Comment 1

[Mark Banner (:standard8)]

Attachment: Local video is sometimes displayed in the wrong location on the standalone Loop UI.

I took a look at the numbers and did some diagrams and things. I think the main	thing we weren't accounting for	was the	mode where the leading axis had already filled up the axis height or width, and so the pillar/letterbox would be the other way to what was expected.

I've therefore taken that into account, and I think in doing so hopefully simplified some of the calculations. I've tried various scenarios and I think we get it right all the time now.

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8562806 [details] [diff] [review]
Local video is sometimes displayed in the wrong location on the standalone Loop UI.

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

I think that makes sense too. Thanks for diving into this - would love to see those diagrams ;) But in all seriousness, a bit of ASCII art inside a comment never hurt anybody...

::: browser/components/loop/test/shared/mixins_test.js
@@ +283,5 @@
> +
> +          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

nit: missing '.'

@@ +308,5 @@
> +          var result = view.getRemoteVideoDimensions();
> +
> +          expect(result.width).eql(remoteElement.offsetWidth);
> +          expect(result.height).eql(remoteElement.offsetHeight);
> +          // aspect ratio modified from the height

nit: 'Aspect' and missing '.'

@@ +361,5 @@
> +
> +          expect(result.width).eql(remoteElement.offsetWidth);
> +          expect(result.height).eql(remoteElement.offsetHeight);
> +          expect(result.streamWidth).eql(remoteElement.offsetWidth);
> +          // aspect ratio modified from the width

nit: 'Aspect' and missing '.'

Comment 3

[Mark Banner (:standard8)]

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

I'll have a think about diagrams when I do the screenshare bug. Not sure that's too easy though, most of mine were just lines and lengths.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #3)
> I'll have a think about diagrams when I do the screenshare bug. Not sure
> that's too easy though, most of mine were just lines and lengths.

Ooh, that comment was only semi-serious :) I think the code is well documented already.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61623ff5e751