Skip to content
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

DOCS/options: ease --prefetch-playlist warning #15378

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

guidocella
Copy link
Contributor

There is no known issue with --prefetch-playlist and file-local-options. It works just fine with them, and people have been using it since 2017 and haven't reported related issues. So ease the warnings to not scare users away from using a useful option.

Also when --prefetch-playlist was implemented it also prefetched the stream cache until 559a400 removed it - see #6753. So if such issues ever existed, they were fixed by not making it prefill the stream cache. Now it doesn't do much, it only opens a connection to the next file, and the cache of the next video starts empty.

There is no known issue with --prefetch-playlist and file-local-options.
It works just fine with them, and people have been using it since 2017
and haven't reported related issues. So ease the warnings to not scare
users away from using a useful option.

Also when --prefetch-playlist was implemented it also prefetched the
stream cache until 559a400 removed it - see mpv-player#6753. So if such issues
ever existed, they were fixed by not making it prefill the stream cache.
Now it doesn't do much, it only opens a connection to the next file, and
the cache of the next video starts empty.
@llyyr
Copy link
Contributor

llyyr commented Nov 25, 2024

Can we reword the description somehow to make it easier to understand? Particularly that it merely opens a demuxer thread for the next file and doesn't actually prefill cache for the next file.

If even contributors like @na-na-hi don't understand what the option actually does, regular users have no hope of understanding the text.

@guidocella
Copy link
Contributor Author

It was much more confusing before and I didn't understand it either, see d4bf179. I explicitly wrote "This does not prefill the cache with the video data of the next URL." at the beginning, so how would you specify it further?

@llyyr
Copy link
Contributor

llyyr commented Nov 25, 2024

Perhaps explaining the use case when you want to use it would help? "This is most useful when reading small files where a significant portion of the load time is spent creating a new demuxer thread" or something to that effect.

file played.
If ``--demuxer`` and related probing options are changed in the time window
between the start of prefetching and the playback of the next file, the old
values are used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it breaks all format specific options and profiles? Kind of defeats the purpose of extension profiles, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's not purpose of extension profiles, they were what was used before implementing conditional profiles and then were deprecated. In conditional profiles this already doesn't work because they are evaluted too late, it seems to work for the first file only if you match by path, and this still works with --prefetch-playlist which doesn't affect the first file.

But yes, setting demuxer options in extension profiles is a reason for not enabling it by default globally. I think it is worth the trade-off for images because you navigate between then much more, and yes the next file could be a video, defaults can't be perfect, but navigating between images faster should be preferable for the majority of users. A note about it can be added in the image profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I will look into discarding prefetched data if demuxer options changed, it already does it stream-open-filename changed, so it should be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants