1101378 - video self-image can be cropped, falsely making users think they're transmitting less video than they are

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

The self image seen by a link-clicker in Rooms is badly cropped at the bottom.  The self-image appears to be about 16:9, but the default capture size is 640x480 (~4:3).  Also, we don't know for sure what aspect ratio will be captured (especially for Chrome), though that's a secondary concern. 

Best would be adaptive to capture resolution (get the videowidth/height and resize to that ratio ).   Next best would be to assume 640x480, and inset rather than crop if we have a choice (likely we don't).

Comment 1

[:shell escalante]

believe Niko and Jaws are planning CSS love for the stand-alone.

Comment 2

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
This patch is for the standalone app. We may choose to uplift it for the sake of shared code with desktop.  Marking this as tracking Fx35 so we get this fixed before Fx35 goes to Release.

Comment 3

[:shell escalante]

do you have bandwidth for this for this week?  if not, Maire will work with it.

Comment 4

[Mark Banner (:standard8)]

Dan's going to look at this.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Last week, :andreio and :dholbert were able to come up with some CSS that made life better, in part because we figured out a way to reproduce the bottom-cropping in a non-trivial way.  Do either of you guys know the steps to reproduce?  Today, I've only able to reproduce the bottom-cropping in a very very trivial way (small number of pixels no matter how I resize).

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Daniel, I can't imagine you'll have a problem being quoted here, but if I'm wrong, I'm very sorry.  Trying to get this fix turned around quickly).  Last week, dholbert said, elsewhere (lightly edited):

> Probably not useful as an actual fix, but if you tweak this script file...
> https://hello.firefox.com/shared/libs/sdk.js
> to comment out lines 5351-5352 (which sets props.height & props.top)
> and 5357-5358 (which sets props.width & props.left), then the bug goes
> away. That's the script that dynamically sets the height & width of the
> video's container-div, to crop it inside its overflow:hidden parent.

