mpv
mpv copied to clipboard
mpv-player
ao: EOF handling for pull-based AO
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 to ao_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 and ao_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 touched ao_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.
Download the artifacts for this pull request:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I've added ao_stop_streaming to handle data after EOF. Now consecutive playback works fine.
@sfan5 I added support for set_pause and EOF handling in ao_audiotrack. I'm sorry that I have no idea how to build mpv for Android. Would you plz test my implementation?
@sunpenghao I'm not familiar with WASAPI and wonder whether ao_wasapi needs EOF handling. Would you plz have a look?
whether
ao_wasapineeds EOF handling
Integrating EOF handling into ao_wasapi might not be that easy. The problem is there in no API to tell WASAPI to "play all remaining data but stop requesting any more". When EOF is caught, we'll need to stop the client at some point in the future to ensure no samples are lost, and only then can we inform the AO to stop streaming. This IMO offsets much if not all the benefit of having EOF handling.
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.
This breaks --loop on ao_pipewire, I can't test ao_audiotrack or ao_avfoundation.
This breaks
--loopon ao_pipewire, I can't test ao_audiotrack or ao_avfoundation.
Weird. It works sometimes and doesn't work in other times. I'm looking into this.
This breaks
--loopon ao_pipewire, I can't test ao_audiotrack or ao_avfoundation.Weird. It works sometimes and doesn't work in other times. I'm looking into this.
@llyyr Fixed in cb7de4d. Could you confirm that?
@sfan5 I wonder if there are any updates. 😃
I'm a bit occupied atm, I'll try to get to it tomorrow.
@sfan5 BTW I wonder why ao_audiotrack is a pull-based AO. It uses a thread to feed data to AudioTrack_write. IMO a push-based AO is more suitable.
To reiterate: We noticed that for pull AOs between EOF and reset or stop, the AO thread would be spinning calling ao_read_data.
Additionally it only applies to AOs that use the return value from ao_read_data. (If you ignore it you will be streaming silence, which means the callback won't immediately be called again.)
Effectively that is avfoundation, pipewire and audiotrack.
What I'm not sure about is how avfoundation and audiotrack (which both use a thread) normally block. AudioTrack_write might block, maybe??
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: Doesn't this situation happen every time you listen a media file to its end? Then if restarting the streaming introduces additional latency, is gapless playback even still possible?
What I'm not sure about is how avfoundation and audiotrack (which both use a thread) normally block.
AudioTrack_writemight block, maybe??
I didn't get you. What's the problem if AudioTrack_write might block?
In ao_avfoundation, enqueueSampleBuffer does not block. It just appends the buffer to its internal buffer. I have tested, and it can accept arbitrary duration of audio data.
I know I suggested this myself but given how fragile the audio code is maybe we shouldn't do this.
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.
Then if restarting the streaming introduces additional latency, is gapless playback even still possible?
I'm not very familiar with ao_audiotrack. In ao_pipewire and ao_avfoundation, it involves merely the de-registrate and re-registrate of the processing callback, which is an instant operation. Giving there are still samples queued inside the internal buffer of pipewire and avfoundation when mpv is restarting playback (I have confirmed that when debugging), it won't introduce additional latency.
@ruihe774 can you split the ao_audiotrack change into a separate PR? We can then consider this ready for merging.
I didn't get you. What's the problem if AudioTrack_write might block?
It isn't. Inversely it would be a problem if it didn't block.
Or does ao_read_data block? One of the functions during normal AO playback must be blocking or waiting, otherwise the thread would be spinning at 100% constantly...
@ruihe774 can you split the ao_audiotrack change into a separate PR? We can then consider this ready for merging.
Ok.
It isn't. Inversely it would be a problem if it didn't block. Or does ao_read_data block? One of the functions during normal AO playback must be blocking or waiting, otherwise the thread would be spinning at 100% constantly...
Giving this, I'm wondering why ao_audiotrack is not a push-based AO. The logic in ao_thread in ao_audiotrack.c looks similar to ao_thread in buffer.c, imo.
Giving this, I'm wondering why >
ao_audiotrackis not a push-based >AO. The logic inao_threadin >ao_audiotrack.clooks similar to >ao_threadinbuffer.c, imo.
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(>﹏<)
each write call returns immediately
I believe it is just the case for AudioTrack_write. It returns immediately.
Then, when resuming, you can almost effortlessly continue writing the buffer from the interrupted position to the audiotrack.
AudioTrack.pause does not flush or discard the queued data.
@ruihe774 3fc8929caf6ba4b89c849a8fcde76841f26d8584 causes playback hangs at random intervals (sometimes it only takes a few seconds until it happens, other times it takes a few minutes), seeking fixes it. It's been happening for llyyr too.
@ruihe774 3fc8929 causes playback hangs at random intervals (sometimes it only takes a few seconds until it happens, other times it takes a few minutes), seeking fixes it. It's been happening for llyyr too.
I'd appreciate it if you could provide your log file with -v -v -v.
There you go, I had to zip it because otherwise it's too big for github.
@llyyr Fixed in cb7de4d. Could you confirm that?
I actually managed to get this again despite that commit, though it's definitely much much rarer. I think it might be worth reverting these for now until we figure out what's going on here since there's also #14237
@llyyr Fixed in cb7de4d. Could you confirm that?
I actually managed to get this again despite that commit, though it's definitely much much rarer. I think it might be worth reverting these for now until we figure out what's going on here since there's also #14237
OK. But only the pipewire part. #14239