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

sd_ass: introduce sub-ass-prune-delay #15229

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

llyyr
Copy link
Contributor

@llyyr llyyr commented Oct 30, 2024

No description provided.

@llyyr llyyr force-pushed the libass-pruning branch 3 times, most recently from 83379cc to 836371b Compare October 30, 2024 09:42
@llyyr llyyr marked this pull request as ready for review October 30, 2024 09:44
@sfan5
Copy link
Member

sfan5 commented Oct 30, 2024

do we really have to make this configurable?
if you seek events will be re-read anyway so something like 10s should be guaranteed safe

@kasper93
Copy link
Contributor

Why is this even configurable on libass side? Why can't they manage their internal memory cache?

@llyyr
Copy link
Contributor Author

llyyr commented Oct 30, 2024

Why is this even configurable on libass side? Why can't they manage their internal memory cache?

This is a new API, the use case is playback of a video file made solely out of ASS vector-art (>1TiB file in total). libass currently keeps all events in memory until ass_flush_events or ass_free_track is called. libass/libass#747

do we really have to make this configurable?

Setting it to a really small number should be fine too, but there may be use cases where you want it to be 0.

@llyyr llyyr changed the title sd_ass: introduce sub-ass-prune-delay sd_ass: enable automatic pruning of older events from memory Oct 30, 2024
@llyyr
Copy link
Contributor Author

llyyr commented Oct 30, 2024

Changed it to just unconditionally enable pruning if the API is available. I tested it briefly and nothing seems broken by it

Scratch that, this breaks if a subtitle needs to be re-rendered because the user switched play-dir during runtime. I don't think we can enable this at all then without re-decoding events in the event of play-dir, but how much do we care about reverse playback anyway?

@llyyr llyyr changed the title sd_ass: enable automatic pruning of older events from memory sd_ass: introduce sub-ass-prune-delay Oct 30, 2024
@cubicibo
Copy link

cubicibo commented Oct 30, 2024

Why is this even configurable on libass side? Why can't they manage their internal memory cache?

libass cannot know the user intention and use-case.

  • User may parse an entire script once then perform random seek forward and backward: libass must not do anything.
  • User may only render content incrementally: libass can optionally discard expired events.
  • User can playback content where subtitle and video packets are multiplexed. Relying on libass cache can be faster than flushing everything and re-parsing.
  • User can watch a continuous stream with closed captions (e.g. Twitch): pruning must be performed to prevent a (long-term) memory leak.

This should probably be enabled by default with a reasonable time window for ASS tracks multiplexed with AV content, like MKVs, or with livestreams that include closed captions rendered by libass.

Copy link

github-actions bot commented Oct 30, 2024

Download the artifacts for this pull request:

Windows
macOS

@sfan5
Copy link
Member

sfan5 commented Oct 30, 2024

I tested it briefly and nothing seems broken by it

Wouldn't it also break sub-seek if the previous event is older than prune-delay?

@llyyr
Copy link
Contributor Author

llyyr commented Oct 30, 2024

Wouldn't it also break sub-seek if the previous event is older than prune-delay?

Yes, so sub-seek and switching to play-dir=- during runtime is broken if we enable it unconditionally

@llyyr llyyr force-pushed the libass-pruning branch 2 times, most recently from 4682ece to dd856bf Compare October 30, 2024 12:59
@Samillion
Copy link
Contributor

This would solve #9014 right?

@llyyr
Copy link
Contributor Author

llyyr commented Oct 30, 2024

This would solve #9014 right?

No, this will only apply to the subtitle track, not the OSD. I don't believe this is necessary or even warranted for the OSD since as avih said in that issue: Sure, with OSD/OSC it uses more ram (and grows quicker initially), because libass has a big cache. However, that should eventually stop growing

It shouldn't be growing infinitely in the first place, so if it does that's a whole another issue.

@Samillion
Copy link
Contributor

Noted. Thank you for the response.

sub/sd_ass.c Outdated Show resolved Hide resolved
@kasper93
Copy link
Contributor

Have anyone tested it?

@llyyr
Copy link
Contributor Author

llyyr commented Oct 31, 2024

Have anyone tested it?

Yes, I tested it. Works as expected, I listed the things it breaks in the docs that's why I don't think we can enable it by default unfortunately

@llyyr llyyr force-pushed the libass-pruning branch 2 times, most recently from 1d95b17 to caca230 Compare November 3, 2024 04:07
Disabled by default because it breaks sub-seek and playback in cases
where the user changes play-dir from + to - during runtime and past
"seen" events need to be re-rendered.

Available since libass/libass@dcc9eb7
@kasper93 kasper93 merged commit 2211673 into mpv-player:master Nov 3, 2024
25 checks passed
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.

5 participants