Skip to content

Improve responsiveness when opening many files#827

Draft
estorok wants to merge 1 commit intocelluloid-player:masterfrom
estorok:master
Draft

Improve responsiveness when opening many files#827
estorok wants to merge 1 commit intocelluloid-player:masterfrom
estorok:master

Conversation

@estorok
Copy link
Copy Markdown

@estorok estorok commented Dec 6, 2022

For #826
@estorok
Reduce playlist widget view updates
4574c24 
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.
gnome-mpv
gnome-mpv requested changes Dec 6, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gnome-mpv gnome-mpv left a comment •
edited
Loading

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
Copy Markdown
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

2 participants

@estorok @gnome-mpv