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.
(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.
Priority: -- → P2
Bug 1021547 *might* be related... it's concerning devicelPixelRatio, which I don't know is the real issue here.
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.
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
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.
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?
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.
That looks like a fix we can uplift to 38, right?
status-firefox38: --- → affected
tracking-firefox38: --- → +
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
Attachment #8577292 - Flags: approval-mozilla-aurora?
Attachment #8577292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
status-firefox38: affected → fixed
You need to log in before you can comment on or make changes to this bug.