Closed
Bug 1395289
Opened 7 years ago
Closed 7 years ago
Screensharing is broken (only a quarter of the screen updates + garbage) OSX (regression)
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: jib, Assigned: dminor)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.24 KB,
patch
|
jesup
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1379743 +++ STRs on OSX (Windows 10 works fine): 1. Open https://jsfiddle.net/jib1/3xytpne5/ and share screen 2. Move Firefox window around the screen. Expected result: Firefox window is seen moving across entire screen capture. Actual result: Movement is only seen in the upper-left quarter of the screen capture, with some of the screen garbled near the middle. Workaround: - No workaround. Regression range: 7:06.01 INFO: No more inbound revisions, bisection finished. 7:06.01 INFO: Last good revision: c7428449127566105bdd94b2823b01e0e5e007d5 7:06.01 INFO: First bad revision: 5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e 7:06.01 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7428449127566105bdd94b2823b01e0e5e007d5&tochange=5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e Another regression from bug 1341285.
Reporter | ||
Updated•7 years ago
|
Rank: 14
Reporter | ||
Comment 1•7 years ago
|
||
Note the symptoms can also be seen in the preview in the permission doorhanger before clicking "Allow", which is what I used to determine the regression range (to sidestep Bug 1379743).
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
I'm not sure if this code was removed because it looked unnecessary or if it was causing problems on some displays. If the latter, please let me know. I'm guessing I could duplicate any problems on non-retina displays by connecting my cheap monitor to my macbook, but I'd have to buy a cable to be able to do so.
Attachment #8903247 -
Flags: review?(rjesup)
Reporter | ||
Comment 3•7 years ago
|
||
See also https://bugs.chromium.org/p/webrtc/issues/detail?id=6900
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Dan Minor [:dminor] from comment #2) > Created attachment 8903247 [details] [diff] [review] > Convert from "points" to pixels > > I'm not sure if this code was removed because it looked unnecessary or if it > was causing problems on some displays. If the latter, please let me know. > I'm guessing I could duplicate any problems on non-retina displays by > connecting my cheap monitor to my macbook, but I'd have to buy a cable to be > able to do so. My mistake, the code was removed upstream, not locally.
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8903247 [details] [diff] [review] Convert from "points" to pixels I think this needs to be #ifdeffed based upon the version of OS X we're building with.
Attachment #8903247 -
Flags: review?(rjesup)
Assignee | ||
Comment 6•7 years ago
|
||
The code path to ScreenRefresh depends upon the version of OS X we're running. On older versions, the conversion from "points" to pixels is still necessary.
Attachment #8903247 -
Attachment is obsolete: true
Attachment #8903267 -
Flags: review?(rjesup)
Reporter | ||
Comment 7•7 years ago
|
||
Note, the exact same symptoms have been reported on Windows in bug 139256, so I'm expanding this bug to cover Windows, and closing the other as a dup. See also comment 3.
Summary: Screensharing is broken in OSX (only a quarter of the screen updates + garbage) (regression) → Screensharing is broken (only a quarter of the screen updates + garbage) OSX, Windows (regression)
Comment 9•7 years ago
|
||
Comment on attachment 8903267 [details] [diff] [review] Convert from "points" to pixels on older versions of OS X Review of attachment 8903267 [details] [diff] [review]: ----------------------------------------------------------------- r+, note this only covers Mac. Very Soon Now we should have the builders updated past 10.7 (to 10.12?); it should have landed months ago...
Attachment #8903267 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7) > Note, the exact same symptoms have been reported on Windows in bug 139256, > so I'm expanding this bug to cover Windows, and closing the other as a dup. > See also comment 3. I'm unable to reproduce this issue on Windows 8.1 in a VM or on a Windows 10 laptop. :jib, were you able to reproduce this on Windows?
Flags: needinfo?(jib)
Assignee | ||
Comment 11•7 years ago
|
||
On Windows there are separate GDI and DirectX code paths for screen sharing, so it's possible I was just "lucky" on the two systems I tried.
Reporter | ||
Comment 12•7 years ago
|
||
I've confirmed this patch fixes it on my OSX Sierra 10.12.3. The commit message confused me a bit, since Sierra is not "older", I guess it means on our min-version-compatible compile target. I too was unable to reproduce on Windows 10, the symptom screenshot in bug 1392565 just seemed undeniably similar. We can separate the two bugs again if that lets us land a mac fix sooner.
Flags: needinfo?(jib)
Assignee | ||
Comment 13•7 years ago
|
||
Thank you. I've reopened the other bug to investigate the problem on Windows and I'll land patch here soon. I've also rewritten the commit message to be (hopefully) clearer.
Comment 14•7 years ago
|
||
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/80ead4d0839f Convert from points to pixels when invalidating rect for screensharing when building on OS X older than 10.8; r=jesup
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/80ead4d0839f
Comment 16•7 years ago
|
||
Please request Beta approval on this when you get a chance. I've verified that it grafts cleanly.
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(dminor)
Target Milestone: mozilla56 → mozilla57
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8903267 [details] [diff] [review] Convert from "points" to pixels on older versions of OS X Approval Request Comment [Feature/Bug causing the regression]: Bug 1341285 - upstream webrtc.org builds against a newer version of OS X than we do, and we missed back porting some of the changes required to support the version we build against. [User impact if declined]: Broken screen sharing on OS X [Is this code covered by automated tests?]: Bug 1380346 tracks improving automated tests that will catch problems like this in the future. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Not strictly necessary, we've tested this on a few systems. Steps to reproduce are in Comment 0 if someone wants to verify this fix. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: This just restores code that was removed during the branch 57 update. [String changes made/needed]: None.
Flags: needinfo?(dminor)
Attachment #8903267 -
Flags: approval-mozilla-beta?
Comment 18•7 years ago
|
||
Comment on attachment 8903267 [details] [diff] [review] Convert from "points" to pixels on older versions of OS X Fix for screensharing on some versions of MacOS, seems worth a try on beta 10.
Attachment #8903267 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/edd07c36dc0d
Comment 20•7 years ago
|
||
Since the fix for this bug is specific to macOS, and 1392565 tracks the Windows bug, can the summary of this bug be updated to remove the reference to Windows?
Assignee | ||
Updated•7 years ago
|
Summary: Screensharing is broken (only a quarter of the screen updates + garbage) OSX, Windows (regression) → Screensharing is broken (only a quarter of the screen updates + garbage) OSX (regression)
Assignee | ||
Updated•6 years ago
|
Blocks: Screensharing
You need to log in
before you can comment on or make changes to this bug.
Description
•