1135655 - Default tab (aka browser) sharing resolution is poor compared to window sharing

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Mark Banner (:standard8)]

Attachment: Screen shot of bad resolutions

I've been testing the patches on bug 1133493 (note: currently a special build), and I noticed that tab sharing appears to have a lower default resolution than window sharing.

We're not setting up anything special for resolution requirements as far as I know.

STR:

1) With the special build, join a room, share the link to another FF instance on the same network/machine (I'm on Mac, so I can share to the same).

2) Monitor the output of sharing tabs vs windows against some text.

Actual Results

=> Tab sharing is more blurry and unreadable than window sharing.

Expected Results

=> Similar resolution and readability for both.

The attached screenshots show:

    Original Tab  |  Tab share result
  ----------------|--------------------
  Original Window | Window share result

Comment 1

[Mike de Boer [:mikedeboer]]

My guess would be that tab sharing hits a path where it skips the HiDPI aware capturing, since we're both using retina screens on Mac see this happen.

Comment 2

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> My guess would be that tab sharing hits a path where it skips the HiDPI
> aware capturing, since we're both using retina screens on Mac see this
> happen.

Note, I was on a non-retina screen when I was testing this (my second monitor).

I've just tested on a retina screen, and I get the same result.

Comment 3

[Mike de Boer [:mikedeboer]]

Bug 1021547 *might* be related... it's concerning devicelPixelRatio, which I don't know is the real issue here.

Comment 4

[:shell escalante]

Hi Maire,  Just checking where this falls in folks backlogs for taking work.  We're uplifting tab sharing for Fx38. it's definitely something we'd hope fixed for Fx38 because of visibility - but totally get Fx39 if there's other blockers or it's higher risk to uplift.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STR needs patch from Bug 1140313 for now.

Comment 6

[Maire Reavy [:mreavy]]

Assigning to jib for initial investigation (so we can determine the root cause of the problem).

Shell -- it's safer to assume Fx39.  Depending on what the cause is, it may be a quick fix that we can uplift to Fx38 -- or it may be more complicated.  Jib will take a deeper look to determine why this is happening, and then we'll assess the LOE to fix it.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

So window sharing looks nice because it defaults to 0x0 which down below translates to 2452x1370 (I'm guessing the size of the window in my case:

692584448[12e4367c0]: chose cap 0x0 @0fps codec 7 raw 0
692584448[12e4367c0]: Video device 4097 allocated
692584448[12e4367c0]: Start
692584448[12e4367c0]: started all sources
1530925056[127d3d300]: Video FrameSizeChange: 2452x1370

Whereas tab sharing dutifully respects the media Prefs default of 640x480 and gets 640x480:

692584448[12e4367c0]: ChooseCapability: prefs: 0x0 @30-10fps
692584448[12e4367c0]: chose cap 640x480 @30fps codec 7 raw 0
692584448[12e4367c0]: Video device 4097 allocated
692584448[12e4367c0]: Audio config: aec: 1, agc: -1, noise: 1
692584448[12e4367c0]: Start audio for stream 1334ba4c0
692584448[12e4367c0]: Audio config: aec: 0, agc: -1, noise: 0
692584448[12e4367c0]: Start
692584448[12e4367c0]: started all sources
1997386512[100437a00]: MediaCaptureWindowState: window 61 capturing video audio    
1997386512[100437a00]: Returning success for getUserMedia()
1257246720[12ccc7640]: Video FrameSizeChange: 640x480

That's why the latter looks fuzzy. If I replace 640x480 down below with 2452x1370 then tab sharing looks great as well.

If the low-level code knows the size of the window, then it should use that instead of 640x480, otherwise we can pass in the dimensions using constraints.

Comment 8

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

I reviewed the code here.

Try replacing the aPrefs.GetWidth()/GetHeight()/maxFPS with 4096/4096/30
It really shouldn't be respecting those defaults, though if the user asks for a specific size, it probably should respect that.  (And note that the other captures probably *don't* respect such a request)

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

That's the spot I changed, so I know that works already. Are you saying we want to land 4096/4096/30?

Comment 10

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

Well, I'd prefer no particular limit (though in practice there are limits.  4Kx4K is overkill currently, so that's ok (I presume this is merely an upper limit).  30 is a reasonable upper limit for fps (perhaps 60 in some special cases, but I don't see that as very useful in practice).

Comment 11

[Maire Reavy [:mreavy]]

Jib -- Thanks for digging in on this. I'm ok with landing 4096/4096/30.  It would resolve the pain we have with poor resolution for tab sharing, and the limit is large enough that I don't foresee it being a problem in practice for users.  Further, this sort of patch would be low risk to uplift since we'd love to fix this for Fx38 -- the first release of Hello that will support tab sharing.

So, can you put up a patch for review?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: bump up tab sharing dimension defaults to match screensharing

Default to 4096x4096x30 and alter code to use min(innerDimensions, mBufferMax) instead of mBufferMax.

Comment 13

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

Comment on attachment 8577292 [details] [diff] [review]
bump up tab sharing dimension defaults to match screensharing

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

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +214,2 @@
>      size = IntSize(width, (width * ((float) innerHeight/innerWidth)));
>    }

I would love to get rid of the %4 stuff (and I think that bug may now be fixed); if so, we could simplify this code a fair bit

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #13)
> I would love to get rid of the %4 stuff (and I think that bug may now be
> fixed); if so, we could simplify this code a fair bit

That's Bug 1125393. It doesn't look like it's been fixed.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/64f3fa7a5a8d

Comment 16

[:Gavin Sharp [email: [e-mail]]]

That looks like a fix we can uplift to 38, right?

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8577292 [details] [diff] [review]
bump up tab sharing dimension defaults to match screensharing

Approval Request Comment
[Feature/regressing bug #]: Introduced with tab-sharing.
[User impact if declined]:
Fuzzy video quality when tab sharing, which looks bad on text. Much worse than with window sharing.
[Describe test coverage new/current, TreeHerder]:
Fixed symptom verified locally. Landed on m-i
[Risks and why]:
Very low risk. The scope is contained to the new tab-sharing feature, and the patch merely ups the default dimensions of mBufMax from the prefs-alterable cameras default of 640x480 to a richer hardcoded 4096x4096, and modifies the code to treat it as an actual max rather than a final buffer value (i.e. use innerWindow dimensions when less). These are default values that can be changed with constraints.

[String/UUID change made/needed]: none

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/64f3fa7a5a8d

Comment 19

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0ce72fae360