Closed Bug 1409018 Opened 2 years ago Closed Last year

Screen capture does not update on OS X since we started targetting OS X 10.9

Categories

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

62 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 + fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Upstream webrtc.org dropped support for OS X SDK 10.7 before us, requiring us to resurrect old code and ifdef the new code. Now that we've updated our required SDK to 10.11, we should just use the new code.
Rank: 35
Priority: -- → P2
Rank: 35 → 15
See Also: → 1405083
Assignee: nobody → dminor
The OS X 10.8 and above code is not getting refresh and move event callbacks, which means we only get the initial screenshare but no updates (see comments in Bug 1405083). Basically, the problem is that it is using a CFRunLoop to process the events, and we never let that CFRunLoop run. A secondary problem is that we initially register the handlers on the VideoCapture thread rather than the ScreenCapture thread.

We can defer registering the handlers until we're on the ScreenCapture thread by doing it is as part of the CaptureFrame() call. I've experimented with using the CFRunLoopRunInMode() call to process a single pending event as part of CaptureFrame(). This seems to work fine when running screen sharing from the webrtc landing page, but the associated mochitest still fails.

I think what we'll end having to do is to create a PlatformUIThread for OS X (similar to what we had to do for Windows) that runs the RunLoop and calls CaptureFrame on a timer as part of that run loop, which might take a little time to get working.

If we're concerned about leaving screen capture broken while I get this fixed, there are two work arounds: The first would be to let the handlers run on the main loop. The other would be to just capture the entire screen on each call to CaptureFrame rather than trying to only capture the changed part of the screens. That would hurt performance, but might be better than nothing while I work on getting this fixed properly.
Summary: Remove ifdefs for OS X SDK 10.7 from webrtc → Screen capture does not update on OS X since we started targetting OS X 10.9
This regressed with Bug 1270217, but the cause is new code added by Bug 1341285 (the webrtc.org branch 57 update) that was not active until now.
The old version of the code we were using (CGRegisterScreenRefreshCallback, etc.) to support OS X 10.7 is deprecated, so just going back to using that is not great for future maintainability.
(In reply to Dan Minor [:dminor] from comment #1)
> If we're concerned about leaving screen capture broken while I get this
> fixed, there are two work arounds: The first would be to let the handlers
> run on the main loop. The other would be to just capture the entire screen
> on each call to CaptureFrame rather than trying to only capture the changed
> part of the screens. That would hurt performance, but might be better than
> nothing while I work on getting this fixed properly.

Another temporary option might be to hack in our own ifdef'ed definition of std::move to Move.h, and then back out bug 1270217 for now.
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15725c992dccb063565e0683c334f19f41e6048f

The mochitests were still pending when I put this up for review; please disregard this review request if they end up failing.
Comment on attachment 8984277 [details]
Bug 1409018 - Remove ifdefs for OS X 10.7 from webrtc;

https://reviewboard.mozilla.org/r/250084/#review256502
Attachment #8984277 - Flags: review?(apehrson) → review+
Comment on attachment 8984278 [details]
Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

https://reviewboard.mozilla.org/r/250086/#review256504

::: media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm:366
(Diff revision 1)
>  
>    // A self-owned object that will destroy itself after ScreenCapturerMac and
>    // all display streams have been destroyed..
>    DisplayStreamManager* display_stream_manager_;
>  
> +  bool update_screen_configuration_ = false;

Needs a comment on what it means and which thread(s) can/will access it.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm:408
(Diff revision 1)
> +// MOZILLA: Calling RegisterRefreshAndMoveHandlers here causes us
> +// to register on the VideoCapture thread rather than the ScreenCapture
> +// thread which will result in us never receiving any notifications.

Please file a followup for fixing this properly and reverting this hack; and mention the bug number here.
Attachment #8984278 - Flags: review?(apehrson) → review+
Comment on attachment 8984279 [details]
Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

https://reviewboard.mozilla.org/r/250088/#review256512
Attachment #8984279 - Flags: review?(apehrson) → review+
Tracking this recent regression for 62 to make sure the fix lands.
Dan, can you file that followup bug mentioned in comment 10? Thanks.
Flags: needinfo?(dminor)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> Dan, can you file that followup bug mentioned in comment 10? Thanks.

Filed Bug 1468509.
Flags: needinfo?(dminor)
Attachment #8984277 - Attachment is obsolete: true
Attachment #8984278 - Attachment is obsolete: true
Attachment #8984279 - Attachment is obsolete: true
I addressed the review feedback from a different laptop than the one I used to write the patches originally which I guess tricked MozReview into obsoleting the previous patches and dropping the r+'s. I'm going to cancel the review requests, but feel free to re-review if you want :)
Attachment #8985262 - Flags: review?(apehrson)
Attachment #8985263 - Flags: review?(apehrson)
Attachment #8985264 - Flags: review?(apehrson)
Comment on attachment 8985262 [details]
Bug 1409018 - Remove ifdefs for OS X 10.7 from webrtc;

https://reviewboard.mozilla.org/r/250846/#review257634

Assuming this hasn't changed since last time I reviewed.
Attachment #8985262 - Flags: review+
Comment on attachment 8985263 [details]
Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

https://reviewboard.mozilla.org/r/250848/#review257636

LGTM thanks!
Attachment #8985263 - Flags: review?(apehrson)
Comment on attachment 8985263 [details]
Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

https://reviewboard.mozilla.org/r/250848/#review257638
Attachment #8985263 - Flags: review?(apehrson) → review+
Comment on attachment 8985264 [details]
Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

https://reviewboard.mozilla.org/r/250850/#review257640
Attachment #8985264 - Flags: review+
Liz, are you ok with me landing this during the soft code freeze? I think it is low risk and fixes a pretty bad regression. I would have landed last week, but I didn't have my OS X laptop with me and wanted to do some final testing after addressing the review comments.
Flags: needinfo?(lhenry)
Yes, that's fine, but this may not make it into 62.0b1.
Flags: needinfo?(lhenry)
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7bd9bd361a3
Remove ifdefs for OS X 10.7 from webrtc; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/1a8d462805f0
Defer RegisterRefreshAndMoveHandlers call; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/0708df4d3b7a
Re-enable test_getUserMedia_basicScreenshare; r=pehrsons
Thanks, Dan!
I filed bug 1487419 and after a regression range, I think the fix from this bug created the regression.
Also, the screen capture is not updated if you use https://mozilla.github.io/webrtc-landing/gum_test.html -> select Screen-> select 1 screen, you will see that both screens are captured but the action from one of them is not updated. 
Dan, please tell me if I need to file another bug for this ore reopen this one? Thanks
Flags: needinfo?(dminor)
Thanks for your report! I don't we need to reopen this bug as screen sharing seems to be working fine when no external screen is attached. I tested it just now.
Flags: needinfo?(dminor)
I have an iMac (21.5-inch, Late 2013) with an extra monitor attached Samsung S22C300. 
I did a screen recording https://streamable.com/fffm6  and you can see that even if I select screen 1 or screen 2 the action from the iMac screen is not recorded only the action from Samsung screen. 
If I plug out the Samsung screen the iMac screen recording is working.
Depends on: 1487419
You need to log in before you can comment on or make changes to this bug.