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

Remove eventsExecutorService thread #50

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

HelgeStenstrom
Copy link
Collaborator

I don't think it's needed.
The player runs fine without it. The call() method of StreamPlayerEventLauncher gets called anyway.

See also #49 .

Slight reformatting
Executors.newSingleThreadExecutor(new ThreadFactoryWithNamePrefix("StreamPlayerEvent"))); is not needed
@goxr3plus
Copy link
Owner

goxr3plus commented Sep 24, 2019

It's needed i specifically wrote that mechanism to deal with multiple threads calling the same player with different events.

I had issues in XR3Player and spend two nights to solve it.

I will tell you a very simple example.

Stop player is fired and half of the procedure happens and just before the player is stopped the pause triggers from another thread and causes a deadlock.

I can be more specific i remember that happening more than 10 times and the player was stack.

@HelgeStenstrom
Copy link
Collaborator Author

Yes, please be more specific.
I'm interested to learn about the scenario where multiple threads are calling the same player. What is the use case?

How would it be helped by having a separate temporary event thread that doesn't run in parallel, but rather instead of the main thread? On

return eventsExecutorService
.submit(new StreamPlayerEventLauncher(this, status, encodedStreamPosition, description, listeners))
.get();

you are creating a task, start it on the executor, and wait for it complete, rather than just starting it and then do other things in parallel.

@goxr3plus
Copy link
Owner

goxr3plus commented Sep 25, 2019

It's because the upcoming events must run in row instead of one interrupting the other.

The guys from xTremepamedia player which i have send you before have also noticed that and used the exact same solution on their class.

Imagine we had the same idea even for await method :).

I implemented it through trial and error.

@HelgeStenstrom
Copy link
Collaborator Author

I don’t think it looks like the same solution. Compare their play method to yours. They have most of it guarded by a thread lock, you have not. (I’m browsing the code on my mobile phone, so I might be wrong).

Your generateEvent method creates a temporary thread or task on an existing thread, I’m not sure which. They don’t.

@HelgeStenstrom
Copy link
Collaborator Author

I need to correct myself. Their notifyEvent really does create a thread. But they don’t wait for its termination or for it to return a result. I don’t understand their solution either.

goxr3plus and others added 16 commits November 26, 2019 09:40
Some unit tests are improved, but there are more to be done.
StreamPlayerMethodsTest still contains tests that don't pass. The tested methods are candidates for removal from StreamPlayer.

All tests in StreamPlayerMethodsTest must be reviewed: Do they actually verify anything that need to be verified? Or are they too coupled with the current implementation?

StreamPlayerFutureImprovementTest contains tests that currently fail. Failures are caused by behavior in StreamPlayer which I think is wrong or bad. But I may have misinterpreted the intended behavior.
AudioSystem.getMixerInfo() will not return null, so we don't have to …
@goxr3plus goxr3plus added enhancement improvement Refactorings and internal structure improvements labels Dec 31, 2019
goxr3plus and others added 4 commits March 31, 2020 14:49
Slight reformatting
Executors.newSingleThreadExecutor(new ThreadFactoryWithNamePrefix("StreamPlayerEvent"))); is not needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvement Refactorings and internal structure improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants