Closed Bug 1202087 Opened 4 years ago Closed 4 years ago

WebRTC screen capture partially broken on windows 10

Categories

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

42 Branch
x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
Blocking Flags:

People

(Reporter: fippo, Assigned: achronop)

Details

Attachments

(2 files, 8 obsolete files)

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.
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
jimm: can you give this a try in win10?  If you see it, any ideas?
Flags: needinfo?(jmathies)
Flags: needinfo?(bugspam.Callek)
backlog: --- → webrtc/webaudio+
Rank: 21
Priority: -- → P2
I can't get this talky site to work, there's no prompt to share in nightly.
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)
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)
I'll try again on a different device..
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)
(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)
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)
(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)
also I was able to see universal app windows when sharing the desktop.
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.
Attached patch wip-bug1202087.patch (obsolete) — Splinter Review
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: nobody → achronop
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())) {
    ...
    ...
  }
> 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.
Attached patch wip-bug1202087_updated.patch (obsolete) — Splinter Review
Updated according to the comments.
Attachment #8686659 - Attachment is obsolete: true
I tested this code and works but for application sharing. I guess we need to have something similar for window sharing?
(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.
Attached patch wip-bug1202087_toreview.patch (obsolete) — Splinter Review
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 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+
Attached patch wip-bug1202087_reviewed.patch (obsolete) — Splinter Review
Updated with review comments.
Attachment #8688436 - Attachment is obsolete: true
Keywords: checkin-needed
Try is green. I set it for check in.
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)
Attached patch bug1202087-non-shareable-app (obsolete) — Splinter Review
I upload the patch after pulling from central. If you still have problems please send me the .rej file.
Flags: needinfo?(achronop)
Attached file rej file (obsolete) —
(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 patch.
Attachment #8688467 - Attachment is obsolete: true
Attachment #8689460 - Attachment is obsolete: true
Attachment #8690834 - Attachment is obsolete: true
Flags: needinfo?(achronop)
Keywords: checkin-needed
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)
It should be fine now. Sorry for the inconvenience.
Attachment #8690854 - Attachment is obsolete: true
Flags: needinfo?(achronop)
Keywords: checkin-needed
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)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/958530278472
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1229518
No longer depends on: 1229518
You need to log in before you can comment on or make changes to this bug.