Closed
Bug 1132064
Opened 8 years ago
Closed 8 years ago
Local video is sometimes displayed in the wrong location on the standalone
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox38 fixed)
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(2 files)
276.87 KB,
image/png
|
Details | |
11.34 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
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
Comment 4•8 years ago
|
||
(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.
Updated•8 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•