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

RESOLVED FIXED in Firefox 38

Status

()

Core
WebRTC: Audio/Video
P2
normal
Rank:
21
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: jib)

Tracking

(Blocks: 1 bug)

33 Branch
mozilla39
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed)

Details

(Whiteboard: [sharing])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8567899 [details]
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
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.
(Reporter)

Comment 2

3 years ago
(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.

Updated

3 years ago
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [sharing]
Bug 1021547 *might* be related... it's concerning devicelPixelRatio, which I don't know is the real issue here.
(Reporter)

Updated

3 years ago
Blocks: 923225
Flags: firefox-backlog+

Comment 4

3 years ago
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.
Flags: needinfo?(mreavy)

Updated

3 years ago
Flags: firefox-backlog+
STR needs patch from Bug 1140313 for now.
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.
Assignee: nobody → jib
Rank: 24 → 21
Flags: needinfo?(mreavy)
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.
Flags: needinfo?(mreavy)
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)
That's the spot I changed, so I know that works already. Are you saying we want to land 4096/4096/30?
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).
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?
Flags: needinfo?(mreavy)
Created attachment 8577292 [details] [diff] [review]
bump up tab sharing dimension defaults to match screensharing

Default to 4096x4096x30 and alter code to use min(innerDimensions, mBufferMax) instead of mBufferMax.
Attachment #8577292 - Flags: review?(rjesup)
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
Attachment #8577292 - Flags: review?(rjesup) → review+
(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.
Keywords: checkin-needed
That looks like a fix we can uplift to 38, right?
status-firefox38: --- → affected
tracking-firefox38: --- → +
Flags: needinfo?(jib)
Target Milestone: --- → mozilla39
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
Flags: needinfo?(jib)
Attachment #8577292 - Flags: approval-mozilla-aurora?
Attachment #8577292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/64f3fa7a5a8d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.