Fix audio format negotiation #5716
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevmitch
reviewed
Apr 7, 2018
DOCS/man/options.rst
Outdated
|
|
||
| ``--alsa-buffer-time=<microseconds>`` | ||
| Set the requested buffer time in microseconds. A value of 0 skips requesting | ||
| anything from the ALSA API. This and the period count option uses the ALSA |
Member
There was a problem hiding this comment.
should refer explicitly to alsa-periods
|
Trying to sneak in buffer reduction here as well, huh? The original issues still stand - 25ms is too small to ensure no buffer underruns will happen, increases CPU and is a hack to solve a problem that shouldn't have existed in the first place. |
Author
No, it's part of another PR, and I was too lazy to switch branches, Mr. Sherlock. Also it's not 25ms anymore. |
ghost
mentioned this pull request
added 16 commits
April 15, 2018 23:11
There is a dedicated thread for feeding audio to the ALSA API from a buffer with a larger size. There is little reason to have such a large device buffer.
Same deal as with the previous commit for ALSA. Untested.
This tried to avoid running the audio/video functions depending on whether any of the audio or video related format restrictions were called (so the filter would show an error if a mismatching media type was passed in). It was a shit idea anyway, so fuck it.
The audio format neogitation code was pretty complicated, although the idea was simple: when the format changes (or on the first audio frame), filter only the new frame through the entire filter chain, discard the resulting frame, but use the format to initialize the AO. This was useful for "fudging" the channel remix behavior (upmix or downmix), and moving it before other filters. Apparently this was useful for things like DRC filters, which might work better in stereo, and which also can only achieve the desired volume levels by doing it before a downmix, which would modify the volume. This mechanism was introduced in commit 60048b7 (which the commit message also describes as "idiotic heuristic"). Knowing the output format is inherently necessary for this, because otherwise we can't know what the hell the user defined filters will do. There were problems with robustness. Some filters needed more than one frame. Resampling in particular would discard initial audio at high resampling ratios. Some filters might drop audio intentionally (like clipping data on timestamp ranges). There were also allegations that some decoders output 0 length frames (although that is invalid in libavcodec). The state machine was excessively complex and hard to understand too. There are 3 things that could have been done: 1. Fix robustness problems by doing more heuristics, like repeating audio frames or simply decoding several frames. Since filters can behave differently, this would have added lots of complexity. 2. Make use of libavfilter's format negotiation, and add the same to mpv builtin filters. This is sort of annoying, because the format negotiation in libavfilter changes the state of the filters. It also reports only some parameters (mostly all for audio, but a lot of holes for video). It would remove some of the state machine, but not all. 3. Drop the channel remix fudging, and do the same as the video chain. This would not require format negotiation, but instead you can just filter the audio frames, and look what comes out of it. If nothing comes out, simply never create an AO. This commit selects option 3. It removes the remix fudging, which means the loss of a feature. Users can instead add "--af=format=channels=2" before their DRC filter, or something. I'm also considering changing the default for --audio-channels back to stereo, and downmix in the decoder or at the start of the filter chain, which would give the same results, except requiring more configuration. Implementation-wise, this is still a bit different from the video path. The VO always remains the same instance, while the AO might have to be recreated on configuration changes. This still requires explicit format change handling + draining old data, but by putting it into f_autoconvert, not much new code is needed.
Print them as a warning. Note that there may be some cases where it underruns, without being a bad condition. This could possibly happen e.g. if the last chunk is written, and then it resumes playback some time after that. Eventually I want to add more code to avoid such spurious warnings.
Until recently, the AO was reinitialized strictly only on decoder format changes. But the commit for simplifying audio format negotiation removed this. Now the AO is recreated for any format change. This is sort of annoying if you change playback speed. The insertion/removal of af_scaletempo can change the sample format. For example, the acompressor filter will convert output to double, so toggling scaletempo will force the format back to float. This recreates the AO under the --gapless-audio=weak default. This likely affects a lot of other filters too. Work this around by allowing sample format changes, and keeping the current AO format in these cases. This is probably not a big problem. Most audio APIs force the output format to float anyway. This means you actually have to worry about what the default gapless mode does to your audio. If you start with a file that uses 8 bit per sample, and then continue playing a 24 bit FLAC, it will be converted down to 8 bit per sample. (Assuming they are played in a way that uses the gapless logic.)
Normally we don't even try this, but in corner cases it can happen. For example when inserting lavcac3enc at runtime, and display-sync-resample was active.
The first change is about spdif - I mostly ignore spdif issues these days, but it seems like the recent changes made handling of it slightly better (but I didn't really test). The second change is about broken libavfilter filters. We won't restore the old behavior, because people were complaining about the old behavior in the past. Possibly we could make libavfilter export this was metadata and use the old behavior if we know they're broken - but it doesn't exist yet.
The fix-pts option basically uses the old af_lavfi's (before filter rewrite) timestamp logic. The rest is explained in the manpage.
Fundamentally, scripts are loaded asynchronously, but as a feature,
there was code to wait until a script is loaded (for a certain arbitrary
definition of "loaded"). This was done in scripting.c with the
wait_loaded() function.
This called mp_idle(), and since there are commands to load/unload
scripts, it meant the player core loop could be entered recursively. I
think this is a major complication and has some problems. For example,
if you had a script that does 'os.execute("sleep inf")', then every time
you ran a command to load an instance of the script would add a new
stack frame of mp_idle(). This would lead to some sort of reentrancy
horror that is hard to debug. Also misc/dispatch.c contains a somewhat
tricky mess to support such recursive invocations. There were also some
bugs due to this and due to unforeseen interactions with other messes.
This scripting stuff was the only thing making use of that reentrancy,
and future commands that have "logical" waiting for something should be
implemented differently. So get rid of it.
Change the code to wait only in the player initialization phase: the
only place where it really has to wait is before playback is started,
because scripts might want to set options or hooks that interact with
playback initialization. Unloading of builtin scripts (can happen with
e.g. "set osc no") is left asynchronous; the unloading wasn't too robust
anyway, and this change won't make a difference if someone is trying to
break it intentionally. Note that this is not in mp_initialize(),
because mpv_initialize() uses this by locking the core, which would have
the same problem.
In the future, commands which logically wait should use different
mechanisms. Originally I thought the current approach (that is removed
with this commit) should be used, but it's too much of a mess and can't
even be used in some cases. Examples are:
- "loadfile" should be made blocking (needs to run the normal player
code and manually unblock the thread issuing the command)
- "add-sub" should not freeze the player until the URL is opened (needs
to run opening on a separate thread)
Possibly the current scripting behavior could be restored once new
mechanisms exist, and if it turns out that anyone needs it.
With this commit there should be no further instances of recursive
playloop invocations (other than the case in the following commit),
since all mp_idle()/mp_wait_events() calls are done strictly from the
main thread (and not commands/properties or libmpv client API that
"lock" the main thread).
Recursive invocation was needed up until the previous commit. Drop this feature, and simplify the code. It's more logical, and easier to detect miuses of the API. This partially reverts commit 3878a59. The original reason for it was removed.
(Not sure if worth the trouble, but it does seem less awkward.)
Normally, MPV_RENDER_PARAM* arguments are copied, unless documented otherwise. Of course we can't copy X11 Display or Wayland wl_display types, but for arguments that are "summarized" in a struct (like MPV_RENDER_PARAM_OPENGL_FBO), a copy is expected. Also add some unused infrastructure to make this explicit, and to make it easier to add parameter types that require a copy. Untested.
Although this was never observed to be happening, it seems definitely possible: we first tell the main thread to exit, and then we ask the main thread to do some work for us (with mp_dispatch_run()). Obviously this is racy, and the main thread could exit without doing this, which would block mp_dispatch_run() forever. Fix this by changing the order of operation, so that it makes sense. We could also just store the pthread_t of the main thread in some variable, but the fact that pthread_create() might set the pthread_t argument _after_ starting the thread makes this a bit messy (at least it doesn't seem to be guaranteed on a superficial look at the manpage).
Member
|
Some of this PR has already been pushed in, but I have now finished my rounds of testing with the rest:
Thus, unless I will find any issues with it, I will be pushing the contents of this branch (pr5716 without the scripting/dispatch changes), and after it is found out which side borks mpv-android (quite likely mpv-android itself, but we'll have to check) the rest can be handled as well. |
added 2 commits
April 16, 2018 17:43
I changed avio_flush() and introduced avformat_flush() exactly for this reason. Used with DVD/BD only (on seeks and when setting the "angle" property). Seems to work, but wasn't tested too thoroughly (I don't care about optical discs, I only want this ugly stuff gone that might even violate the API/ABI).
This was slightly broken: since mp_initialize() did not necessarily interrupt core_thread() (which is waiting for initialization), it did not enter mp_play_files(), which would have sent an IDLE event. I suppose that in some cases (like with mpv-android), the initial IDLE event was never actually sent, because the first wakeup of the core thread happens with the "loadfile" command, which will disallow the core thread an IDLE event.
Member
|
OK, the mpv-android issues were due to it depending on not getting the IDLE event at start (which was an unfortunate misfeature that happened to work during the last year or two). The new commits look OK, cleaner lavf flushing and making sure libmpv always sends IDLE messages at start. |
Member
|
Aand my usual luck. I had only tested mingw-w64 without the dispatch/script changes. They seem to break mpv.exe, so merging the rest of it and closing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first 3 are some old commits.
The second commit of the other 3 commits contains the interesting changes.