search

chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.35k stars 467 forks source link

Chomium / Windows - When using own Low Level Keyboard Hook, Hook is lost on WebRTC session start #2609

Closed magreenblatt closed 5 years ago

magreenblatt commented 5 years ago

Original report by Romain Caire (Bitbucket: Romain Caire).

What steps will reproduce the problem?

  1. Add your own Low Level Keyboard Hook (LLKH) to your app and run CEF3 in OSR Mode enabling WebRTC Sample code for this test can be found here: https://github.com/iburel/cef. Run using flags --off-screen-rendering-enabled --enable-media-stream to enable OSR Mode and WebRTC
  2. Run your app, press some keys, the LLKH is called successfully.
  3. Run your app to a website using WebRTC (for example: https://appr.tc/) and now your LLKH is not working anymore...

What version of the product are you using? On what operating system?

Tested with the latest Master on Windows 10 (Lastest Stable)

magreenblatt commented 5 years ago
  1. Low Level Keyboard Hook is not something that CEF supports.
  2. With OSR your application is the source of all keyboard events. There is no reason to use Low Level Keyboard Hook with OSR.
magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

Sorry If I wasn't clear.

We use a Low Level Keyboard Hook in our own code (with our Msg Loop on Windows) in order to prevent some keys to be propagated to the OS (For exemple, the windows key) In this low level keyboard hook, we receive keyboard events (key press/release) and the forward them to CEF using the regular CEF API 

#!c++
browser_->GetHost()->SendKeyEvent(event);

But we've noticed that when we start a WebRTC session in CEF, our global Low Level Keyboard Hook is not called anymore (thru not having any inputs in CEF or our game) A simple test [1] is hooking a LLKH to the CefClient WM_CREATE OSR Window message and logging to Debug Output.

On a non WebRTC websites (Google.com or any others) our LLKH is called (logs are showing) but once we start a WebRTC session our LLKH is not called anymore.

[1] You can find our test here : https://github.com/iburel/cef (need to be run with flags : --off-screen-rendering-enabled --enable-media-stream)

I believe Chromium (for security reason) blocks us from using a LLKH while in a WebRTC session. I've tried to find some trace of LLKH within Chromium but I failed. The only reference I found [2] seams never been called (I added some trace to Chromium source and tested)

[2] https://cs.chromium.org/chromium/src/ui/events/win/keyboard_hook_win_base.cc?q=KEYBOARD_LL&sq=package:chromium&g=0&l=40

magreenblatt commented 5 years ago

Thanks for the explanation. That's something you'll need to take up with the Chromium devs.

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

Alright, I will create an issue.

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

I've posted on Chromium bug tracker : https://bugs.chromium.org/p/chromium/issues/detail?id=935228 I've also found a workaround by commenting some code in Chromium (see Chromium bug for more info) but I'm not sure the side effects it has.

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

Hi, Chromium team told me that since it's not a Chromium bug but a CEF bug within Chromium it was not their resort. :( You can see their answer here : https://bugs.chromium.org/p/chromium/issues/detail?id=935228

I've found a fix but I'm not sure how to propose a PR in CEF for a Chromium Bug ? Maybe through CEF Chromium patches ?

magreenblatt commented 5 years ago

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).

Submitting a patch for chromium changes is quite easy:

  1. Create patch for the chromium source tree that only includes your changes
  2. add a new entry in patch/patch.cfg with your patch file created earlier place in patch/patches

You might want to look at https://bitbucket.org/chromiumembedded/cef/src/master/patch/README.txt where it is also explained how that works.

But if I read the linked issue correctly you are proposing to just disable user input monitoring, if my memory serves correctly this was used to inform webrtc that the user is currently typing so that the webrtc audio layer could filter out the typing noise. It might have changed by now, but that's what I remember from a few years ago.

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

I've created a PR with a Patch that apply at build time that disable the "incriminated" code.

https://bitbucket.org/chromiumembedded/cef/pull-requests/213/completely-disable-the-webrtc-user-input/diff

We've tested successfully this solution and it seams to fix the issue 100% in our end. I didn't see any side-effects with disabling this code.

