Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264737: JavaFX media stream stops playing after reconnecting via Remote Desktop #479

Closed
wants to merge 2 commits into from

Conversation

sashamatveev
Copy link
Member

@sashamatveev sashamatveev commented Apr 24, 2021

Remote Desktop will change default audio device when connecting or disconnecting to it's own audio device. Also, when remote desktop is disconnect, then remote desktop default audio device is removed and system default device is not restored until user logs back to computer. So, after remote desktop is disconnected, then system left without any default audio devices.

To fix this we will unload DirectSound when audio device is gone and will continue playback by throwing away audio data. Once we receive notification that default audio device is back, we will load it and continue playback via this device.

Loading device done in loop, since it is not always available right after notification, but will be after very short period of time.

Tested by connecting/disconnecting remote desktop and switching between remote and normal desktop. In second case audio will switch between remote or speakers.

Since audio device is gone and not restored after disconnect, there will be no audio on machine local speakers, until user logs back again.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264737: JavaFX media stream stops playing after reconnecting via Remote Desktop

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/479/head:pull/479
$ git checkout pull/479

Update a local copy of the PR:
$ git checkout pull/479
$ git pull https://git.openjdk.java.net/jfx pull/479/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 479

View PR using the GUI difftool:
$ git pr show -t 479

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/479.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 24, 2021

👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Apr 24, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 24, 2021

Webrevs

@kevinrushforth kevinrushforth self-requested a review Apr 24, 2021
@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 24, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Apr 24, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth requested a review from arapte Apr 26, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

As far as I can tell, the code changes look fine to me.

While testing this, the stream always keeps playing now. Sometimes I can't hear the audio, when it reconnects while playing the stream the first time. What I did was:

  1. Run Ensemble
  2. Select the "Audio Bar Chart" sample
  3. Disconnect from remote desktop
  4. Reconnect to remote desktop

The audio stream continues to play, as seen by the chart continuing to be updated, but I can't hear anything. If I select any other sample, and then select the Audio Bar Chart sample again, I can disconnect / reconnect and everything is fine -- I can hear the audio.

Btw, the same thing happens if I initially select the Advanced Media sample. If it is the first media sample that is run, then when I disconnect / reconnect, I can't hear anything, but the video keeps playing.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 26, 2021

As a related data point, even if I don't disconnect while playing the initial media stream, if I do the following, there is no problem and I can hear the audio on reconnect:

  1. Run Ensemble
  2. Select the "Audio Area Chart"
  3. Select the "Audio Bar Chart" sample
  4. Disconnect from remote desktop
  5. Reconnect to remote desktop

No problem.

@sashamatveev
Copy link
Member Author

sashamatveev commented Apr 27, 2021

Audio device notification failed in case when CoInitialize() was not called. It worked in some cases, since other parts of JavaFX already initialized it. Fixed by adding CoInitialize() call before registering device notification. Also, removed unused header left from debug printf.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@arapte
Copy link
Member

arapte commented Apr 28, 2021

The change fixes the issue and looks good.
While testing it regressively, I observed an issue. The issue can occur always.
steps to reproduce:

  1. Run Ensemble app on remote desktop
  2. Run Streaming Media Player app
  3. Press the media player buttons for several times. Keep pressing different buttons for a minute or more and at the end press stop backward.
  4. Disconnect remote desktop and connect again
  5. Press play button
  6. I observed two different issues at two different instances
  • Video and audio bot do not play
  • Audio plays but video does not play

However, Looking at the steps to reproduce this does not seem like a stopper to me, so approving
Let me know if you can or cannot reproduce the issue, we can take that as a follow on issue.

arapte
arapte approved these changes Apr 28, 2021
@openjdk
Copy link

openjdk bot commented Apr 28, 2021

@sashamatveev 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:

8264737: JavaFX media stream stops playing after reconnecting via Remote Desktop

Reviewed-by: kcr, arapte

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 5 new commits pushed to the master branch:

  • cc70cdf: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners
  • 33bbf3f: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene
  • 483f171: 8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false
  • 6b63bf5: 8265669: AccumCell should not be visible
  • ed080c8: 8262276: Debug build of WebKit fails

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Apr 28, 2021
@sashamatveev
Copy link
Member Author

sashamatveev commented Apr 28, 2021

@arapte I was not able to reproduce issue as described. It might not be related to removal of audio device during Remote Desktop disconnect. I will go ahead and integrate fix. Can you file a bug for it?

@sashamatveev
Copy link
Member Author

sashamatveev commented Apr 28, 2021

/integrate

@openjdk openjdk bot closed this Apr 28, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Apr 28, 2021
@openjdk
Copy link

openjdk bot commented Apr 28, 2021

@sashamatveev Since your change was applied there have been 5 commits pushed to the master branch:

  • cc70cdf: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners
  • 33bbf3f: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene
  • 483f171: 8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false
  • 6b63bf5: 8265669: AccumCell should not be visible
  • ed080c8: 8262276: Debug build of WebKit fails

Your commit was automatically rebased without conflicts.

Pushed as commit 0a68613.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants