User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

• Open the attached file in Firefox
• Press a button with a height setting below your native screen height & share your whole screen
• Check the resolution of the returned video stream (which is printed)

Actual results:

The video stream resolution is neither the native one nor the request one, but instead somewhere in between, sometimes resulting in really strange resolutions (with odd numbers).

Expected results:

The video stream should be exactly the resolution that was requested.

Oh, I forgot. Here are some examples:

On a 1920*1080 screen:

• 480p -> 1386x780
• 720p -> 1600x900
• 1080p -> 1920x1080
• 1440p -> 1920x1080
• 2160p -> 1920x1080

On a 4k (3840*2160) screen:

• 480p -> 2346x1320
• 720p -> 2560x1440
• 1080p -> 2880x1620
• 1440p -> 3200x1800
• 2160p -> 3840x2160

This behavior is independent of the "UI scale" set in the operating system (tested with 100% and 200%). I also tested on Ubuntu and Windows.

getDisplayMedia is definitely not CSS code :)

Can you confirm this?

I can confirm. Here's one with a slider https://jsfiddle.net/jib1/5e1cbpyt/show

It appears that unless you constrain both width and height, this happens. Does not appear to be a regression. :-(

Depends on D76967

Dan what do you make of this here?

void MouseCursorMonitorWin::Capture() {
→ assert(IsGUIThread(false));

Windows only, and seems to come from the expanded test here.

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7)

Dan what do you make of this here?

void MouseCursorMonitorWin::Capture() {
→ assert(IsGUIThread(false));

Windows only, and seems to come from the expanded test here.

So IsGUIThread is a Windows thing that says the thread can receive UI events. The false just means we don't want to convert the current thread to a UI thread if it isn't already. These checks were added by us and are not present upstream, according to Randell we ran into some weird and hard to track down bugs when we tried to do window capture on non-"GUI" threads on Windows.

This seems like it might be a pre-existing problem that your new testing is exposing. I don't see anything offhand in the tests that would explain why this is now happening. I was wondering if maybe there was something racy with the thread initialization, but it looks like the gUM requests are sequential. Perhaps the old thread has not fully shutdown before we kick off a new one and that is causing problems? Maybe try some logging to see if this problem happens right away, or only after a few tests have run.

I can repro the failing assert in comment 9 locally on Windows 10 with the fiddle in comment 4 if I hold and drag the slider back and forth enough, hovering around low values. It's somewhat intermittent, but repros fairly reliably after just a couple of seconds.

Oddly, the callstack says it's on the PlatformUIThread, as it should be (i.e. similar to regular calls where assert doesn't hit):

xul.dll!webrtc::MouseCursorMonitorWin::Capture() Line 106 C++
xul.dll!webrtc::DesktopAndCursorComposer::CaptureFrame() Line 177 C++
xul.dll!webrtc::DesktopCaptureImpl::process() Line 662 C++
xul.dll!webrtc::DesktopCaptureImpl::Run(void * obj) Line 237 C++
xul.dll!rtc::PlatformUIThread::Run() Line 100 C++
xul.dll!rtc::PlatformThread::StartThread(void * param) Line 157 C++
[External Code]

I tried removing the assert, and eventually I then got the following:

#
# Fatal error in c:/moz/mozilla-central/dom/media/systemservices/video_engine/platform_uithread.cc, line 92
# last system error: 0
# Check failed: InternalInit()
#
#

If the hWnd ends up being null, maybe check GetLastError()?

It would also be interesting to add the assert(IsGUIThread(false)) to InternalInit(). I also wonder what would happen if you ran IsGUIThread(true) in InternalInit. That should convert the thread to a GUI thread if it is not already, but I'm not sure what the consequences of doing that there would be.

Notes to self: I modified the code to keep things going like this:

 void MouseCursorMonitorWin::Capture() {
-  assert(IsGUIThread(false));
+  if (!IsGUIThread(true)) {
+    RTC_LOG_F(LS_ERROR) << "No GUIThread info.";
+    return;
+  }
   assert(callback_);

...and instrumented like this (pardon the triple printfs, it was in response to pilot trouble getting output, but this matters at the end):

 void PlatformUIThread::Run() {
+  printf("JIB: HERE. %lu\n", GetLastError());
+  fprintf(stderr, "JIB: ERRHERE. %lu\n", GetLastError());
+  fprintf(stdout, "JIB: STDHERE. %lu\n", GetLastError());
   RTC_CHECK(InternalInit());  // always evaluates

@@ -30,26 +30,41 @@ bool PlatformUIThread::InternalInit() {
     HMODULE hModule = GetModuleHandle(NULL);
     if (!GetClassInfoW(hModule, kThreadWindow, &wc)) {
       ZeroMemory(&wc, sizeof(WNDCLASSW));
       wc.hInstance = hModule;
       wc.lpfnWndProc = EventWindowProc;
       wc.lpszClassName = kThreadWindow;
       RegisterClassW(&wc);
     }
+    printf("JIB: ZERO. %p\n", hwnd_);
+    fprintf(stderr, "JIB: ERRZERO. %p\n", hwnd_);
+    fprintf(stdout, "JIB: STDZERO. %p\n", hwnd_);
     hwnd_ = CreateWindowW(kThreadWindow, L"", 0, 0, 0, 0, 0, NULL, NULL,
                           hModule, NULL);
+    if (!hwnd_) {
+      printf("JIB: last error = %lu\n", GetLastError());
+      IsGUIThread(true);
+      hwnd_ = CreateWindowW(kThreadWindow, L"", 0, 0, 0, 0, 0, NULL, NULL,
+        hModule, NULL);
+      if (!hwnd_) {
+        printf("JIB: Retry last error = %lu\n", GetLastError());
+      }
+    }
     RTC_DCHECK(hwnd_);
     SetPropW(hwnd_, kThisProperty, this);

     if (timeout_) {
       // if someone set the timer before we started
       RequestCallbackTimer(timeout_);
     }
   }
+  printf("JIB: RETURN. %p\n", hwnd_);
+  fprintf(stderr, "JIB: ERRRETURN. %p\n", hwnd_);
+  fprintf(stdout, "JIB: STDRETURN. %p\n", hwnd_);
   return !!hwnd_;
 }

I then wiggled the sliders (which stops+starts capture a lot) until it provoked the RTC_CHECK(InternalInit()) to crash

tail...
JIB: ZERO. 0000000000000000
JIB: ERRZERO. 0000000000000000
JIB: STDZERO. 0000000000000000
JIB: RETURN. 000000000044067E
JIB: ERRRETURN. 000000000044067E
JIB: STDRETURN. 000000000044067E
JIB: HERE. 0
JIB: ERRHERE. 0
JIB: STDHERE. 0
JIB: RETURN. 000000000044067E
JIB: ERRRETURN. 000000000044067E
JIB: STDRETURN. 000000000044067E
JIB: HERE. 0
JIB: ERRHERE. 0
JIB: STDHERE. 0
JIB: ZERO. 0000000000000000
JIB: ERRZERO. 0000000000000000
JIB: STDZERO. 0000000000000000
JIB: RETURN. 000000000045067E
JIB: ERRRETURN. 000000000045067E
JIB: STDRETURN. 000000000045067E
JIB: HERE. 0
JIB: ERRHERE. 0
JIB: STDHERE. 0
JIB: ZERO. 0000000000000000
JIB: ERRZERO. 0000000000000000
JIB: STDZERO. 0000000000000000
JIB: RETURN. 000000000046067E
JIB: ERRRETURN. 000000000046067E
JIB: STDRETURN. 000000000046067E
JIB: HERE. 0
JIB: ERRHERE. 0
JIB: STDHERE. 0
JIB: RETURN. 000000000046067E
JIB: ERRRETURN. 0000000000000000
JIB: STDRETURN. 0000000000000000


#
# Fatal error in c:/moz/mozilla-central/dom/media/systemservices/video_engine/platform_uithread.cc, line 110
# last system error: 0
# Check failed: InternalInit()
#
#

Looking at the last 3 lines before the crash, it looks like hwnd_ got cleared in between adjacent printf statements, suggesting a race.

(this is without the patch in this bug fyi)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jib, could you have a look please?
For more information, please visit auto_nag documentation.

Depends on D76968

https://hg.mozilla.org/mozilla-central/rev/bbf25d4bea9b
https://hg.mozilla.org/mozilla-central/rev/61441c5460ea
https://hg.mozilla.org/mozilla-central/rev/1e558aa6ab2b