1634044 - getDisplayMedia with `height: { ideal: value }` leads to very strange resolutions somewhere between native and the requested one

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Lukas Kalbertodt]

Attachment: displayResolution.html

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.

Comment 1

[Lukas Kalbertodt]

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

getDisplayMedia is definitely not CSS code :)

Comment 3

[Byron Campen [:bwc]]

Can you confirm this?

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1634044 - Test precise downscaling with getDisplayMedia() & single constraint.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1634044 - Downscale getDisplayMedia() correctly with only width or height constraint present.

Depends on D76967

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 9

[Dan Minor [:dminor]]

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

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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]

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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()
#
#

Comment 12

[Dan Minor [:dminor]]

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.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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)

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1634044 - comment out assert(IsGUIThread(false)) in MouseCursorMonitorWin::Capture() to pass new tests.

Depends on D76968

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbf25d4bea9b
Test precise downscaling with getDisplayMedia() & single constraint. r=dminor
https://hg.mozilla.org/integration/autoland/rev/61441c5460ea
Downscale getDisplayMedia() correctly with only width or height constraint present. r=dminor
https://hg.mozilla.org/integration/autoland/rev/1e558aa6ab2b
comment out assert(IsGUIThread(false)) in MouseCursorMonitorWin::Capture() to pass new tests. r=dminor

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25687 for changes under testing/web-platform/tests

Comment 18

[Atila Butkovits]

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

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot