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

RESOLVED FIXED in Firefox 35

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: dmose)

Tracking

({privacy})

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox35? fixed, firefox36 fixed, firefox37 fixed)

Details

(Whiteboard: [standalone][rooms])

Attachments

(3 attachments, 3 obsolete attachments)

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).
believe Niko and Jaws are planning CSS love for the stand-alone.
backlog: --- → Fx35+
Priority: -- → P1
Whiteboard: [standalone][rooms]
[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.
do you have bandwidth for this for this week?  if not, Maire will work with it.
Flags: needinfo?(mreavy)
Flags: needinfo?(jaws)
Dan's going to look at this.
Assignee: nobody → dmose
Flags: needinfo?(mreavy)
Flags: needinfo?(jaws)
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).
Flags: needinfo?(dholbert)
Flags: needinfo?(andrei.br92)
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).
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.
Flags: needinfo?(nperriault)
(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.
Flags: needinfo?(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.)
(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.
Flags: needinfo?(andrei.br92)
(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).
Flags: needinfo?(nperriault) → needinfo?(sfranks)
(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.
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.
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).
(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.
Flags: needinfo?(sfranks)
Keywords: privacy
Summary: room-joiner self-image badly cropped at bottom → video self-image can be cropped, falsely making users think they're transmitting less video than they are
Blocks: 1110507
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.
Attachment #8535275 - Flags: review?(nperriault)
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.
I tried it in current Chrome, and it seems to fix the problem there too.
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
Attachment #8535275 - Flags: review?(nperriault) → review+
Attachment #8535275 - Attachment is obsolete: true
Attachment #8535739 - Attachment description: self-image can be cropped, meaning the user doesn't see the entire sent images, r=NiKo` → [landed fx-team] self-image can be cropped, meaning the user doesn't see the entire sent images, r=NiKo`
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.
https://hg.mozilla.org/mozilla-central/rev/4cdd32ea6f7d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Attachment #8535739 - Flags: approval-mozilla-beta?
Attachment #8535739 - Flags: approval-mozilla-aurora?
Attachment #8535739 - Flags: approval-mozilla-beta?
Attachment #8535739 - Flags: approval-mozilla-beta+
Attachment #8535739 - Flags: approval-mozilla-aurora?
Attachment #8535739 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
Blocks: 1112727
Iteration: --- → 37.1
You need to log in before you can comment on or make changes to this bug.