> (With those lines removed, that container stays the same size as its
> parent, and nothing overflows & is cropped. (and the video content gets
> to honor its "object-fit:contain" default-styling in that region, and
> get letterboxed.)

> Also, the comment above the function does say "This code positions the
> video element so that we don't get any letterboxing", so it does explicitly
> intend to crop the video". 

> Two options come to mind for workaround the SDK behavior, from discussing
> w/ Andrei:

> (1) use a !important style rule to override those dynamic
> property-settings on the abspos ancestor (the thing with
> class=OT_video-container). Specifically, we need height:100%; width:
> 100%; top: 0; left: 0; -- all !important, to override the JS tweaks.

> (2) make our video element itself be absolutely positioned, and add
> some JS (to be invoked on each window-resize) to walk up the tree to
> find our video's abspos ancestor and its overflow:hidden parent, and set
> our <video> top/left CSS properties to the negation of the abspos
> ancestor's top/left properties, and set our height/width to match the
> overflow:hidden-element's size.  So, the <video> element would then
> exactly fit inside of the overflow:hidden ancestor, and none of it would
> be cropped.

> Both of these are pretty fragile, if they change their CSS class names
> (for the first option) or page structure (in the second option).

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix _only_ joiner self-image cropped on standalone

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

https://www.evernote.com/shard/s3/sh/af4a502e-a0ce-4cb4-a4c9-740e7554f95e/727ff3777dec74b3557281643438cfee is how the above patch plays out on my Mac.  Instead of potentially cropping information that might actually be sent to the other end (annoying and potential privacy fail), it has the opposite problem that it shows information that might not be displayed on the other end (in this case, my nose -- annoying, but not a potential privacy fail).

So I suspect this is a reasonable patch to take and uplift.

That said, this is really just band-aiding over one of the larger set of problems described in part by bug 1104051.  So if we're likely to get a fix for that that's low-risk enough before 35 launches, the patch in this bug may not be worth doing (though I could certainly land it now.

More holistically, I think there's some big picture thinking here around how close we can/should try to come to the ideal case of where the local image on either side shows _exactly_ the video that will be presented on the other side.  We may want to spin off a separate bug for that.

:NiKo`, would love your thoughts (especially, but not necessarily exclusively) on this patch itself and what to do about it, given your work on bug 1104051.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug (stubby browser-window causes cropping in selfview pane)

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #5)
> Do either of you guys know the steps
> to reproduce?  Today, I've only able to reproduce the bottom-cropping in a
> very very trivial way (small number of pixels no matter how I resize).

I can easily reproduce extreme cropping by simply using a window that's short-and-squat (or skinny-and-tall). Screenshot attached, w/ both ends shown. (self-view on top, & the actually-sent-video on bottom)

For demonstration purposes, I held up a water bottle, which is completely off-screen in the preview, but is shown in the actually-sent video.

Comment 10

[Daniel Holbert [:dholbert]]

(dmose: I suspect your request-for-STR might've been about a failure-case that's different from what my screenshot shows -- but the short-window scenario is the only way I've seen this sort of bug.)

Comment 11

[Andrei Oprea [:andreio]]

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #5)
> Last week, :andreio and :dholbert were able to come up with some CSS that
> made life better, in part because we figured out a way to reproduce the
> bottom-cropping in a non-trivial way.  Do either of you guys know the steps
> to reproduce?  Today, I've only able to reproduce the bottom-cropping in a
> very very trivial way (small number of pixels no matter how I resize).

The screenshot that Daniel posted is the same thing I noticed.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #8)
> :NiKo`, would love your thoughts (especially, but not necessarily
> exclusively) on this patch itself and what to do about it, given your work
> on bug 1104051.

I'm mostly tweaking the layout in that bug (which is already super painful enough so I don't really bother trying to improve the video situation in there — tbh I'm mostly fighting not breaking it too much).

(In reply to Daniel Holbert [:dholbert] from comment #9)
> For demonstration purposes, I held up a water bottle, which is completely
> off-screen in the preview, but is shown in the actually-sent video.

Damn, that's a *very* good point. I'm kinda binary guy, so if we want perfect privacy compliance (I think we do), we should ensure *never* cropping video streams at all. That means always preserving cam original aspect ratio, and {letter|pillar}box whatever remaining space.

I'd love hearing UX' thoughts here (poke :sevaan).

Comment 13

[Maire Reavy [:mreavy]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #12)
> Damn, that's a *very* good point. I'm kinda binary guy, so if we want
> perfect privacy compliance (I think we do), we should ensure *never*
> cropping video streams at all. That means always preserving cam original
> aspect ratio, and {letter|pillar}box whatever remaining space.
> 
> I'd love hearing UX' thoughts here (poke :sevaan).

With my manager hat on, we *MUST* preserve video (and audio) privacy -- even at the expense of some ugliness.  The user needs to know that what he/she sees on the screen is what actually gets sent to the other side.  Aspect ratio is also important, but less important than cropping of the user's self video.  If we have to crop the user's self video (for any reason), then we can NOT send the cropped part of the video to the remote side of the call.  

With this requirement in mind, I'd also love to hear UX's thoughts.

Comment 14

[Maire Reavy [:mreavy]]

One other note: the reason this was triaged as "Fx35+, P1" is because of the privacy concern.  If this bug ONLY solved the privacy concern (such that the self video shown to the user matched, pixel for pixel, the video sent to the remote side), I would consider this bug resolved and I'd open a follow-on bug (or bugs) for addressing any additional issues.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

The patch here more-or-less fixes the most basic problem as described in comment 14 for the standalone case only.  The more or less part is that it's still possible to force the picture partway out of the viewport by resizing, but there's usually (always?) a scrollbar in that case.

The fx-embedded view still has privacy issues here, both in the default size of the chat window as well as the popped out version.

Next up: see whether it's practical to do a bit of automated testing here (I have a theory, but I'll timebox that investigation).

Comment 16

[Sevaan Franks [:sevaan]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #13)
> (In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my
> attention from comment #12)
> > Damn, that's a *very* good point. I'm kinda binary guy, so if we want
> > perfect privacy compliance (I think we do), we should ensure *never*
> > cropping video streams at all. That means always preserving cam original
> > aspect ratio, and {letter|pillar}box whatever remaining space.
> > 
> > I'd love hearing UX' thoughts here (poke :sevaan).
> 
> With my manager hat on, we *MUST* preserve video (and audio) privacy -- even
> at the expense of some ugliness.  The user needs to know that what he/she
> sees on the screen is what actually gets sent to the other side.  Aspect
> ratio is also important, but less important than cropping of the user's self
> video.  If we have to crop the user's self video (for any reason), then we
> can NOT send the cropped part of the video to the remote side of the call.  
> 
> With this requirement in mind, I'd also love to hear UX's thoughts.

I'm on side with this. I would rather letterbox than crop. Having said that, there are probably some layout tweaks we could do here to maximize the viewing area.

I've just started work on a mobile link clicker UI, and maybe we can revert to that layout when the window hits a certain size. See Bug 1083779.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: self-image can be cropped, meaning the user doesn't see the entire sent images

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: self-image can be cropped, meaning the user doesn't see the entire sent images

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8535275 [details] [diff] [review]
self-image can be cropped, meaning the user doesn't see the entire sent images

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

This fixes both the link-clicker and embedded sides of this issue (as far as I can tell, even crisp manual testing is not straightforward here, for reasons described in the CSS comment).

I've spun off bug 1110507 to work on fixing the cases where the user sizes the window in such a way that the local video is partially scrolled out of the viewport.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Images demonstrating the fix

Here's an image of the fix.  While it doesn't look pretty, nothing we've got looks pretty at this sort of resizing (which it would perhaps ideal to just disallow or force to scale with a transform or something.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

I tried it in current Chrome, and it seems to fix the problem there too.

Comment 22

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8535275 [details] [diff] [review]
self-image can be cropped, meaning the user doesn't see the entire sent images

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

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #20)

Looks good to me, and solves the privacy issue. Ship it!

::: browser/components/loop/content/shared/css/conversation.css
@@ +521,5 @@
> + * The !importants are necessary to override the SDK attempts to avoid
> + * letterboxing entirely.
> + *
> + * If we could easily use test video streams with the SDK (eg if initPublisher
> + * supported something like a "testMediaToStreamURI" parameter that it would

Great idea. Please file a bug about this, and assign it to tokbox.

@@ +522,5 @@
> + * letterboxing entirely.
> + *
> + * If we could easily use test video streams with the SDK (eg if initPublisher
> + * supported something like a "testMediaToStreamURI" parameter that it would
> + * use to source the stream rather than the output of gUM, it wouldn't be too9

Nit: s/too9/too

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [landed fx-team] self-image can be cropped, meaning the user doesn't see the entire sent images, r=NiKo`

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

After discussion on IRC, it looks like it's better to do this by monkeypatching gUM directly, so that the SDK is functionally tested too.  I've spun off bug 1110973 for that.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4cdd32ea6f7d

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8535739 [details] [diff] [review]
[landed fx-team] self-image can be cropped, meaning the user doesn't see the entire sent images, r=NiKo`

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: Significant privacy problem - users may send video they don't realize they're sending due to cropping.

[Describe test coverage new/current, TBPL]: On m-c momentarily.  Manually tested.

[Risks and why]: Important Privacy issue.  Very low risk - all CSS.

[String/UUID change made/needed]: none

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d18b94c716e

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c638300b2fc