1043350 - screensharing causes a switch to the Windows 7 Basic appearance and a blinking mouse cursor

Status: VERIFIED FIXED

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

Description

[Florian Quèze [:florian]]

Attachment: Screenshot

When I start sharing the screen with Firefox on Windows 7, the screen turns black for about a second, and then I have a notification saying:
 *The color scheme has been changed to Window 7 Basic*
 A running program isn't compatible with certain visual elements of Windows.
(see attached screenshot)

During the screenshare, the mouse cursor keeps blinking.

Once I stop the screenshare, the screen reverts back to normal.

Note: window sharing doesn't cause this issue.

Comment 1

[Maire Reavy [:mreavy]]

Gcp, Matt -- Can one of you take this bug?

Comment 2

[Maire Reavy [:mreavy]]

Hi Roman -- Thanks for looking at this bug.  If you think this bug is not one you want to fix, just "unassign" yourself and "needinfo" me in this bug to find a new owner.

Comment 3

[Roman Skalish]

Hi, about first part of bug "When I start sharing the screen with Firefox on Windows 7, the screen turns black for about a second". We have such behavior because we disable Aero mode on Win 7, see ScreenCapturerWin::Start() function:
  // Vote to disable Aero composited desktop effects while capturing. Windows
  // will restore Aero automatically if the process exits. This has no effect
  // under Windows 8 or higher.  See crbug.com/124018.
  if (composition_func_)
    (*composition_func_)(DWM_EC_DISABLECOMPOSITION);

From Google comments:
"There are no obvious signs of poor performance in the Task Manager; CPU usage is <2%.  I think the dialog is triggered when the desktop window manager sees unexpectedly high response times from the display driver, which might be the case if its requests are being delayed by the host pulling too much data back from the display too often.
One option to avoid this is to disable Aero for the duration of remote sessions."

Actually I've tested sharing the screen without disabling Aero mode and it works for me mach faster, also there wasn't delays in windows rendering (when I was trying to move some windows).

So I see two cases here leave it as is (because it is related only to Win 7) or remove this disabling code.

About second part of the bug "During the screenshare, the mouse cursor keeps blinking.", still looking on it, looks like we need to add double buffering to prevent blinking.

Comment 4

[Roman Skalish]

Small note: Tested it on Chrome 34.0.1847.116 and looks like Chrome doesn't disable Win Aero mode now, as it was temporary fix. See crbug.com/124018 "This is a temporary measure until we work out what causes Windows to prompt about poor video performance."
About blinking mouse cursor it is repro in Chrome 34, looking on it.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

As clarified on IRC: Windows 7 is very popular, disabling Aero is very annoying. Given that you say Chrome no longer does it and it seems to perform better that way, this seems like a no-brainer to me. Remove the disabling.

Comment 6

[Ethan Hugg [:ehugg]]

I agree with Gian-Carlo for what it's worth. We can't switch to Win basic to do this.

Comment 7

[Matthew Miller [:m_and_m]]

I agree with Gian-Carlo; this might have been the most necessary in early Vista releases.

Comment 8

[Roman Skalish]

Attachment: Bug1043350_fixed_screen_turns_black.patch

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8463404 [details] [diff] [review]
Bug1043350_fixed_screen_turns_black.patch

Review of attachment 8463404 [details] [diff] [review]:
-----------------------------------------------------------------

One question: on what is the statement that Chrome no longer does it based?
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc?r=6499

As far as I can see, they're still doing this: 

The reason why they don't get Aero is that they don't use that code at all, but use this API:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms692162%28v=vs.85%29.aspx
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc?r=6499

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Oops, that got mangled. Should've said "...they're still doing this. The reason they don't fall back to Aero..."

Comment 11

[Roman Skalish]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> Comment on attachment 8463404 [details] [diff] [review]
> Bug1043350_fixed_screen_turns_black.patch
> 
> Review of attachment 8463404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question: on what is the statement that Chrome no longer does it based?
> https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/
> desktop_capture/win/screen_capturer_win_gdi.cc?r=6499
> 
> As far as I can see, they're still doing this: 
> 
> The reason why they don't get Aero is that they don't use that code at all,
> but use this API:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms692162%28v=vs.
> 85%29.aspx
> https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/
> desktop_capture/win/screen_capturer_win_magnifier.cc?r=6499

So as I understood Chrome uses Magnification API, right? Because when I launch screensharing on Chrome break point related to disable Aero wasn't hit.
And according your comment at IRC we should marge our WebRTC code with new WebRTC code which uses Magnification API?

Comment 12

[Gian-Carlo Pascutto [:gcp]]

I'm trying to verify this right now with Chromium, but superficially it does look like we want to update to use that new API as well.

Comment 13

[Roman Skalish]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)
> I'm trying to verify this right now with Chromium, but superficially it does
> look like we want to update to use that new API as well.

Okay, so for now this bug will stay on hold?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Updating to Magnifier might not be possible for Firefox 33, and I don't think we can just ignore this bug.

Comment 15

[Roman Skalish]

So, how then we should proceed with this bug? Can it be with removed functionality related to disable Aero until updating to Magnifier?

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Yeah, that sounds like a reasonable approach to me. We might find out more when the feature gets more testing.

Comment 17

[Roman Skalish]

And about blinking mouse cursor I am out of ideas. I thought that this issue is related to double buffering, but looks like it doesn't. Not sure that updating to Magnifier will fix this issue...

Comment 18

[Roman Skalish]

So for version 33, I will attach patch with removed functionally related to disable Aero (because current attached patch is generated by git), yes?

Comment 19

[Ethan Hugg [:ehugg]]

Comment on attachment 8463404 [details] [diff] [review]
Bug1043350_fixed_screen_turns_black.patch

Review of attachment 8463404 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 20

[Matthew Miller [:m_and_m]]

Comment on attachment 8463404 [details] [diff] [review]
Bug1043350_fixed_screen_turns_black.patch

Review of attachment 8463404 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, it looks fine to me.

Comment 21

[Roman Skalish]

Attachment: Bug1043350.patch

re-create patch by hg

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Your patch needs a description/commit message, and 
# User Roman <[e-mail]>
you probably want your full name in there? Check your hgrc and qrefresh the patch with -U option added.

Comment 23

[Roman Skalish]

Attachment: Bug1043350.patch

Added commit message and full name

Comment 24

[Gian-Carlo Pascutto [:gcp]]

I successfully debugged Chrome now, and I can confirm they just disable the code that disables Aero, and do not use the Magnifier API on Windows 7:

http://webrtc.googlecode.com/svn-history/r6803/trunk/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc
i.e. the call:
  if (options.disable_effects()) {

returns false, because:

https://chromium.googlesource.com/chromium/chromium/+/trunk/content/browser/media/capture/desktop_capture_device.cc

438. // Leave desktop effects enabled during WebRTC captures.
439. options.set_disable_effects(false);

So this patch should be all we need for now.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8466991 [details] [diff] [review]
Bug1043350.patch

Review of attachment 8466991 [details] [diff] [review]:
-----------------------------------------------------------------

Checking the Chrome code, we should just set the flag that prevents this code from activating. Yanking it out would work as well, but it needlessly increases our diff versus the upstream WebRTC code.

So can we just add a options.set_disable_effects(false); in the right place?

Jesup, what approach do you prefer?

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8466991 [details] [diff] [review]
Bug1043350.patch

Review of attachment 8466991 [details] [diff] [review]:
-----------------------------------------------------------------

>Checking the Chrome code, we should just set the flag that prevents this code from activating. Yanking it out would work as well, but it needlessly increases our diff versus the upstream WebRTC code.
>So can we just add a options.set_disable_effects(false); in the right place?

I'd prefer to keep differences limited; if it's possible to turn this off with simple patch, that's preferable.

If for some odd reason we couldn't do that, we can do this.

Comment 27

[Roman Skalish]

Attachment: Bug1043350disabledAero.patch

Comment 28

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=43057a1cca8c

Comment 29

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8467615 [details] [diff] [review]
Bug1043350disabledAero.patch

Review of attachment 8467615 [details] [diff] [review]:
-----------------------------------------------------------------

The patch description is the wrong way around.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +374,5 @@
>      MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForScreen(webrtc::DesktopCaptureOptions::CreateDefault(), webrtc::kFullDesktopScreenId);
>      desktop_capturer_cursor_composer_.reset(new DesktopAndCursorComposer(pAppCapturer, pMouseCursorMonitor));
>    } else if (type == Screen) {
> +#if defined(WEBRTC_WIN)
> +    ScreenCapturer *pScreenCapturer = ScreenCapturer::CreateWithDisableAero(false);

I don't like this for 3 reasons:

1) This ends up creating an extra copy of DesktopCaptureOptions. 
2) It requires a (needless) #ifdef WEBRTC_WIN
3) It uses code that upstream (no longer) uses, as you can see in the Chrome code I linked, and which is generally a bad idea. In fact, screen_capturer.h which contains the definition points out that the functions you are using are deprecated and about to be removed, see crbug.com/172183.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Attachment: Do not disable Aero mode when screen capturing. r=

Comment 31

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ce6be46c38

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e4ce6be46c38

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8467688 [details] [diff] [review]
Do not disable Aero mode when screen capturing. r=

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: visual jarring on Win7 when sharing

[Describe test coverage new/current, TBPL]: on m-c for 3 weeks, heavily used manually

[Risks and why]: minimal risk - it just tells sharing to stop disabling aero

[String/UUID change made/needed]: none

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9b04954e337

Comment 35

[Bogdan Maris, Desktop QA]

Reproduced the issue on old Nightly (2014-07-24), the issue regarding the W7 Basic theme change is solved in latest Aurora and Firefox 33 beta 2 but the mouse cursor still blinks. The mouse issue will be solved in bug 1053264.