Closed
Bug 1043350
Opened 10 years ago
Closed 10 years ago
screensharing causes a switch to the Windows 7 Basic appearance and a blinking mouse cursor
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla34
People
(Reporter: florian, Assigned: gcp)
References
Details
Attachments
(3 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
gcp
:
review-
jesup
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jesup
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Gcp, Matt -- Can one of you take this bug?
Flags: needinfo?(linuxwolf)
Flags: needinfo?(gpascutto)
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Comment 2•10 years ago
|
||
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.
Assignee: nobody → rskalish
Flags: needinfo?(rskalish)
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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.
Flags: needinfo?(ethanhugg)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Flags: needinfo?(gpascutto)
Comment 6•10 years ago
|
||
I agree with Gian-Carlo for what it's worth. We can't switch to Win basic to do this.
Flags: needinfo?(ethanhugg)
Comment 7•10 years ago
|
||
I agree with Gian-Carlo; this might have been the most necessary in early Vista releases.
Flags: needinfo?(linuxwolf)
Comment 8•10 years ago
|
||
Attachment #8463404 -
Flags: review?(linuxwolf)
Attachment #8463404 -
Flags: review?(gpascutto)
Attachment #8463404 -
Flags: review?(ethanhugg)
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
Oops, that got mangled. Should've said "...they're still doing this. The reason they don't fall back to Aero..."
Updated•10 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla34
Comment 11•10 years ago
|
||
(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?
Assignee | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
(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?
Assignee | ||
Comment 14•10 years ago
|
||
Updating to Magnifier might not be possible for Firefox 33, and I don't think we can just ignore this bug.
Comment 15•10 years ago
|
||
So, how then we should proceed with this bug? Can it be with removed functionality related to disable Aero until updating to Magnifier?
Assignee | ||
Comment 16•10 years ago
|
||
Yeah, that sounds like a reasonable approach to me. We might find out more when the feature gets more testing.
Comment 17•10 years ago
|
||
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•10 years ago
|
||
So for version 33, I will attach patch with removed functionally related to disable Aero (because current attached patch is generated by git), yes?
Updated•10 years ago
|
Flags: needinfo?(linuxwolf)
Flags: needinfo?(gpascutto)
Flags: needinfo?(ethanhugg)
Assignee | ||
Updated•10 years ago
|
Attachment #8463404 -
Flags: review?(gpascutto) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8463404 [details] [diff] [review] Bug1043350_fixed_screen_turns_black.patch Review of attachment 8463404 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8463404 -
Flags: review?(ethanhugg) → review+
Updated•10 years ago
|
Flags: needinfo?(ethanhugg)
Comment 20•10 years ago
|
||
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.
Attachment #8463404 -
Flags: review?(linuxwolf) → review+
Updated•10 years ago
|
Flags: needinfo?(linuxwolf)
Comment 21•10 years ago
|
||
re-create patch by hg
Attachment #8463404 -
Attachment is obsolete: true
Attachment #8466918 -
Flags: review+
Flags: needinfo?(rskalish)
Assignee | ||
Comment 22•10 years ago
|
||
Your patch needs a description/commit message, and # User Roman <rskalish@lohika.com> you probably want your full name in there? Check your hgrc and qrefresh the patch with -U option added.
Flags: needinfo?(gpascutto)
Comment 23•10 years ago
|
||
Added commit message and full name
Attachment #8466918 -
Attachment is obsolete: true
Attachment #8466991 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
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?
Attachment #8466991 -
Flags: review+ → feedback?(rjesup)
Comment 26•10 years ago
|
||
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.
Attachment #8466991 -
Flags: feedback?(rjesup) → feedback-
Comment 27•10 years ago
|
||
Attachment #8466991 -
Attachment is obsolete: true
Attachment #8467615 -
Flags: review?(rjesup)
Attachment #8467615 -
Flags: review?(gpascutto)
Updated•10 years ago
|
Attachment #8467615 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 28•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=43057a1cca8c
Assignee | ||
Comment 29•10 years ago
|
||
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.
Attachment #8467615 -
Flags: review?(gpascutto) → review-
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8467688 -
Flags: review?(rjesup)
Assignee | ||
Updated•10 years ago
|
Assignee: rskalish → gpascutto
Status: NEW → ASSIGNED
Updated•10 years ago
|
Attachment #8467688 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ce6be46c38
https://hg.mozilla.org/mozilla-central/rev/e4ce6be46c38
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: qe-verify+
Comment 33•10 years ago
|
||
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
Attachment #8467688 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
Updated•10 years ago
|
Attachment #8467688 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Comment 35•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•