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

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

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.

Comment 1

[Dan Minor [: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.

Comment 2

[Dan Minor [:dminor]]

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.

Comment 3

[Dan Minor [:dminor]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

(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.

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Remove ifdefs for OS X 10.7 from webrtc;

Review commit: https://reviewboard.mozilla.org/r/250084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250084/

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

This defers the call to register the refresh and move handlers to the
CaptureFrame() call so that they will be registered on the ScreenCapture
thread.

This also calls CFRunLoopInMode to process any pending sources in the
run loop corresponding to the ScreenCapture thread so that the
refresh and move notifications are received.

Review commit: https://reviewboard.mozilla.org/r/250086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250086/

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

Review commit: https://reviewboard.mozilla.org/r/250088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250088/

Comment 8

[Dan Minor [:dminor]]

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 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8984277 [details]
Bug 1409018 - Remove ifdefs for OS X 10.7 from webrtc;

https://reviewboard.mozilla.org/r/250084/#review256502

Comment 10

[Andreas Pehrson [:pehrsons]]

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.

Comment 11

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8984279 [details]
Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

https://reviewboard.mozilla.org/r/250088/#review256512

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this recent regression for 62 to make sure the fix lands.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Dan, can you file that followup bug mentioned in comment 10? Thanks.

Comment 14

[Dan Minor [: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.

Comment 15

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Remove ifdefs for OS X 10.7 from webrtc;

Review commit: https://reviewboard.mozilla.org/r/250846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250846/

Comment 16

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

This defers the call to register the refresh and move handlers to the
CaptureFrame() call so that they will be registered on the ScreenCapture
thread.

This also calls CFRunLoopInMode to process any pending sources in the
run loop corresponding to the ScreenCapture thread so that the
refresh and move notifications are received.

Review commit: https://reviewboard.mozilla.org/r/250848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250848/

Comment 17

[Dan Minor [:dminor]]

Attachment: Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

Review commit: https://reviewboard.mozilla.org/r/250850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250850/

Comment 18

[Dan Minor [:dminor]]

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 :)

Comment 19

[Andreas Pehrson [:pehrsons]]

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.

Comment 20

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8985263 [details]
Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

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

LGTM thanks!

Comment 21

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8985263 [details]
Bug 1409018 - Defer RegisterRefreshAndMoveHandlers call;

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

Comment 22

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8985264 [details]
Bug 1409018 - Re-enable test_getUserMedia_basicScreenshare;

https://reviewboard.mozilla.org/r/250850/#review257640

Comment 23

[Dan Minor [:dminor]]

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.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, that's fine, but this may not make it into 62.0b1.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a7bd9bd361a3
https://hg.mozilla.org/mozilla-central/rev/1a8d462805f0
https://hg.mozilla.org/mozilla-central/rev/0708df4d3b7a

Comment 27

[Jonathan Watt [:jwatt]]

Thanks, Dan!

Comment 28

[ovidiu boca[:Ovidiu]]

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

Comment 29

[Dan Minor [: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.

Comment 30

[ovidiu boca[:Ovidiu]]

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.