The real issue lies somewhere within windows not being able to have multiple LLKH/RawInputDevice on multiple window within the same process... See this SO issue : https://stackoverflow.com/questions/9702878/multiple-raw-input-window-sinks

magreenblatt commented 5 years ago

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).

So I actually did double check, one side effect being that this changes the behavior of the TransientSuppressor used in WebRTC [1]
While I can see the use-case for raw input capture, I don't think unconditionally disabling this is the right way to go. (I am ignoring LLKH here because those are just always bad)

We are now talking about making a trade-off, support raw input capture and potentially degrade webrtc audio quality or just leave it as is and leave it up to the user to figure out a way for them to either patch it or find a way to support raw input capture in a different way (There is always the option of going out-of-process)

Personally I would be leaning towards classifying this as WontFix because I don't think the trade off is worth it IMHO.

[1] https://webrtc.googlesource.com/src/+/refs/heads/master/modules/audio_processing/transient/transient_suppressor.cc#173

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

Ok, could you tell me a bit on what the TransientSuppressor does ? The goal is to remove keypress "clicks" in audio stream ?

Could we do a run-time command line flag that disable this ?

magreenblatt commented 5 years ago

Original comment by Alexander Guettler (Bitbucket: xforce, GitHub: xforce).

Ok let me try to explain, as the name suggests the transient suppressor is used to suppress transients in the audio stream. A transient is a short high-amplitude sound, it does this by attenuating those unexpected spikes in the spectrum. In the case of WebRTC it only does this for key presses, meaning the the state of key presses is driving the enabled state of the TransientSuppressor, in the actual implementation of WebRTC it is enabled when a buffer was passed and a key press was detected for that buffer and then gets disabled after 4 seconds of no typing.

TLDR: the end goal is to reduce the loud noise of a keypresses.

magreenblatt commented 5 years ago

Original comment by Romain Caire (Bitbucket: Romain Caire).

Understood and understandable that you would want to keep this feature. Can we add a command line flag --disable-input-monitoring to disable the input monitoring ?

magreenblatt commented 5 years ago

A command-line flag that requires minimal Chromium changes would be more acceptable then the PR as submitted.

magreenblatt commented 5 years ago

Marking this issue as WontFix for now. We can re-open if an acceptable PR is submitted.

magreenblatt commented 4 years ago

Original comment by Damian Büchel (Bitbucket: Damian Büchel).

We are using CEF (CefSharp, to be precise) for a kiosk application which uses low-level keyboard hooks to monitor user input (see https://github.com/SafeExamBrowser). It is imperative for us to be able to use WebRTC and keyboard hooks at the same time, we thus would very much welcome a command-line flag as proposed above.

@{557058:2f2a2aee-b500-4023-9734-037e9897c3ab} As this is a major issue for us, we would be willing to contribute financially towards a timely fix for this bug.

magreenblatt commented 4 years ago

Original comment by Marvin Herbold (Bitbucket: Marvin Herbold).

Romain, would you be willing to update your PR and make it work how you proposed it to, via a --disable-input-monitoring command line flag? We sorely need this fix. Marshall has said that he will accept your PR if you update it to work based off a command line flag. Damian has said they would be willing to contribute financially if that is something you need.

Pretty please?

magreenblatt commented 3 years ago

Original comment by Damian Büchel (Bitbucket: Damian Büchel).

To whom it may concern: The issue apparently has been resolved as of early this year. It is unclear to us whether it was due to a change in Chromium or CEF, but low-level hooks now seem to work when WebRTC is enabled.

magreenblatt commented 1 year ago

Original comment by Stephen James (Bitbucket: Stephen James).

What version of CEF did this resolution appear in?

magreenblatt commented 5 years ago

Original changes by Romain Caire (Bitbucket: Romain Caire).

magreenblatt commented 5 years ago
magreenblatt commented 5 years ago

Original changes by Romain Caire (Bitbucket: Romain Caire).

magreenblatt commented 5 years ago
dbuechel commented 1 year ago

It does appear that there is a regression, as the issue has reappeared according to a report from our community: https://github.com/SafeExamBrowser/seb-win-refactoring/issues/471. It would be great if there was a way to make WebRTC and low-level hooks work together, as at least we in some of our use cases require them both to function as expected simultaneously.