Closed Bug 1132064 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
3

Tracking

(firefox38 fixed)

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8562806 - Flags: review?(mdeboer)
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 '.'
Attachment #8562806 - Flags: review?(mdeboer) → review+
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.
Target Milestone: --- → mozilla38
(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.
https://hg.mozilla.org/mozilla-central/rev/61623ff5e751
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.