On Mon, 24 Jul 2023 09:58:09 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Renjith Kannath Pariyangad has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Disabled OLE1 from CoInit
>
> src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp
> line 482:
>
>> 480: DWORD WINAPI __stdcall DS_StartBufferHelper::ThreadProc(void *param)
>> 481: {
>> 482: ::CoInitializeEx(NULL, COINIT_MULTITHREADED |
>> COINIT_DISABLE_OLE1DDE);
>
> 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](https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ee416365(v=vs.85))
> uses `CoInitializeEx(NULL, 0)`.
>
> The documentation for
> [`CoInitializeEx`](https://learn.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-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?
> @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`](https://learn.microsoft.com/en-us/windows/win32/api/objbase/ne-objbase-coinit)
doesn't spell out the value for `COINIT_MULTITHREADED`.
> In my debug session noticed `DirectSoundCaptureEnumerate` getting 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:
>
> 1. 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](https://github.com/openjdk/jdk/pull/14898#discussion_r1272027291).)
> 2. 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.
> 3. 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`](https://learn.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-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_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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1273238491
