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

demux: make hysteresis-secs respect cache-secs #12702

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

N-R-K
Copy link
Contributor

@N-R-K N-R-K commented Oct 21, 2023

currently hyst_active is only reactivated when the memory limit fills up. but we might stop prefetching due to hitting cache-secs as well, which might occur well before hitting the memory limit. and in those cases, hyst_active never gets reactivated and thus is broken.

this patch activates hysteresis whenever there's nothing more to read or prefetch.

@github-actions
Copy link

github-actions bot commented Oct 21, 2023

Download the artifacts for this pull request:

Windows

@Jules-A
Copy link

Jules-A commented Oct 21, 2023

Need a hysteresis percentage that will cache to a percentage of demuxer-max-bytes or caches-sec, whichever is greater. OR at least a demuxer-hysteresis-bytes as it's hard to get it useful for both high bitrate and low bitrate in it's current state.

@N-R-K
Copy link
Contributor Author

N-R-K commented Oct 21, 2023

currently hyst_active is only reactivated when the memory limit fills up

Reading the docs, seems like this was intended. But this isn't intuitive/doesn't make sense since if the goal is to save power by prefetching in bursts then it make sense to do it when cache-secs fills up before max-bytes as well.

@N-R-K N-R-K changed the title demux: fix hysteresis-secs not respecting cache-secs demux: make hysteresis-secs respect cache-secs Oct 21, 2023
@N-R-K
Copy link
Contributor Author

N-R-K commented Oct 21, 2023

Reading the docs, seems like this was intended.

But then the docs mention cache-secs and demuxer-readahead-secs as well. That's why I was expecting it to work in the first place, and was surprised to see it still buffer non-stop.

@kerneltoast Since you were the one who added this flag, do you have any comment on all of this?

@N-R-K N-R-K force-pushed the hysteresis-fix branch 2 times, most recently from 4073c5c to 418ce26 Compare October 21, 2023 18:43
@N-R-K
Copy link
Contributor Author

N-R-K commented Oct 23, 2023

OR at least a demuxer-hysteresis-bytes

This was lightly discussed on the original PR.
Initially I also thought specifying bytes might be better but after trying it out I think this option makes more sense in time than bytes.

@Jules-A
Copy link

Jules-A commented Oct 23, 2023

OR at least a demuxer-hysteresis-bytes

This was lightly discussed on the original PR. Initially I also thought specifying bytes might be better but after trying it out I think this option makes more sense in time than bytes.

I wasn't talking out 1 or the other but the option to specify both (if you ignore the % idea).

@N-R-K N-R-K force-pushed the hysteresis-fix branch 2 times, most recently from 3a25b1d to f64f7bf Compare October 28, 2023 18:49
Copy link
Contributor

@kasper93 kasper93 left a comment

Choose a reason for hiding this comment

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

if the goal is to save power by prefetching in bursts then it make sense to do it when cache-secs fills up before max-bytes as well.

Exactly my thoughts, this bothered me too for some time.

Change LGTM and works as advertised. 👍

Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

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

Needs a rebase but seems fine.

N-R-K added 2 commits October 30, 2023 22:53
- brace goes to next line for multi-line conditional
- sort standard headers some more
currently hysteresis-secs only works when the demuxer-max-bytes fills
up. but it's possible for the cache-secs/demuxer-readahead-secs to be
reached first.

in those cases, hysteresis-secs doesn't work and keeps buffering
non-stop. but the goal of this option was to save power by avoiding
non-stop buffering so go ahead and make it respect cache-secs as well.

additionally remove some redundant repetition from the docs.
@Dudemanguy Dudemanguy merged commit 3789f1a into mpv-player:master Oct 30, 2023
14 checks passed
@N-R-K N-R-K deleted the hysteresis-fix branch October 30, 2023 17:32
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.

4 participants