Closed Bug 1135655 Opened 5 years ago Closed 5 years ago

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

Categories

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

33 Branch
All
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: standard8, Assigned: jib)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sharing])

Attachments

(2 files)

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.
(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.
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.
Rank: 24
Flags: firefox-backlog+
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)
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)
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.
That looks like a fix we can uplift to 38, right?
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
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.