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

Move resume and reset into the wasapi audio thread #1620

Merged
merged 2 commits into from
Feb 23, 2015

Conversation

kevmitch
Copy link
Member

Moving these operations to the audio thread ensures that these operations are never happening simultaneously with thread_feed, which makes life much simpler.

I also made the pre-filling of the buffer happen only if there is less than a full buffer of audio already waiting to be played. This seems to be the key to remove fixing #1529 for me.

@ghost
Copy link

ghost commented Feb 23, 2015

I question the way it uses events to signal the state. (I know, this is how other things are handled here too; I don't like that either.)

What happens if both reset and resume are signaled? Or any other unlikely combination of states. Would that lead to chaos? But there's nothing that would prevent it.

IMO signaling state over a single state variable or so would be better, and maybe even less code.

In any case, if it helps, feel free to merge anyway.

@kevmitch
Copy link
Member Author

Are you suggesting that that MsgWaitForMultipleObjects should have a timeout and check the flags there periodically? If so, how is that really different from what's already going on?

The point about multiple signals is taken, and in light of that I should reverse the order of the reset and resume events in the event array given to MsgWaitForMultipleObjects. According to microsoft, if multiple events are signalled, MsgWaitForMultipleObjects will return and unsignal the first one in the array. Any remaining events will wait for the next call.

This deterministic order of processing multiple events seems like the same behaviour you would get from processing state variables in a timeout branch and doesn't add the confusion of mixing signalling methods.

@ghost
Copy link

ghost commented Feb 23, 2015

No, no polling. Just a single event, and a state variable.

What you suggest can also work, but doesn't it feel more complicated? You also have to manage all these event objects.

@ghost
Copy link

ghost commented Feb 23, 2015

PS: and I don't mean th block the PR either, so go ahead if you like.

This makes things a bit more deterministic. It ensures that the audio
thread isn't doing anything between IAudioClient_Stop(),
IAudioClient_Reset() and setting the sample_count to 0.

Buffer overfilling on resume is still a problem in exclusive mode (see
next commit).
This echanges the two events hForceFeed/hFeedDone for hResume. This
like the last commit makes things more deterministic.

Importantly, the forcefeed is only done if there is not already a full
buffer yet to be played by the device. This should fix some of the
problems with exclusive mode.

This commit also removes the necessity to have a proxy to the
AudioClient object in the main thread.

fixes mpv-player#1529
@kevmitch kevmitch merged commit c52833b into mpv-player:master Feb 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant