Ended up being too flawed and caused trouble in other areas. There's
other approaches to trying to solve the issue this meant to address in
the works that should be better, so let's wait on that. Fixes#13613 and
fixes#13622.
This reverts commit e3af545421.
This seems more robust than relying on the audio status to actually be
playing. For files where there is no audio or the audio start is
delayed, this guards against that but it allows the subtraction to
always occur otherwise on normal files with audio.
Playback speed changes should be treated as a discontinuity just like
seeking. Previously, this was being treated internally as just plain
normal playback, but that can't really work. The frame timings from
before the speed change and after the speed change are completely
different and shouldn't be compared to each other. This lead to frames
being adjusted to weird places and possibly even being skipped (as if
mpv was seeking) on speed changes. What we should do is clear out and
reset all av related fields like what happens when you seek, but it is
not quite as aggressive. No need to do a full video state reset or such.
We also wait an arbitrary amount of frames before adjusting for av sync
again. compute_audio_drift already used a magic number of 10 which
sounds reasonable enough so define that and use it here. Fixes#13513.
When calculating the audio pts, mpv multiplies the ao delay by the
current audio speed and subtracts it from the written audio pts. This
doesn't really make sense though. mpctx->video_pts is never affected by
the playback speed, and this leads to weird behavior like the audio-pts
property changing values while paused merely because the playback speed
changes. Remove the multiplication and simply subtract the delay by a
factor of 1 instead. When updating the av_diff in player/video, this
does actually need to take in account the audio speed so we do the
calculation there.
32-bit signed integer can hold ~2.1s stored as nanoseconds. While frame
duration doesn't make sense to be this long, the existing clamp is to
10s. Change type to double, which is consistent with other fields in
vo_frame.
This workarounds the issue with vo_gpu that would freeze in certain
condition. I haven't go deep inside to understand why, so this should be
considered as a workaround.
Fixes: #13256
This reverts commit cb2b579f61.
This breaks files where the audio starts much later after the video
since mpv reads the first audio packet even if it isn't supposed to
start yet, resulting in the audio_status being STATUS_READY for the
entire time mpv is "waiting" for the first audio packet to be played.
During move of this code from vo_gpu_next.c to video.c someone(TM) tried
to be smart and simplify the expression. The num_vsync includes error
compensation which can cause it to display +-1 vsync at the same rate.
We explicitly don't want to include this in "ideal" parameters.
Also num_vsyncs was already rounded so we produced off timings in
general.
Revert back to proper way of translating the time.
Fixes: 5e5a325
Relative to frame PTS timeline as oposed to display vblank.
Those values are relative to unadjusted video timeline. They will be
used by gpu-next where it expect virtual frame vsync, not display vblank
time.
Currently VOCTRL are completely unusable for frequent data query. Since
the HDR parameter addition to video-params, the parameters can change
each frame. In which case observe on those parameter would be triggered
constantly. The problem is that quering those parameters involves VOCTRL
which in turn involves whole render cycle of delay.
Instead update VO params on each draw_frame. This requires changes to VO
reconfiguration condition, but in practice it should only be triggered
when image size or data layout changes. In other cases it will be
handled internal by VO driver.
I'm not quite happy with this solution, but don't see better one without
changing observe/notify logic significantly. There is no good way
currently to handle VOCTRL that are constantly queried.
This adds unfortunate synchronization of player command with VO thread,
but there is not way around that and if too frequent queries of this
param becomes a problem we can thing of other solutions.
Changes the way to get data from VO driver added by a98c5328dc
Fixes: 84de84bFixes: #12825
This would cause mpv to, in some very specific scenarios, have a
negative vsync_offset after seeking which would result in mpv requesting
a pts before the first frame to libplacebo.
Fix it by setting it to 0 when we reset state, such as after seeking.
Fixes: https://github.com/mpv-player/mpv/issues/12813
Previously, the av sync change calculation was only done if the
audio_status was STATUS_PLAYING, but there is at least one or two more
states where this should be done. player/audio is capable of adding
delay if the state is anything besides STATUS_EOF. This means that while
calling adjust_sync, the delay value could have changed from the audio
side of the equation from the previous playloop, and it doesn't
necessarily mean that the current audio_status is STATUS_PLAYING either.
So the old code would technically skip this case. In practice, this is
just one frame so it hardly matters, but it should be taken into
account. For example, STATUS_READY is definitely possible here in
adjust_sync. I'm not sure if it's actually possible for STATUS_SYNCING
to happen but the audio code can change add delay with that status, so
it doesn't hurt. STATUS_DRAINING is probably not relevant, but again
include it to mirror the audio code logic. Of course, STATUS_EOF is
obviously a no-no since that means no audio at all, so we return from
there. I didn't take hard measurements or anything, but this does seem
to result in slightly smaller av sync fluctuations on discontinuities
(like seeking) which makes sense because we're now making an additional
correction that wasn't previously done.
Another change is to always try adjust_sync as long as the frame_time is
nonzero. The old logic only did this if the video_status was playing or
greater, but it is possible to get new frames with a different PTS that
do not have that status. The audio is still playing so logically it
should be adjusted as well. Again, this happens for just one frame, so
it doesn't really matter in practice but it should make more sense. A
zero frame_time is skipped since that would mean the pts did not advance
and the previous playloop should have done the adjustment for that time
already.
3038e578af recently changed the logic here
so it wouldn't trigger on still images, but after thinking about the
code here some more, I don't believe it's needed at all. Doing an ao
reset when you flip the video track is very disruptive and not really
desirable at all. Since the ao no longer adds bogus delay values while
the video is off, there should be no need to do a full reset for syncing
reasons. The delay value will be zero, so we can let the audio just play
normally and let the video code do its thing. There is one slight trick
here however. When using a display sync mode, part of the syncing code
will try to update the audio and video playback speed. This can cause an
audio underrun if we're just turning the video back one. An easy way to
avoid most of these is to not update the speed if we are in the
STATUS_SYNCING state for video. This isn't quite perfect and underruns
are still possible, but it actually seems to depend on the AO. e.g. I
couldn't trigger an underrun with alsa but I could with pipewire. In any
case, the audio artifact here is much less noticeable than doing a full
ao reset, so it's still an improvement.
The point of the mpctx->delay field is for calculating a/v sync and
adjusting the frame timings appropriately. However, the frame_time was
always subtracted from mpctx->delay regardless of the audio status. This
meant that for a video with no audio, every single frame had a
subtraction with nothing ever added to it meaning that you get massive
negative delay times. For weird videos where the audio starts way later,
the massive delay leads to the VO sleeping for basically about as long
as the video was previously playing without audio. This results in
nothing being rendered during that brief period of time and just overall
badness. When using display-sync, it happens to work since the video
doesn't adjust itself based on audio and it renders anyway.
The fix is to simply not touch mpctx->delay in player/video.c unless
there's actually audio playing. This is what the rest of the code
already does aside from setting it to 0 on resets or EOFs. Move the
calculation into adjust_sync after the audio status check. It works
exactly the same as before except that we don't constantly subtract
bogus values when there's no audio playing. The reverse situation in
player/audio.c also has the same issue. For something that is only
audio, mpctx->delay is always added to but nothing will ever subtract
from it. It's not really clear if this particular version could ever
cause a real bug, but logically it needs to be guarded in the same way.
The field here should only be updated if the video is actually playing.
Fixes#12025.
vo_still_displaying() is racey with vo_request_wakeup_on_done() and above
that it doesn't work as expected. VO can run out of work and go to sleep
for 1000s, while the play thread still returns on vo_still_displaying()
check, because of a check `now < frame_end` so it never advances and go
to sleep itself.
This fixes dead lock that we have when image parameters changes during
playback.
This reverts commit 0c9ac5835b.
Fixes: #12575
Setting `--video-crop=0x0+0+0` applies full frame crop, ignoring the
container one. Setting --video-crop=0 disables manual crop and restores
container one if it is available.
When playing a sparse video stream, the debug log gets hit with the
video EOF constantly since the audio is still playing. There's no
practical use for this so just do add some logic to only signal it once
if it is sparse.
The track during lavfi-complex can actually be NULL which meant that
ca4192e2df regressed lavfi-complex by
causing mpv to crash during runtime changes of the filter. Additionally,
it's possible for the decoder wrapper to also be NULL. check_framedrop
within write_video checks this, but check_for_hwdec_fallback does not.
Perhaps, it's impossible for this to happen, but we might as well add
the check here to be on the safe side since mp_decoder_wrapper_control
is not designed to handle a NULL.
It's a bit of an edge case, but since we now allow the disabling of the
software fallback it's possible to have a situation where hwdec
completely fails and the mpv window is still lingering from the previous
item in the playlist. What needs to happen is simply that the vo_chain
should uninit itself and handle force_window if needed. In order to do
that, a new VDCTRL is added that checks vd_lavc if force_eof was set.
player/video will then start the uninit process if needed after getting
this.
So far there was no way to sync video to display and have audio sync to
video without changes in pitch.
With this option the audio does not get resampled (pitch change) and
instead the corrected audio speed is applied to audio filters.
Instead of choosing based on smallest deviation from set speed,
use the speed change from the smallest factor that does not
exceed `video-sync-max-video-change`.
mpv's core already keeps track of whether or not it thinks a track is an
image. Some VOs (i.e. wayland) would benefit from knowing if what is
currently being displayed is an image or not so add a new VOCTRL that
signals this anytime we load a new file with a VO. Additionally, let's
add a helper enum for signaling the kind of content that is being
displayed. There is now MP_CONTENT_NONE (strictly for force window being
used on a track with no image/video), MP_CONTENT_IMAGE, and
MP_CONTENT_VIDEO. See the next commit for the actual usage of this (with
wayland).
The video sync logic for mpv lies completely within its core at
essentially the highest layer of abstraction. The problem with this is
that it is impossible for VOs to know what video sync mode mpv is
currently using since it has no access to the opts. Because different
video sync modes completely changes how mpv's render loop operates, it's
reasonable that a VO may want to change how it renders based on the
current mode (see the next commit for an example).
Let's just move the video sync option to mp_vo_opts. MPContext, of
course, can still access the value of the option so it only requires
minor changes in player/video.c. Additionally, move the VS_IS_DISP
define from to player/core.h to common/common.h. All VOs already have
access to common/common.h, and there's no need for them to gain access
to everything that's in player/core.h.
When changing video track with subtitles with unknown duration, they
aren't shown until you seek, cycle sub back and forth, or apply a video
filter. This is caused by reinit_video_chain_src() calling
reset_subtitle_state() -> sub/sd_ass.c:reset() -> ass_flush_events()
when ctx->duration_unknown is true.
The ass_flush_events() call was added in a714f8e so subs with unknown
duration wouldn't multiply on seek, i.e. when reset_subtitle_state() is
called from reset_playback_state(). So keep calling it from there, and
in reinit_video_chain_src() use just term_osd_set_subs(mpctx, NULL)
instead to clear any subtitles printed to the terminal. The
reset_subtitle_state() call was added in c1ac97b to "reset the state
correctly when switching between video/no-video", but with it removed I
no longer notice any issue doing that.
Essentially, this lets video.c decide whether to consider a video track
cover art, instead of having the decoder wrapper use the lower level
sh_stream flag.
Some pain because of the dumb threading shit. Moving the code further
down to make some of it part of the lock should not change behavior,
although it could (framedrop nonsense).
This commit should not change actual behavior, and is only preparation
for the following commit.
I don't see the point of this. Not doing it may defer an error to later.
That's OK? For now, it seems better to reduce the encoding internal API.
If someone can demonstrate that this is needed, I might reimplement it
in a different way.
This replaces the two buffers (ao_chain.ao_buffer in the core, and
buffer_state.buffers in the AO) with a single queue. Instead of having a
byte based buffer, the queue is simply a list of audio frames, as output
by the decoder. This should make dataflow simpler and reduce copying.
It also attempts to simplify fill_audio_out_buffers(), the function I
always hated most, because it's full of subtle and buggy logic.
Unfortunately, I got assaulted by corner cases, dumb features (attempt
at seamless looping, really?), and other crap, so it got pretty
complicated again. fill_audio_out_buffers() is still full of subtle and
buggy logic. Maybe it got worse. On the other hand, maybe there really
is some progress. Who knows.
Originally, the data flow parts was meant to be in f_output_chain, but
due to tricky interactions with the playloop code, it's now in the dummy
filter in audio.c.
At least this improves the way the audio PTS is passed to the encoder in
encoding mode. Now it attempts to pass frames directly, along with the
pts, which should minimize timestamp problems. But to be honest, encoder
mode is one big kludge that shouldn't exist in this way.
This commit should be considered pre-alpha code. There are lots of bugs
still hiding.
This mode drops or repeats audio data to adapt to video speed, instead
of resampling it or such. It was added to deal with SPDIF. The
implementation was part of fill_audio_out_buffers() - the entire
function is something whose complexity exploded in my face, and which I
want to clean up, and this is hopefully a first step.
Put it in a filter, and mess with the shitty glue code. It's all sort of
roundabout and illogical, but that can be rectified later. The important
part is that it works much like the resample or scaletempo filters.
For PCM audio, this does not work on samples anymore. This makes it much
worse. But for PCM you can use saner mechanisms that sound better. Also,
something about PTS tracking is wrong. But not wasting more time on
this.
Can be useful to force it to adapt to extreme speed changes, while a
higher limit would just use a fraction closer to the original video
speed.
Probably useful for testing only.
The wakeup at the end of VO frame rendering seems redundant, because
after rendering almost no state changes. The player core can queue a new
frame once frame rendering begins, and there's a separate wakeup for
this. The only thing that actually changes is in->rendering. The only
thing that seems to depend on it and can trigger a wakeup is the
vo_still_displaying() function. Change it so that it needs an explicit
call to a new API function, so we can avoid wakeups in the common case.
The vo_still_displaying() code is mostly just moved around due to
locking and for avoiding forward declarations.
Also a somewhat risky change (tasty new bugs).
llyyr
Kacper Michajłow
Dudemanguy
llyyr
Harri Nieminen
Thomas Weißschuh
StratusFearMe21
Christoph Heinrich
sfan5
Guido Cella
wm4