-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Revert "ao_coreaudio: switch to ao_read_data_nonblocking()" #13902
Conversation
This reverts commit 36d5b52.
Download the artifacts for this pull request: |
Maybe 0341a6f needs to be reverted too? |
No, the blocking function does not guarantee that |
Is it possible to do the same for ao_avfoundation? diff --git a/audio/out/ao_avfoundation.m b/audio/out/ao_avfoundation.m
index 7654916519..61bf39620e 100644
--- a/audio/out/ao_avfoundation.m
+++ b/audio/out/ao_avfoundation.m
@@ -76,12 +76,7 @@ static void feed(struct ao *ao)
int64_t cur_time_mp = mp_time_ns();
int64_t end_time_av = MPMAX(p->end_time_av, cur_time_av);
int64_t time_delta = CMTimeGetNanoseconds(CMTimeMake(request_sample_count, samplerate));
- int real_sample_count = ao_read_data_nonblocking(ao, data, request_sample_count, end_time_av - cur_time_av + cur_time_mp + time_delta);
- if (real_sample_count == 0) {
- // avoid spinning by blocking the thread
- mp_sleep_ns(10000000);
- goto finish;
- }
+ int real_sample_count = ao_read_data(ao, data, request_sample_count, end_time_av - cur_time_av + cur_time_mp + time_delta);
if ((err = CMBlockBufferCreateWithMemoryBlock(
NULL, |
@kasper93 avfoundation can well handle audio buffer of variable length. |
@@ -89,7 +89,7 @@ static OSStatus render_cb_lpcm(void *ctx, AudioUnitRenderActionFlags *aflags, | |||
|
|||
int64_t end = mp_time_ns(); | |||
end += p->hw_latency_ns + ca_get_latency(ts) + ca_frames_to_ns(ao, frames); | |||
int samples = ao_read_data_nonblocking(ao, planes, frames, end); | |||
int samples = ao_read_data(ao, planes, frames, end); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not use samples
returned from ao_read_data
, which may be less than requested. The original implementation did not use it and always submit buffers of requested length to CoreAudio. CoreAudio expect the buffer length be equal to requested.
The point is that sleeping 10ms when no samples are returned makes no sense. You can as well just use the blocking variant. |
That's cool. Let me rephrase my question. Could we remove this terrible hack of blocking the audio thread for 10 ms, by all and any means necessary? |
I don't think "sleeping 10ms" and "blocking variant" are the same thing. FWIW, #11955 (comment), #11955 (comment) |
I assumed that the sleep was added after the author noticed that it would spuriously return zero (usually on mutex contention, like you said). If this isn't the case then that doesn't apply ofc. |
The core does not stop AO until EOF. In fact, there is no API for core to tell pull-based AO to "stop requesting data but continue playing enqueued data". Some AO, like pipewire, will starve and spin near EOF; but because pipewire usually has very short device buffer (1024 samples usually), it will not spin too many times. This is not the case for avfoundation, which can be starving for more than 1 second near EOF. |
@sfan5 why you've merged it? I have a ongoing review #13902 (comment) |
I see. Maybe that'd be a good thing to address in the future. Just sleeping in the ao thread feels like a dirty fix.
We decided on IRC that we are okay with merging the revert since OP confirmed that it helps their case. |
it was actually because of me. i didn't see your comment and i felt like reverting to the state before would be reasonable. we were talking about the release and what is blocking it on irc and decided to merge it.
i am still very much interested in a proper solution, if possible. |
0341a6f is a hacky fix for ae908a7 (see #12643 (comment)). Since we've reverted ae908a7, 0341a6f is no longer necessary. Giving there are no issues reported for it, I'm OK with current solution. |
I think we need to revert it. cc @handlerug |
I just confirmed that #13348 still reproduces with |
Yes. We need another revert for 0341a6f. I'm still waiting for response from @handlerug. |
Go ahead, I don’t see any purpose in that commit if we are now back to blocking like before.On 19 Apr 2024, at 10:13, Rui He ***@***.***> wrote:
I just confirmed that #13348 still reproduces with libmpv built from the current master. This PR did not fix it.
Yes. We need another revert for 0341a6f. I'm still waiting response from @handlerug.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Fixes #13880