1202087 - WebRTC screen capture partially broken on windows 10

Status: RESOLVED FIXED

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

Description

[Philipp Hancke [:fippo]]

Attachment: captured window is shown on the right, captured stream in the upper left

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

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

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

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

jimm: can you give this a try in win10?  If you see it, any ideas?

Comment 3

[Jim Mathies [:jimm]]

I can't get this talky site to work, there's no prompt to share in nightly.

Comment 4

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

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?

Comment 5

[Justin Wood (:Callek)]

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)

Comment 6

[Jim Mathies [:jimm]]

I'll try again on a different device..

Comment 7

[Jim Mathies [:jimm]]

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?

Comment 8

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

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

Comment 9

[Philipp Hancke [:fippo]]

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.

Comment 10

[Jim Mathies [:jimm]]

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

Comment 11

[Jim Mathies [:jimm]]

also I was able to see universal app windows when sharing the desktop.

Comment 12

[Alex Chronopoulos [:achronop]]

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.

Comment 13

[Alex Chronopoulos [:achronop]]

Attachment: wip-bug1202087.patch

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.

Comment 14

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

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())) {
    ...
    ...
  }

Comment 15

[Alex Chronopoulos [:achronop]]

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

Comment 16

[Alex Chronopoulos [:achronop]]

Attachment: wip-bug1202087_updated.patch

Updated according to the comments.

Comment 17

[Alex Chronopoulos [:achronop]]

I tested this code and works but for application sharing. I guess we need to have something similar for window sharing?

Comment 18

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

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

Comment 19

[Alex Chronopoulos [:achronop]]

Attachment: wip-bug1202087_toreview.patch

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.

Comment 20

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

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

Comment 21

[Alex Chronopoulos [:achronop]]

Attachment: wip-bug1202087_reviewed.patch

Updated with review comments.

Comment 22

[Alex Chronopoulos [:achronop]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ef4233cef0

Comment 23

[Alex Chronopoulos [:achronop]]

Try is green. I set it for check in.

Comment 24

[Carsten Book [:Tomcat]]

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!

Comment 25

[Alex Chronopoulos [:achronop]]

Attachment: bug1202087-non-shareable-app

I upload the patch after pulling from central. If you still have problems please send me the .rej file.

Comment 26

[Carsten Book [:Tomcat]]

Attachment: rej file

Comment 27

[Carsten Book [:Tomcat]]

(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

Comment 28

[Alex Chronopoulos [:achronop]]

Attachment: bug1202087-non-shareable-app updated

Updated patch.

Comment 29

[Carsten Book [:Tomcat]]

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 ?

Comment 30

[Alex Chronopoulos [:achronop]]

Attachment: bug1202087-non-shareable-app new update

It should be fine now. Sorry for the inconvenience.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd639be1a4

Comment 32

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=17865945&repo=mozilla-inbound

Comment 33

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6499724b05d0

Comment 34

[Alex Chronopoulos [:achronop]]

Attachment: bug1202087-non-shareable-app fix build error

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

Comment 35

[Pulsebot]

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

Comment 36

[Carsten Book [:Tomcat]]

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