RFR: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library [v13]

[Alexey Ivanov]

On Fri, 6 Oct 2023 11:17:01 GMT, Renjith Kannath Pariyangad <duke at openjdk.org> wrote:

>> Hi Reviewers,
>> 
>> Observations :
>> 1. Without com initialize if we access Mixer for recording, library loaded invalid GUID and clipped description in windows(ID not found in registry). With com initialization library load proper GUID (same as registry).
>> 2. For Play back device always loading proper device GUID irrespective of com Initialization.
>> 
>> 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.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Whitespace error fixed
>  - Whitespace error fixed
>  - CoInitializeEx error check modified with better condition

Changes requested by aivanov (Reviewer).

src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp line 188:

> 186: 
> 187:     HRESULT hr=::CoInitializeEx(NULL, COINIT_MULTITHREADED | COINIT_DISABLE_OLE1DDE);
> 188:     if(hr != S_OK || hr != S_FALSE || hr != RPC_E_CHANGED_MODE) {

Suggestion:

    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.

src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_DirectSound.cpp line 234:

> 232:     }
> 233: 
> 234:     if(hr != RPC_E_CHANGED_MODE) {

Suggestion:

    if (hr != RPC_E_CHANGED_MODE) {

Space between the `if` keyword and the opening parenthesis.

-------------

PR Review: https://git.openjdk.org/jdk/pull/14898#pullrequestreview-1662190325
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348907643
PR Review Comment: https://git.openjdk.org/jdk/pull/14898#discussion_r1348908246