Skip to content

player: normalize paths for resuming playback - #12303

Merged
kasper93 merged 2 commits into
mpv-player:masterfrom
guidocella:normalize-paths
May 5, 2024
Merged

kasper93 merged 2 commits into
mpv-player:masterfrom
guidocella:normalize-paths

Conversation

@guidocella

Copy link
Copy Markdown
Contributor

Paths like foo.mkv, ./foo.mkv .//foo.mkv, ../"$(basename "$PWD")"/foo.mkv, and C:\foo.mkv and C:/foo.mkv on Windows, use different config files for resuming playback, so if you quit-watch-later and later play the same file with a different path mpv does not resume playback. This commit normalizes the paths to fix this.

References #495

Someone needs to fix the code for Windows, and also test if the performance impact of calling GetFullPathNameW on thousands of playlist entries is acceptable. On Linux realpath is actually much slower than this normalizing function. The loop in mp_check_playlist_resume with 2330 images takes 0.012 seconds before this patch, 0.014 with it, and if I make it use realpath with /dir/../dir/* it takes 0.038 seconds.

@natural-harmonia-gropius

Copy link
Copy Markdown
Contributor

Will result of mp.get_property("path") be affected by this?

@guidocella

Copy link
Copy Markdown
Contributor Author

Will result of mp.get_property("path") be affected by this?

No, it affects the paths that determine the hashes of watch later config files.

@natural-harmonia-gropius

Copy link
Copy Markdown
Contributor

No, it affects the paths that determine the hashes of watch later config files.

Could you add a normalize-path command just like expand-path?

@guidocella

Copy link
Copy Markdown
Contributor Author

Could you add a normalize-path command just like expand-path?

That's fine if maintainers think it's worth adding, but it should be in a later PR.

@guidocella
guidocella marked this pull request as ready for review September 26, 2023 08:45
@guidocella

Copy link
Copy Markdown
Contributor Author

I made this Unix only since there is no interest in fixing it on Windows.

@github-actions

github-actions Bot commented Sep 26, 2023 •

Copy link
Copy Markdown

Download the artifacts for this pull request:

Windows
macOS

@N-R-K N-R-K left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Untested, but seems fine code wise.

Comment thread options/path.c Resolved
@stacyharper

Copy link
Copy Markdown

I also relate the same problem because I mount some video files in different locations, depending on the machine I am using. Because my user name differ, even if I mount my files in ~/foo, it ends up in /home/stacy/foo, or /home/willow/foo depending on the installation.

What about ignoring the path while building the hash? We could just use metadata, and/or file name?

@guidocella

Copy link
Copy Markdown
Contributor Author

--ignore-path-in-watch-later-config

@stacyharper

Copy link
Copy Markdown

I was aware of this argument. But both with or without have pros and cons. I would prefer relying on a md5 of the complete file, rather than it's file name and/or path.

@guidocella

Copy link
Copy Markdown
Contributor Author

mpv compares the watch later path of every playlist entry to determine what file to resume from, and it would be too slow to calculate the hash of the content of thousands of playlist entries, which can even be network URLs.

@stacyharper

Copy link
Copy Markdown

Fair points. Sorry for debating on this MR o/

@garoto

garoto commented Jan 10, 2024 •

Copy link
Copy Markdown
Contributor

What would be the computational considerations regarding MD5 hashing the first 4096 [1] bytes of every file the player loads? Tested with around ~900 matroskas I have laying around and found no hash collisions, and performace was acceptable disregarding OS|HW caching mechanics.

Super fast on Winblows using busybox-w32 built-in utils (head|md5sum), and I can only speculate that C code doing the same would be even less demanding. Just hide it behind an option or something.

Totally OT for this PR I know.

[1]: random bytes pulled from my a**.

@guidocella

Copy link
Copy Markdown
Contributor Author

There are also redirect entries for directory paths which don't have any content to hash. But I guess it would be fine as an alternative option, maybe limited to current file. It would also fix mpv-android/mpv-android#542.

Comment thread misc/path_utils.c Resolved
@guidocella
guidocella force-pushed the normalize-paths branch 2 times, most recently from 1c58f56 to f822c5a Compare April 20, 2024 20:32
@kasper93

kasper93 commented May 5, 2024

Copy link
Copy Markdown
Member

Please add a test for mp_normalize_path in test/paths.c. Things like that can easily break, especially on different platforms.

@guidocella
guidocella force-pushed the normalize-paths branch 3 times, most recently from 7714a2e to d1b9fca Compare May 5, 2024 15:20
Paths like foo.mkv, ./foo.mkv .//foo.mkv, ../"$(basename
"$PWD")"/foo.mkv, and C:\foo.mkv and C:/foo.mkv on Windows, use
different config files for resuming playback, so if you quit-watch-later
and later play the same file with a different path, mpv does not resume
playback. This commit normalizes the paths on Unix to fix this.
Comment thread misc/path_utils.c Resolved
@kasper93
kasper93 force-pushed the normalize-paths branch from 0369b2a to 1f59cca Compare May 5, 2024 17:14
@kasper93
kasper93 force-pushed the normalize-paths branch 3 times, most recently from 2603f79 to 4b2310b Compare May 5, 2024 17:22
@kasper93
kasper93 force-pushed the normalize-paths branch from 4b2310b to 6e9b6fa Compare May 5, 2024 17:26
@kasper93
kasper93 merged commit 053d971 into mpv-player:master May 5, 2024
@guidocella
guidocella deleted the normalize-paths branch May 5, 2024 17:38
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.

7 participants