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)

56 Branch
All
macOS
defect

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)

+++ 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.
Rank: 14
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: nobody → dminor
Status: NEW → ASSIGNED
See Also: → 1380346
Attached patch Convert from "points" to pixels (obsolete) — Splinter Review
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)
(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.
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)
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)
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 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+
(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)
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.
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)
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.
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
https://hg.mozilla.org/mozilla-central/rev/80ead4d0839f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Please request Beta approval on this when you get a chance. I've verified that it grafts cleanly.
Flags: needinfo?(dminor)
Target Milestone: mozilla56 → mozilla57
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 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+
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?
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)
You need to log in before you can comment on or make changes to this bug.