The stream selection state wasn't improved. I didn't realize this messed
with caches. All in all, just not a good idea. Back to drawing board I
guess.
This reverts commit f40bbfec4f.
This replaces the previous commit and makes more sense. The internal
demux marked tracks as eager depending on their type and for subtitles
it would always lazily read them unless there happened to be no
available av stream. However, we want the sub stream to be eager if the
player is paused. The existing subtitle is still preserved on the
screen, but if the user changes tracks that's when the problem occurs.
So to handle this case, propagate the mpctx->paused down to the stream
selection logic. This modifies both demuxer_refresh_track and
demuxer_select_track to take that boolean value. A few other parts of
the player use this, but we can just assume false there (no change in
behavior from before) since they should never be related to subtitles.
The core player code is aware of its own state naturally, and can always
pass the appropriate value so go ahead and do so. When we change the
pause state, a refresh seek is done on all existing subtitle tracks to
make sure their eager state is the appropriate value (i.e. so it's not
still set to eager after a pause and a track switch). Slightly invasive
change, but it works with the existing logic instead of going around it
so ultimately it should be a better approach. We can additionally remove
the old force boolean from sub_read_packets since it is no longer
needed.
Actually, I thought of a better way of handling this shortly after
merging this. Revert it and redo it in the next commit.
This reverts commit c2c157ebec.
a323dfae42 almost fixed subtitle tracks
disappearing when paused but it actually missed one part: the behavior
of demux_read_packet_async_until. It's a bit unintuitive, but for
subtitle streams, that function would only return the very first packet
regardless of whatever pts you pass to it. So the previous commit worked
on the very first subtitle, but not actually any of the others (oops).
This is because subtitle streams never marked as eager and thus never
actually read farther ahead. While the video is playing, this is OK, but
if we're paused and switching subtitle tracks then the stream should be
eagerly read. Luckily, the logic is already there in the function for
this. All we have to do add an extra argument to
demux_read_packet_async_until to force the stream to be read eagerly and
then it just works. Be sure to unset the eager flag when we're done.
Actually fixes the bug for real this time.
Several parts of the code need to access options here. There's no point
in hiding it demux.c so just expose it in the demux.h header. This means
pulling it out of demux_internal and putting it in the demuxer struct
instead.
A bit different from the OPT_REPLACED/OPT_REMOVED ones in that the
options still possibly do something but they have a deprecation
message. Most of these are old and have no real usage. The only
potentially controversial ones are the removal of --oaffset and
--ovoffset which were deprecated years ago and seemingly have no real
replacement. There's a cryptic message about --audio-delay but who
knows. The less encoding mode code we have, the better so just chuck
it.
c784820454 introduced a bool option type
as a replacement for the flag type, but didn't actually transition and
remove the flag type because it would have been too much mundane work.
In debug mode the macro causes an assertion failure.
In release mode it works differently and tells the compiler that it can
assume the codepath will never execute. For this reason I was conversative
in replacing it, e.g. in mpv-internal code that exhausts all valid values
of an enum or when a condition is clear from directly preceding code.
Without this change the same track encoded as Opus - which requires R128
tagging - and e.g. Vorbis with ReplayGain tagging have different volumes.
This is caused by ReplayGain 2 having a higher reference level of -18 dB
LUFS, while EBU R128 has a lower reference level of -23 dB LUFS.
For the results of gain application to match, the read EBU R128
values need to be boosted according to the difference in reference
levels.
Patch inspired by mpd's source code.
Buffering ahead nonstop into the cache results in nonstop disk or network
activity to read stream data from wherever it may originate. Currently,
there's no way to configure the demuxer to back off once it's buffered
ahead enough data, since the cache limit will be perpetually not-reached as
a stream continues to play, until the entire stream is eventually buffered.
On a laptop with an i9-12900H with decoding performed by the iGPU,
watching a locally-saved 1080p video which hasn't been buffered into the
page cache consumes approximately 15 W even with caching enabled. When
configuring a hysteresis to make the demuxer back off, power consumption
drops to 9 W when watching the same video, resulting in a whopping 6 W of
power savings.
To make it possible to attain significant power savings via caching, add
a --demuxer-hysteresis-secs option to configure a hysteresis to make the
demuxer back off until there's only the configured number of seconds
remaining in the cache from the current playback position.
This feature is disabled by default.
This seems to work on gcc, clang and mingw as-is, but I made it
conditional on __GNUC__ just in case, even though I can't figure out
which compilers we care about that don't export this define.
Also replace all instances of assert(0) in the code by MP_UNREACHABLE(),
which is a strict improvement.
in->byte_level_seeks field is written and modified inside
update_bytes_read at the same time when demux_get_reader_state
is executing, which locks the demux thread mutex. This results
in a data race, reported by Thread Sanitizer when playing mp3 file
of sufficient long length.
Though, only when the output format is matroska, to avoid muxing errors.
This is quite useful when the input has ASS subtitles, as they tend to
rely on embedded fonts.
dump_cache() calls qsort() to order an array of pointers, while the
comparator forgets it's receiving pointers to pointers.
Since cache-dumping over multiple cache ranges is fairly rare, this
seems to have gone unnoticed.
Unfortunately, attached pictures (from tags etc.) are treated as video
tracks. That meant --sub-create-cc-track added a CC track for them as
well. Stop doing that.
See: #7608
Add an infrastructure for collecting performance-related data, use it in
some places. Add rendering of them to stats.lua.
There were two main goals: minimal impact on the normal code and normal
playback. So all these stats_* function calls either happen only during
initialization, or return immediately if no stats collection is going
on. That's why it does this lazily adding of stats entries etc. (a first
iteration made each stats entry an API thing, instead of just a single
stats_ctx, but I thought that was getting too intrusive in the "normal"
code, even if everything gets worse inside of stats.c).
You could get most of this information from various profilers (including
the extremely primitive --dump-stats thing in mpv), but this makes it
easier to see the most important information at once (at least in
theory), partially because we know best about the context of various
things.
Not very happy with this. It's all pretty primitive and dumb. At this
point I just wanted to get over with it, without necessarily having to
revisit it later, but with having my stupid statistics.
Somehow the code feels terrible. There are a lot of meh decisions in
there that could be better or worse (but mostly could be better), and it
just sucks but it's also trivial and uninteresting and does the job. I
guess I hate programming. It's so tedious and the result is always shit.
Anyway, enjoy.
As an unfortunate disaster, min/max values use the type double, which
causes tons of issues with int64_t types. Anyway, OPT_BYTE_SIZE is often
used as maximum for size_t quantities, which can have a size different
from (u)int64_t.
OPT_BYTE_SIZE still uses in64_t, because in theory, you could use it for
file sizes. (demux.c would for example be capable of caching more than
2GB on 32 bit platforms if a file cache is used. Though for some reason
the accounting code still uses size_t, so that use case is broken. But
still insist that it _could_ be used this way.)
There were various inconsistent attempts to set m_option.max to a value
such that the size_t/int64_t upper limit is not exceeded. Due to the
double max field, this didn't really work correctly. Try to fix this
with the M_MAX_MEM_BYTES constant. It's a good approximation, because on
32 bit it should allow 2GB (untested, also would probably exhaust
address space in practice but whatever), and something "high enough" in
64 bit.
For some reason, clang 11 still warns. But I think this might be a clang
bug, or I'm crazy. The result is correct anyway.
Change all OPT_* macros such that they don't define the entire m_option
initializer, and instead expand only to a part of it, which sets certain
fields. This requires changing almost every option declaration, because
they all use these macros. A declaration now always starts with
{"name", ...
followed by designated initializers only (possibly wrapped in macros).
The OPT_* macros now initialize the .offset and .type fields only,
sometimes also .priv and others.
I think this change makes the option macros less tricky. The old code
had to stuff everything into macro arguments (and attempted to allow
setting arbitrary fields by letting the user pass designated
initializers in the vararg parts). Some of this was made messy due to
C99 and C11 not allowing 0-sized varargs with ',' removal. It's also
possible that this change is pointless, other than cosmetic preferences.
Not too happy about some things. For example, the OPT_CHOICE()
indentation I applied looks a bit ugly.
Much of this change was done with regex search&replace, but some places
required manual editing. In particular, code in "obscure" areas (which I
didn't include in compilation) might be broken now.
In wayland_common.c the author of some option declarations confused the
flags parameter with the default value (though the default value was
also properly set below). I fixed this with this change.
Before this commit, option declarations used M_OPT_MIN/M_OPT_MAX (and
some other identifiers based on these) to signal whether an option had
min/max values. Remove these flags, and make it use a range implicitly
on the condition if min<max is true.
This requires care in all cases when only M_OPT_MIN or M_OPT_MAX were
set (instead of both). Generally, the commit replaces all these
instances with using DBL_MAX/DBL_MIN for the "unset" part of the range.
This also happens to fix some cases where you could pass over-large
values to integer options, which were silently truncated, but now cause
an error.
This commit has some higher potential for regressions.
Change to it 1000 hours, which is "infinite" enough. (Hesitant to use
INFINITY, as that is not in the option's range. The option parser
rejects it because it causes only problems in API users and so on.)
The demuxer cache employs a strange method to make track switching
instant with caching enabled. Normally this would mean you have to wait
until the cache has played out (and you get new packets, including
packets from the newly selected track), or you have to perform a slow
high level seek (decoding video again etc.). The strange method is that
it performs a demuxer-level seek without a high level seek so it looks
like a continuous stream to the decoder, and the newly select stream
gets packets at the current playback position. This is called a refresh
seek.
This works only if some weird heuristics work. It needs a packet "unique
ID", for which it uses either dts or pts. The value must be strictly
monotonic increasing. If this doesn't work, the referesh seek can't be
executed, and the user has to wait until the end of the cache. Sometimes
there are files that simply do not work.
In the present case, there's actually a hack that we can extend. Packets
with unset position are likely generated by the parser, and the hack
which this commit touches simply attempts to make up a new (hopefully
unique) position value, even if the value itself makes no sense. It only
ha to be deterministic.
It turns out libavcodec sometimes output packets with repeating position
values. This commit tries to handle this case too with the same hack.
Fixes: #7498
Preparation for a future commit. The demuxer queues might be read from
other threads than the one to issue the seek, and passing SEEK_BLOCK
with such a seek will provide a convenient way to synchronize this.
In this case the video track has seek_start == seek_end, and due to the
"seek_start >= seek_end" condition, this was considered broken, and no
seek range was created, breaking cached seeking.
Fix this by allowing the case if they're equal, and a valid timestamp.
(NB: seeking backward in this will still jump to position 0, because it
is the video timestamp. This is unfortunately how it's supposed to work.
HR-seeks will also do this, but decode and skip the entire audio until
the seek target, so it will mostly appear to work.)
Remove some redundant fields that controlled or indicated whether the
demuxer was/could/should prefetch. Redefine how the eof/reading fields
work.
The in->eof field is now always valid, instead of weirdly being reset to
false in random situations. The in->reading field now corresponds to
whether the demuxer thread is working at all, and is reset if it stops
doing anything.
Also, I always found it stupid that dequeue_packet() forced the demuxer
thread to retry reading if it was EOF. This makes little sense, but was
probably added for files that are being appended to (running downloads).
It makes no sense, because if the cache really tried to read until file
EOF, it would encounter partial packets and throw errors, so all is lost
anyway. Plus stream_file now handles this better. So stop this behavior,
but add a temporary option that enables the old behavior.
I think checking for ds->eager when enabling prefetching never really
made sense (could be debated, but no, not really). On the other hand,
the change above exposed a missing wakeup in the backward demuxing code.
Some chances of regressions that could make it stuck in certain states
or so, or incorrect demuxer cache state reporting to the player
frontend.
A negative subtitle delay means that subtitles from the future should be
shown earlier. With muxed subtitles, subtitle packets are demuxed along
with audio and video packets. But since they are demuxed "lazily",
nothing guarantees that subtitle packets from the future are available
in time.
Typically, the user-observed effect is that subtitles do not appear at
all (or too late) with large negative --sub-delay values, but that using
--cache might fix this.
Make this behave better. Automatically extend read-ahead to as much as
needed by the subtitles. It seems it's the easiest to pass the subtitle
render timestamp to the demuxer in order to guarantee that everything is
read. This timestamp based approach might be fragile, so disable it if
no negative sub-delay is used.
As far as the player frontend part is concerned, this makes use of the
code path for external subtitles, which are not lazily demuxed, and may
already trigger waiting.
Fixes: #7484
Subtitle tracks are usually "lazy" (ds->eager=false), There are a number
of weird special cases associated with it. One of them is that they have
some sort of "temporary" EOF (to signal that there isn't a packet right
now, and the decoder should not block playback by waiting for more
packets). In a the next commit, I want to call mark_stream_eof() in case
of (some) of these temporary EOFs.
The problem is that mark_stream_eof() also calls the functions touched
by this commit. Basically they shouldn't do any complex work due to
these temporary EOFs (because they might happen very often). It turns
out that lazy tracks barely matter here: they do not extend the seek
range of a packet/EOF happens on them, they do not trigger seek range
joining, and they do not support backward demuxing.
This change should enable the following commit, while not causing any
behavior changes (i.e. bugs) with the current state.
These have ->segmented set (so the codec can be initialized properly),
but have no segment start or end times. This code was (probably) the
only thing which didn't handle this case.
While paused, the decoders typically stop reading data from the demuxer.
But for some reason, the file size is returned as a public field in
struct demuxer (wat...), and updated only when the packet reading
function is called. This caused the file size property to always return
the same value when paused, even though the demuxer thread was reading
new data, and the internal file size was updated.
Fix with a simple hack.
Dudemanguy