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

Improve responsiveness when opening many files #827

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estorok
Copy link

@estorok estorok commented Dec 6, 2022

For #826

This commit improves performance when opening or enqueuing
many (150+) files at once.

Celluloid was unresponsive when it attempted to
open many files at once, either as command line
arguments or through the --enqueue option. Additionally,
the timestamp/scrubber repeatedly jumped between the current
video position and 0:00 many times
(until the metadata cache update handler had run for each
video in the playlist).

This is due to too many view updates on the playlist widget.
Calling the playlist handler is a heavy operation that
updates the contents of the playlist widget, clearing
the current playlist model and building another one
based off the (up-to-date) internal playlist, which takes O(n) time.
I expected that the playlist widget should get updated once per
batch file open, but in fact the playlist view was getting
regenerated like this at least two times per enqueued file,
so processing an opened playlist was O(n^2).

The first piece calling playlist updates is the
metadata cache update handler
(metadata_cache_update_handler in celluloid-controller.c).
This handler is run for each new opened file which includes a
playlist view update. By running the playlist view update only once
at the end of the playlist we get a dramatic speedup, and the
timestamp no longer erratically skips between 0:00 and the current time.

The second piece responsible is load_file (celluloid-player.c).
For each loaded file, the old behavior was to notify the playlist
update handler (g_object_notify) if idle. However, the result
appeared to be that when enqueuing many files into an idle
Celluloid instance, the playlist update handler was called for each,
which caused a large slowdown when enqueuing many files.
We can instead defer the playlist update to mpv_property_changed
which appears to be just as functional.
Copy link
Collaborator

@gnome-mpv gnome-mpv left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. From my testing, the patch works fine except for one case where the playlist doesn't get updated properly.

Steps to Reproduce:

  1. Open a file and let it play to the end.
  2. Enqueue a new file by running celluloid --enqueue filename.mkv
  3. Note how the file that was enqueued doesn't appear in the playlist.

@estorok estorok marked this pull request as draft December 6, 2022 06:15
@estorok
Copy link
Author

estorok commented Dec 6, 2022

The problem is indeed that the playlist widget didn't get signaled to update, since the playlist entries eventually appear when becoming non-idle.

Idea 1: The celluloid-player load_file could be given information to know whether it's the last file in a batch. One way would be to add another argument, and the hacky way that avoids changing any interfaces would be to use the 2nd bit of append as another bool.

Idea 2: Notify the player elsewhere that the playlist changed 'g_object_notify(G_OBJECT(player), "playlist")', e.g. at the end of every multi file open, however I'm not sure that the CelluloidPlayer is always exposed where needed. I will try some things to see what works

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.

2 participants