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

osc.lua: extract icon_font and icons variables #15645

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

guidocella
Copy link
Contributor

osc.lua: extract icon_font and icons variables

Define the icon font and icons in variables instead of scattering them throughout the code to make it easy to change them in the future, or for forks to change them. Also put the hex values in comments to easily compare which icons in the font are unused, and explain how to get the decimal values of the bytes.

Copied from ModernX.

TOOLS/mpv-osd-symbols.sfdir: remove unused icons

E003 is a stop icon that was never used
E010 to E013 are the progress bar icons removed by ef3c0e6
E111 is a duplicate of the icon to exit fullscreen, E109

Copy link

github-actions bot commented Jan 4, 2025

Download the artifacts for this pull request:

Windows
macOS

next = "\238\132\129", -- E101
pause = "\238\128\130", -- E002
play = "\238\132\129", -- E101
play_backward = "\238\132\144", -- E110
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we align = vertically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of aligning them but assignments in other tables in the file are not aligned, do you want to align only this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

it would look nice with constant value length, but whatever. avih also don't like aligning

@na-na-hi
Copy link
Contributor

na-na-hi commented Jan 5, 2025

Removing glyphs can affect third party scripts which use them for rendering icons. I also don't see a reason to remove the stop icon, as it's a common functionality and likely that some UI changes will use it later.

local icon_font = "mpv-osd-symbols"

-- Running this in Lua 5.3+ or LuaJIT converts a hexadecimal Unicode code point
-- to the decimal value of every byte for Lua 5.1 and 5.2 compatibility:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove support for puc lua?

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove support for puc lua?

I think we really shouldn't.

luajit is a project with a bus-factor of 1, highly non-hackable, and way way more complex code than puc-lua (5.1 or 5.2).

Such a core part of mpv (lua scripting) cannot depend on one person to release sources which build correctly on every platform which mpv supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be in favor of dropping support for Lua 5.1 because it makes you waste a lot of time converting unicode code points (I have the same issue in my LRC script) and the performance improvement of LuaJIT is nice for fuzzy finding, it will matter even more once history search is merged since that can grow infinitely. If LuaJIT is abandoned we should switch to Lua 5.4.

Copy link
Member

@avih avih Jan 5, 2025

Choose a reason for hiding this comment

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

I would be in favor of dropping support for Lua 5.1 because it makes you waste a lot of time converting unicode code points

Surely you don't need to do that more than once, because at least this comment was about literal codepoints embedded in literal strings as part of the script soure code, and that wouldn't change with each iteration of any loop. And if you do and can't avoid it and have to do whatever a lot, then write the slow part in C.

Using luajit is not a magic solution. because even if it's 5x faster with luajit, you run it on a RPI and it becomes x5 slower again.

It's a script, it can't be expected to perform like C code. But before that, do ensure your whatever algorithm is efficient, because typically it should be enough to gain the speed you prefer.

Also, IIRC 5.1 and 5.2 perform similarly across different use cases, from my own local tests in the past (sometimes one is faster than the other, but it's not necessarily always 5.2 which is faster, and largely they're similar).

And it's better to support more than once version, so I think we should keep both.

it will matter even more once history search is merged since that can grow infinitely.

Something which can grow indefinitely doesn't sound like a great idea to me. Surely you can drop some years old historical commands which you once issued accidentally. So pick a reasonable limit and a useful search algorithm, and it should be OK, and if not, again, write it in C, or drop this feature.

It's a script. Don't push it beyond reasonable limits.

And one can't expect minimum absolute performance (e.g. "this must finish in under 5ms"), because it can run on a wide range of processors, and an inefficient algorithm will remain slow regardess of the processor (or scripting VM), especially as the input grows.

So keep the runtime linear as much as possible with the input size, or at most log-linear, ensure each iteration is reasonably efficient, and then ensure the input size is also within reasonable limits.

That's how code works. When the input becomes big, the algorithm and implementation matter more than the absolute performance of each line of code, and limits should be put in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was talking about the watch history. It is up to users when to clear that. I did suggest integrating the C version of fuzzy search iniitally but it was rejected in favor of a Lua script because that was smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, these were my measurements last time I tested it for calling fzy when typing 1 letter in the g-r selector (10k properties):

lua 5.1: 0.166s
luajit: 0.08s
C: 0.005s

Copy link
Member

Choose a reason for hiding this comment

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

lua 5.1: 0.166s
luajit: 0.08s

If I got that right, luajit ended up 2x faster than 5.1? IMO that's a pretty small diffrence in the grand scheme of things. Definitely not enough to consider dropping 5.1. (yes, I know luajit can be much faster than 5.1 in some cases, but you always need to look at the overall gains, and not through the needle hole, and in the big pictures there are typically many more factors than how fast one function runs).

Also, I didn't look at the code, but there might still be room for further improvements. Usually there is, but not always enough of it to warrant the additional effort and possible complexity, but sometimes a small change is enough to make a big difference which is worth it.

@guidocella guidocella changed the title osc.lua: extract icon_font and icons variables and remove unused icons osc.lua: extract icon_font and icons variables Jan 5, 2025
@kasper93
Copy link
Contributor

kasper93 commented Jan 5, 2025

Needs rebase.

Define the icon font and icons in variables instead of scattering them
throughout the code to make it easy to change them in the future, or for
forks to change them. Also put the hex values in comments to easily
compare which icons in the font are unused, and explain how to get the
decimal values of the bytes.

Copied from ModernX.
@kasper93 kasper93 merged commit 31d4515 into mpv-player:master Jan 5, 2025
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.

4 participants