-
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
ao: EOF handling for pull-based AO #13967
Conversation
Download the artifacts for this pull request: |
I've added |
@sfan5 I added support for @sunpenghao I'm not familiar with WASAPI and wonder whether |
Integrating EOF handling into Also, I'm not sure I'm grasping the full context here. Is EOF handling needed so that the AO doesn't spin around EOF? If so, WASAPI doesn't have this issue in the first place because it pulls data at a stable interval (the device scheduling period, usually ~10ms) and each time requests a fixed size. Padding silence after EOF should suffice. |
@sfan5 I wonder if there are any updates. 😃 |
This breaks |
Weird. It works sometimes and doesn't work in other times. I'm looking into this. |
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.
LGTM for ao_pipewire.
I'm a bit occupied atm, I'll try to get to it tomorrow. |
@sfan5 BTW I wonder why |
audio/out/ao_audiotrack.c
Outdated
} | ||
|
||
mp_cond_timedwait(&p->wakeup, &p->lock, MP_TIME_MS_TO_NS(300)); |
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.
wouldn't that mean it sleeps after every write?
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.
My bad. Fixed in 5e8ef9d.
audio/out/ao_audiotrack.c
Outdated
|
||
JNIEnv *env = MP_JNI_GET_ENV(ao); | ||
if (paused) { | ||
MP_JNI_CALL_VOID(p->audiotrack, AudioTrack.pause); |
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.
thought I'd have to verify: since the feed thread and not synchronized could this cause audio to be paused before the intended point (cut off)?
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'd expect ao->driver->set_pause
to perform an immediate pause, just like what we do in the push-based AO.
To reiterate: We noticed that for pull AOs between EOF and What I'm not sure about is how avfoundation and audiotrack (which both use a thread) normally block. I know I suggested this myself but given how fragile the audio code is maybe we shouldn't do this. (also since the suboptimal behavior is very minor) Another thought: |
I didn't get you. What's the problem if In
I did it carefully. BTW I'm also making other improvements to the AO codebase (e.g. #14058). IMO I don't consider the audio code a frozen codebase.
I'm not very familiar with |
@ruihe774 can you split the ao_audiotrack change into a separate PR?
It isn't. Inversely it would be a problem if it didn't block. |
Ok.
Giving this, I'm wondering why |
When the stream is draining, setting stream to active has no effect.
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.
Tested and verified with ao_pipewire
: without the EOF handling on_process
callback runs 7 times more without any data to queue
The pull mode has its advantages, such as when pausing, a buffer is not completely written to the audiotrack, for example, only half of it is written. Then, when resuming, you can almost effortlessly continue writing the buffer from the interrupted position to the audiotrack. If you use the push mode, then every time you pause and resume, you have to consider how to handle the unwritten buffer, and you need to ensure that each write call returns immediately because there is no separate thread to maintain the state, which is really annoying. I just wrote a gst audiotrack-based audiosink(>﹏<) |
I believe it is just the case for
|
There you go, I had to zip it because otherwise it's too big for github. |
As we've previously discussed in #13902, a mechanism for pull-based AO to know EOF is needed. This PR addresses this by adding an
eof
out param toao_read_data
. The AO can know whether EOF has been reached by checking its value and do proper EOF handling like stop requesting more data.Meanwhile, to reduce code duplication, this PR merges
ao_read_data
andao_read_data_nonblocking
into a unified interface.ao_read_data
now accepts three more arguments:bool *eof, bool pad_silence, bool blocking
. I have not touchedao_read_data_converted
yet; maybe it can also be unified.This is a draft: I have only implemented EOF handling in
ao_avfoundation
. Work need to be done for other AOs. Also, proper handling of audio frames after EOF is WIP.