Closed
Bug 1202087
Opened 7 years ago
Closed 7 years ago
WebRTC screen capture partially broken on windows 10
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: fippo, Assigned: achronop)
Details
Attachments
(2 files, 8 obsolete files)
38.25 KB,
image/png
|
Details | |
4.53 KB,
patch
|
Details | Diff | Splinter Review |
with insider build 10532 at least. See also https://code.google.com/p/chromium/issues/detail?id=526883 even though the behaviour is slight different. Chrome can render those windows if they have focus, Firefox never displays them. Some applications (see the chrome bug for a list) can't be captured. Others (like paint) still can. It's not yet clear what the difference is. To reproduce, go to talky.io, join a room and try to share the photos application. Screenshot attached. I'd note that the mouse cursor is captured.
Comment 1•7 years ago
|
||
See also bug 1196542, though that seems to be because a ~0x0 window is grabbed by the automated tests because it's first in the list
Comment 2•7 years ago
|
||
jimm: can you give this a try in win10? If you see it, any ideas?
Flags: needinfo?(jmathies)
Flags: needinfo?(bugspam.Callek)
Updated•7 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 21
Priority: -- → P2
![]() |
||
Comment 3•7 years ago
|
||
I can't get this talky site to work, there's no prompt to share in nightly.
Comment 4•7 years ago
|
||
fippO: Perhaps you blocked win10? jimm: did you start a call between two machines or tabs, and then try to share a window? Can you detail what sequence didn't get a prompt?
Flags: needinfo?(fippo)
Comment 5•7 years ago
|
||
Testing this (for me) outside of automation in a test suite is not necessarily easy. But if no-one else can test easily I'll give it a shot (clearing n-i for now)
Flags: needinfo?(bugspam.Callek)
![]() |
||
Comment 6•7 years ago
|
||
I'll try again on a different device..
![]() |
||
Comment 7•7 years ago
|
||
Tried on surface pro and managed to get things to load up. I was able to see myself, but the panel to the right was blank. (There was a little animated rocket ship glyph I could play with too, was I on the right page?) There was a "share screen" button, clicking that didn't seem to do anything. I also test the gum_test page - http://mozilla.github.io/webrtc-landing/gum_test.html all windows rendered black. Looks like we have a pretty bad capture issue on win10. I did this testing with latest nightly. Is there a chance I'm hitting that 0x0 window bug?
Flags: needinfo?(jmathies)
Comment 8•7 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #7) > Tried on surface pro and managed to get things to load up. I was able to see > myself, but the panel to the right was blank. (There was a little animated > rocket ship glyph I could play with too, was I on the right page?) There was > a "share screen" button, clicking that didn't seem to do anything. Aha. Misunderstanding: you have to be *in* a call to share a window/application/etc. You need two machines, or two tabs in the same browser (which should work, if a little confusing). I advise muting.... ;-) > I also test the gum_test page - > > http://mozilla.github.io/webrtc-landing/gum_test.html > > all windows rendered black. > > Looks like we have a pretty bad capture issue on win10. Yeah, that's bad, though I don't know anyone has tried it on win10 Surface. We had several people (callek, etc) that "normal" usage on win10 worked at least in gum tests. Try the gum test again with: NSPR_LOG_MODULES=mediamanager:4,getusermedia:4,webrtc_trace:65535 WEBRTC_TRACE_FILE=nspr Thanks! > I did this testing with latest nightly. Is there a chance I'm hitting that > 0x0 window bug? Virtually none. That's likely just in automation, or if you somehow select a 0x0 window.
Flags: needinfo?(jmathies)
Reporter | ||
Comment 9•7 years ago
|
||
jesup: might be related to the surface (i vaguely recall some chrome issues with screensharing), i just tested that plain windows 10 is not blocked -- the only thing we block for screensharing is Android the bad news: https://code.google.com/p/chromium/issues/detail?id=526883#c14 suggests this is something not easily fixed.
Flags: needinfo?(fippo)
![]() |
||
Comment 10•7 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #8) > (In reply to Jim Mathies [:jimm] from comment #7) > > Tried on surface pro and managed to get things to load up. I was able to see > > myself, but the panel to the right was blank. (There was a little animated > > rocket ship glyph I could play with too, was I on the right page?) There was > > a "share screen" button, clicking that didn't seem to do anything. > > Aha. Misunderstanding: you have to be *in* a call to share a > window/application/etc. You need two machines, or two tabs in the same > browser (which should work, if a little confusing). I advise muting.... ;-) > > > I also test the gum_test page - > > > > http://mozilla.github.io/webrtc-landing/gum_test.html > > > > all windows rendered black. > > > > Looks like we have a pretty bad capture issue on win10. > > Yeah, that's bad, though I don't know anyone has tried it on win10 Surface. > We had several people (callek, etc) that "normal" usage on win10 worked at > least in gum tests. > > Try the gum test again with: > NSPR_LOG_MODULES=mediamanager:4,getusermedia:4,webrtc_trace:65535 > WEBRTC_TRACE_FILE=nspr > Thanks! I didn't have much luck getting output, is this generated in the content process perhaps? That said, I did get a couple native win32 windows to work in a local build. Most everything I have running is universal. :/
Flags: needinfo?(jmathies)
![]() |
||
Comment 11•7 years ago
|
||
also I was able to see universal app windows when sharing the desktop.
Assignee | ||
Comment 12•7 years ago
|
||
I'll start working on that. I am planing to work inside DesktopDeviceInfoWin::InitializeApplicationList() and filter out all non-shareable applications, for win10, in the same way that chromium does it. Any comment is welcome.
Assignee | ||
Comment 13•7 years ago
|
||
This is the fix for non-shareable apps on win10. Since it is not tested yet I do not set it for review but feel free to comment if you think so.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → achronop
Comment 14•7 years ago
|
||
Comment on attachment 8686659 [details] [diff] [review] wip-bug1202087.patch Review of attachment 8686659 [details] [diff] [review]: ----------------------------------------------------------------- I presume this is similar to how Chrome filters non-sharable windows? ::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc @@ +92,5 @@ > continue; > } > > + // Filter out "Modern Apps" which associated window is non-shareable. > + const size_t classLength = 256; Is there a constant for GetClassName? Is 256 a "safe" number? @@ +96,5 @@ > + const size_t classLength = 256; > + WCHAR class_name[classLength]; > + const int class_name_length = GetClassName(hWnd, class_name, classLength); > + if (IsWindows8OrGreater() > + && GetClassName(hWnd, class_name, classLength) > 0 typical Mozilla style/style-guide: if (foo() && bar() && (xyyzy() || qwerty())) { ... ... }
Assignee | ||
Comment 15•7 years ago
|
||
> I presume this is similar to how Chrome filters non-sharable windows? Yes it follows the same logic with chromium. Chromium fix: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/desktop_capture/window_capturer_win.cc&l=56 > Is there a constant for GetClassName? Is 256 a "safe" number? I do not see in the doc any constant for that. 256 is the maximum length for WNDCLASS::lpszClassName which hold the class name.
Assignee | ||
Comment 16•7 years ago
|
||
Updated according to the comments.
Attachment #8686659 -
Attachment is obsolete: true
Assignee | ||
Comment 17•7 years ago
|
||
I tested this code and works but for application sharing. I guess we need to have something similar for window sharing?
Comment 18•7 years ago
|
||
(In reply to Alexandros Chronopoulos [:achronop] from comment #17) > I tested this code and works but for application sharing. I guess we need to > have something similar for window sharing? Yes, that's right.
Assignee | ||
Comment 19•7 years ago
|
||
Modern apps are filtered out from application and window sharing list for windows 8 or greater. This is successfully tested on win10 with gum test and talky.io.
Attachment #8687142 -
Attachment is obsolete: true
Attachment #8688436 -
Flags: review?(rjesup)
Comment 20•7 years ago
|
||
Comment on attachment 8688436 [details] [diff] [review] wip-bug1202087_toreview.patch Review of attachment 8688436 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc @@ +49,5 @@ > // windows as well (e.g. toolbars). > if (wcscmp(class_name, L"Progman") == 0 || wcscmp(class_name, L"Button") == 0) > return TRUE; > > + // Win8 introduced "Modern Apps" which associated window is very minor nit: which -> whose
Attachment #8688436 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 21•7 years ago
|
||
Updated with review comments.
Attachment #8688436 -
Attachment is obsolete: true
Assignee | ||
Comment 22•7 years ago
|
||
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ef4233cef0
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 23•7 years ago
|
||
Try is green. I set it for check in.
Comment 24•7 years ago
|
||
Hi, this failed to apply: patching file media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc Hunk #1 FAILED at 11 1 out of 2 hunks FAILED -- saving rejects to file media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh wip-bug1202087_reviewed.patch could you take a look, thanks!
Flags: needinfo?(achronop)
Assignee | ||
Comment 25•7 years ago
|
||
I upload the patch after pulling from central. If you still have problems please send me the .rej file.
Flags: needinfo?(achronop)
Comment 26•7 years ago
|
||
Comment 27•7 years ago
|
||
(In reply to Alex Chronopoulos [:achronop] from comment #25) > Created attachment 8689460 [details] [diff] [review] > bug1202087-non-shareable-app > > I upload the patch after pulling from central. If you still have problems > please send me the .rej file. hi, attached is the rej file
Flags: needinfo?(achronop)
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 28•7 years ago
|
||
Updated patch.
Attachment #8688467 -
Attachment is obsolete: true
Attachment #8689460 -
Attachment is obsolete: true
Attachment #8690834 -
Attachment is obsolete: true
Flags: needinfo?(achronop)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 29•7 years ago
|
||
hm this still fails to apply? 1 out of 2 hunks FAILED -- saving rejects to file media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug1202087-non-shareable-app do you know what is going on here ?
Flags: needinfo?(achronop)
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 30•7 years ago
|
||
It should be fine now. Sorry for the inconvenience.
Attachment #8690854 -
Attachment is obsolete: true
Flags: needinfo?(achronop)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 31•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd639be1a4
Keywords: checkin-needed
Comment 32•7 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=17865945&repo=mozilla-inbound
Flags: needinfo?(achronop)
Comment 33•7 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6499724b05d0
Assignee | ||
Comment 34•7 years ago
|
||
Patch updated to fix build error. Pushed to try to verify and the result is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=853484bb77c3 Hopefully no more surprises...
Attachment #8692453 -
Attachment is obsolete: true
Flags: needinfo?(achronop)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 35•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/958530278472
Keywords: checkin-needed
Comment 36•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/958530278472
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•