8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library #14898
Conversation
…eChooser or COM library
|
👋 Welcome back Renjithkannath! A progress list of the required criteria for merging this PR into |
Renjithkannath
changed the title
|
@Renjithkannath The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
I think this patch would also address https://bugs.openjdk.org/browse/JDK-7116070 |
| @@ -479,7 +479,7 @@ DS_StartBufferHelper::Data::~Data() { | |||
|
|
|||
| DWORD WINAPI __stdcall DS_StartBufferHelper::ThreadProc(void *param) | |||
| { | |||
| ::CoInitialize(NULL); | |||
| ::CoInitializeEx(NULL, COINIT_MULTITHREADED); | |||
There was a problem hiding this comment.
The documentation for the CoInitializeEx suggests to disable the COINIT_DISABLE_OLE1DDE:
https://learn.microsoft.com/en-us/windows/win32/learnwin32/initializing-the-com-library
I think we do not need ole1 here and can disable that.
There was a problem hiding this comment.
@mrserb, thank you for your review I have disabled ole1 by adding the flag COINIT_DISABLE_OLE1DDE.
|
@Renjithkannath This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1933 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@Renjithkannath |
| ::CoInitialize(NULL); | ||
| ::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); |
There was a problem hiding this comment.
You're changing the COM for this thread from COINIT_APARTMENTTHREADED to COINIT_MULTITHREADED.
Do DirectSound objects support multi-threading? I couldn't find anything quickly.
A Microsoft sample for Creating the Device Object uses CoInitializeEx(NULL, 0).
The documentation for CoInitializeEx says, “You need to initialize the COM library on a thread before you call any of the library functions…”
This implies that any thread which creates DirectSound objects must initialise COM first. I can't see it happening. Do I miss anything?
There was a problem hiding this comment.
@aivanov-jdk, Thank you for your time and reviews,
CoInitializeEx(NULL, 0) also resolving this problem because as per document The default is COINIT_MULTITHREADED.
In my debug session noticed DirectSoundCaptureEnumerate getting called ahead of thread call or ::CoInitialize and that was the root cause this failure.
For resolving this found 3 ways and all works:
- Use COM multi-threading :- Simplest approach with respect to other solutions
- Hold the call till CoInitialize : - Can end up in deadlock situation so that need to be taken care.
- Do another CoInitialize before call :- not a good approach
This implies that any thread which creates DirectSound objects must initialise COM first. I can't see it happening. Do I miss anything?
DS_StartBufferHelper class initializing COM in thread and all member functions checking initialization status before making any calls. So another solution may be restructure the code for accessing isInitialized() method from outer methods and proceed. (approach 2 in better way)
Tried this approach with ::CoInitialize and observed truncated names, but its not failing for lock unlock workflow.
There was a problem hiding this comment.
@aivanov-jdk, Thank you for your time and reviews,
CoInitializeEx(NULL, 0)also resolving this problem because as per document The default is COINIT_MULTITHREADED.
This is what I expected, however, the documentation for COINIT doesn't spell out the value for COINIT_MULTITHREADED.
In my debug session noticed
DirectSoundCaptureEnumerategetting called ahead of thread call or ::CoInitialize and that was the root cause this failure.
If it is the case, I can't see that you have ensured that DirectSoundCaptureEnumerate is called after CoInitialize. Do I miss anything?
For resolving this found 3 ways and all works:
- Use COM multi-threading :- Simplest approach with respect to other solutions
Probably… But it is still incorrect. Initialising COM on a thread doesn't mean you can call COM object methods from any thread in your application.
As the documentation says, “You need to initialize the COM library on a thread before you call any of the library functions…” (I have this very quote in my comment above.)
- Hold the call till CoInitialize : - Can end up in deadlock situation so that need to be taken care.
It's a challenge but it is the only right way, as far as I can see… Perhaps not to wait till ::CoInitialize is called on a thread (see above) but to transfer a call for handling to a thread where COM is initialised.
- Do another CoInitialize before call :- not a good approach
Yet it does not break the rules: you can initialise COM, call an API and then call CoUninitialize. If you don't keep any references to COM objects, it is valid.
This implies that any thread which creates DirectSound objects must initialise COM first. I can't see it happening. Do I miss anything?
DS_StartBufferHelperclass initializing COM in thread and all member functions checking initialization status before making any calls. So another solution may be restructure the code for accessingisInitialized()method from outer methods and proceed. (approach 2 in better way)Tried this approach with
::CoInitializeand observed truncated names, but its not failing for lock unlock workflow.
As I explained above, it would still be incorrect: if enumerating sound devices requires COM, it must be initialised on the thread where you enumerate the devices.
There was a problem hiding this comment.
@aivanov-jdk, Thank you for your time and reviews,
CoInitializeEx(NULL, 0)also resolving this problem because as per document The default is COINIT_MULTITHREADED.This is what I expected, however, the documentation for
COINITdoesn't spell out the value forCOINIT_MULTITHREADED.
As per the declaration yes its 0 tagCOINIT
In my debug session noticed
DirectSoundCaptureEnumerategetting called ahead of thread call or ::CoInitialize and that was the root cause this failure.
If it is the case, I can't see that you have ensured that
DirectSoundCaptureEnumerateis called afterCoInitialize. Do I miss anything?
Yes, You are right in main thread no CoInitialize
For resolving this found 3 ways and all works:
- Use COM multi-threading :- Simplest approach with respect to other solutions
Probably… But it is still incorrect. Initialising COM on a thread doesn't mean you can call COM object methods from any thread in your application.
As the documentation says, “You need to initialize the COM library on a thread before you call any of the library functions…” (I have this very quote in my comment above.)
- Hold the call till CoInitialize : - Can end up in deadlock situation so that need to be taken care.
It's a challenge but it is the only right way, as far as I can see… Perhaps not to wait till
::CoInitializeis called on a thread (see above) but to transfer a call for handling to a thread where COM is initialised.
These two approach didn't do ::CoInitialize in running thread (main thread)
- Do another CoInitialize before call :- not a good approach
Yet it does not break the rules: you can initialise COM, call an API and then call
CoUninitialize. If you don't keep any references to COM objects, it is valid.
Tried this approach and it works, we may need to do the similar approach for another function DAUDIO_GetDirectAudioDeviceDescription because this also calling DirectSoundEnumerateW & DirectSoundCaptureEnumerateW
This implies that any thread which creates DirectSound objects must initialise COM first. I can't see it happening. Do I miss anything?
DS_StartBufferHelperclass initializing COM in thread and all member functions checking initialization status before making any calls. So another solution may be restructure the code for accessingisInitialized()method from outer methods and proceed. (approach 2 in better way)
Tried this approach with::CoInitializeand observed truncated names, but its not failing for lock unlock workflow.
As I explained above, it would still be incorrect: if enumerating sound devices requires COM, it must be initialised on the thread where you enumerate the devices.
Got your point.
There was a problem hiding this comment.
Probably… But it is still incorrect. Initialising COM on a thread doesn't mean you can call COM object methods from any thread in your application.
Note that "Objects created on a COM thread in a multithread apartment (MTA) must be able to receive method calls from other threads at any time", so if we initialisethe devices on one thread using COM they will be avaliable on other threads as well. But the spec for DirectSoundEnumerate says nothing about COM... But from some examples I see ppl wrap it by the CoInitializeEx(NULL, COINIT_MULTITHREADED)/CoUninitialize();
Not sure we can call CoInitializeEx on the main thread.
There was a problem hiding this comment.
- Do another CoInitialize before call :- not a good approach
Yet it does not break the rules: you can initialise COM, call an API and then call
CoUninitialize. If you don't keep any references to COM objects, it is valid.Tried this approach and it works, we may need to do the similar approach for another function
DAUDIO_GetDirectAudioDeviceDescriptionbecause this also callingDirectSoundEnumerateW&DirectSoundCaptureEnumerateW
Does this approach return full names for devices, without truncating them?
There was a problem hiding this comment.
But if that code requare com my expectation would be that they call CoInitialize/CoUninitialize.
This CoInitialize initially was added here https://bugs.openjdk.org/browse/JDK-6950553 (check the stack trace),
b8b9c35 can we check that the DAUDIO_GetDirectAudioDeviceDescription is really uses that com object w/o initialisation?
- Call CoInitializeEx, enumerate devices, and call CoUninitialize.
- Create an executor service which initialises COM on its worker thread, enumerate devices on the worker thread and return the devices. The worker thread may terminate (after it calls CoUninitialize) if it doesn't receive more requests in a period of time.
We can try reuse the "Filechooser.invokeCom" machinery, but I do not think I like it.
There was a problem hiding this comment.
is it possible that our "::CoInitialize(NULL);" prevents the ::CoInitializeEx(NULL, COINIT_MULTITHREADED); in the lib later?
There was a problem hiding this comment.
is it possible that our "::CoInitialize(NULL);" prevents the ::CoInitializeEx(NULL, COINIT_MULTITHREADED); in the lib later?
It is not only possible, it is explicitly stated. If the threading mode is different, the function fails with RPC_E_CHANGED_MODE.
But if that code requare com my expectation would be that they call CoInitialize/CoUninitialize.
Exactly! Microsoft docs don't state COM needs be initialised for enumerating devices but it looks like it needs to be.
This CoInitialize initially was added here JDK-6950553 (check the stack trace)
It somewhat confirms, you can't access COM without initialising it.
There was a problem hiding this comment.
We can try reuse the "Filechooser.invokeCom" machinery, but I do not think I like it.
As things look now, I'm inclined to use a similar facility as Filechooser.invokeCom but separate and independent. I mean starting an executor service or a thread which initialises COM and handles enumeration requests. Or re-using that servicing thread that's already exists where COM initialisation was added by JDK-6950553.
I couldn't find an explicit statement whether DirectSound objects require STA or support MTA. If everything works with MTA, I prefer switching to ::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE).
|
The test case attached to JDK-8301846 breaks after I wonder if |
It didn't, the test case still produces truncated names. Although, the mixer name used for recording in the stand-alone test attached to JDK-8301846 is not truncated any more. |
Partially fixing, with the fix this was the output on my machine : Without fix would be like this: mixer 5: >>Microphone Array (Realtek(R) Au<< |
Right. Interesting. |
|
I created a short native app, By default, it calls I get the following output: C:\dev\audio>SoundDevices.exe 0
Sound Devices
000001A3168F1104
{1A29CAD2-72C7-40B4-A39B-94D71FC91E3F}
Speakers (Realtek(R) Audio)
{0.0.0.00000000}.{1a29cad2-72c7-40b4-a39b-94d71fc91e3f}
Sound Capture Devices
000001A3168F0D24
{BDF35A00-B9AC-11D0-A619-00AA00A7C000}
Microphone Array (Realtek(R) Au
{0.0.1.00000000}.{473909c9-5435-4fb1-ab77-18838bdfa76c}
C:\dev\audio>SoundDevices.exe 1
Sound Devices
000001A56FDB1104
{1A29CAD2-72C7-40B4-A39B-94D71FC91E3F}
Speakers (Realtek(R) Audio)
{0.0.0.00000000}.{1a29cad2-72c7-40b4-a39b-94d71fc91e3f}
Sound Capture Devices
000001A56FDB0D24
{473909C9-5435-4FB1-AB77-18838BDFA76C}
Microphone Array (Realtek(R) Audio)
{0.0.1.00000000}.{473909c9-5435-4fb1-ab77-18838bdfa76c}(I removed Primary devices from the output above.) It confirms Renjith's observations: if COM isn't initialised, the description of the capture device, Microphone Array, gets truncated, and its GUID is different from the driver GUID. It get the same results on other systems where I ran the app. |
The latest code still produces truncated strings for the test case in |
|
@aivanov-jdk, thank you very much for your review, JDK-8301846 helps for fixing Primary Sound Driver recording device name truncation issue. |
I referred to JDK-7116070. The root cause of that problem seems to be same as this one. While you and the reviewers have all the context, I suggest looking into JDK-7116070 in more details to understand why there are still devices, or rather ports, with truncated descriptions. |
| HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); | ||
| if(hr != S_OK || hr != S_FALSE || hr != RPC_E_CHANGED_MODE) { |
There was a problem hiding this comment.
| HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); | |
| if(hr != S_OK || hr != S_FALSE || hr != RPC_E_CHANGED_MODE) { | |
| HRESULT hr = ::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); | |
| if (FAILED(hr) && hr != RPC_E_CHANGED_MODE) { |
You said Microsoft recommends using FAILED and SUCCEEDED macros, so we should use them here too.
I think you got the condition wrong. The valid values to continue are
if (hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE) {
// Good - do the stuff
}Here we need to reverse the condition:
if (!(hr == S_OK || hr == S_FALSE || hr == RPC_E_CHANGED_MODE)) {
// Unlock and return
}To negate the condition, you negate all the operators and OR becomes AND. Thus, the condition becomes:
if (hr != S_OK && hr != S_FALSE && hr != RPC_E_CHANGED_MODE)) {
// Unlock and return
}The first two conditions could be converted to FAILED(hr), which gives us the condition I suggested.
| @@ -224,6 +230,11 @@ INT32 DAUDIO_GetDirectAudioDeviceCount() { | |||
|
|
|||
| g_lastCacheRefreshTime = (UINT64) timeGetTime(); | |||
| } | |||
|
|
|||
| if(hr != RPC_E_CHANGED_MODE) { | |||
There was a problem hiding this comment.
| if(hr != RPC_E_CHANGED_MODE) { | |
| if (hr != RPC_E_CHANGED_MODE) { |
Space between the if keyword and the opening parenthesis.
There was a problem hiding this comment.
Included all suggestions
| @@ -184,6 +184,12 @@ INT32 DAUDIO_GetDirectAudioDeviceCount() { | |||
| return 0; | |||
| } | |||
|
|
|||
| HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); | |||
There was a problem hiding this comment.
| HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); | |
| HRESULT hr = ::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE); |
Spaces around the assignment operator.
|
@mrserb Could you take a look at the updated code, please? |
@Renjithkannath Please, add this evaluation to JDK-7116070. The bug itself can be closed as External: we can do nothing about it, it's a limitation of Windows API. If Java migrates to a newer audio API, JDK-7116070 will be resolved. |
|
Thanks @aivanov-jdk, I have closed JDK-7116070 marked as External by adding evaluation into comment. |
|
/integrate |
|
@Renjithkannath |
|
/sponsor |
|
Going to push as commit 613a3cc.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@aivanov-jdk @Renjithkannath Pushed as commit 613a3cc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@Renjithkannath To use the |
Hi Reviewers,
Observations :
Test:
Since screen lock and unlock workflow required for reproducing this issue, did coupe of iteration of manual testing post fix and confirmed its resolving the problem.
To reconfirm nothing is broken, executed all audio related test cases on test bench post fix and all are green.
Please review the changes and let me know your comments if any.
Regards,
Renjith.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14898/head:pull/14898$ git checkout pull/14898Update a local copy of the PR:
$ git checkout pull/14898$ git pull https://git.openjdk.org/jdk.git pull/14898/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14898View PR using the GUI difftool:
$ git pr show -t 14898Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14898.diff
Webrev
Link to Webrev Comment