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

Allow configuring diag font path at build time #1523

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

oscarwcl
Copy link
Contributor

@oscarwcl oscarwcl commented Jan 17, 2024

This allows for better integration with distros that put the system font in a different directory.

As suggested in #1308 (comment).

The equivalent of the old -DUSE_SYSTEM_DIAG_FONT=ON option is now -DDIAG_FONT_PATH=/usr/share/fonts/truetype/liberation/LiberationMono-Regular.ttf

This allows for better integration with distros that put the system font
in a different directory.
@Julusian Julusian merged commit 3c1d1f9 into CasparCG:master Jan 17, 2024
4 checks passed
@@ -12,6 +12,14 @@ set(CASPARCG_DOWNLOAD_CACHE ${CMAKE_CURRENT_BINARY_DIR}/external CACHE STRING "D

option(ENABLE_HTML "Enable HTML module, require CEF" ON)

set(DIAG_FONT_PATH "LiberationMono-Regular.ttf" CACHE STRING
"Path to font that will be used to load diag font at runtime. By default
this loads the font distribtued with the application from the working
Copy link
Contributor

Choose a reason for hiding this comment

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

typo - distribtued, should be distributed

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Sep 10, 2024

I am a bit late to the scene, but what if the font path is not known at build-time? Or, what if it's not the same on the build machine compared to the target machine?...

IMHO the options should be: (1) bundle the font in a specific location (eg, next to the main executable) and (2) use system facilities to locate the font (eg, fontconfig on Linux).

@Julusian
Copy link
Member

The aim of this is to be friendly for distributing as a debian/arch/whatever package. In which case it can be reasonably assumed that the font will be found at a predictable path.

Yeah I didn't think about using a system library to locate it. But I probably wouldn't have done it anyway, because it seems like quite an effort for what it is.
I'll accept a pr that uses fontconfig for this, as long as it isn't too big of a heap of code to do so

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Sep 10, 2024

I've looked at the fontconfig API and it's a bit of a mess... One lazy way of doing it would be something like this:

std::string font_path;
auto pipe = popen("fc-match --format=%{file} LiberationMono-Regular.ttf", "r");
char buf[128];
while (fgets(buf, sizeof(buf), pipe)) font_path += buf;

It's marginally less hacky than hard-coding the path, but if you are cool with it, I can spin off a PR shortly.

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Sep 10, 2024

@Julusian see #1559

One thing I saw that I didn't like was that the get_default_font() function contains a static variable (DEFAULT_FONT). While it has a nice feature of being initialized on first use, it does involve a hidden lock (since C++11) and a check on each invocation.

In this particular case it would be cheaper to create a class static variable (inside graph) and initialize it once. There are also a few other statics inside graph::render() with the same problem. I believe this function gets called multiple times per second, so there could be some performance hit.

@dimitry-ishenko
Copy link
Contributor

To elaborate on using static local variables, see this: https://godbolt.org/z/oEd7GMseT. Every time the function is called, it has to acquire and release the lock.

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Sep 11, 2024

@Julusian here are test results, which show that using global static variables gives ~15% speed improvement on Coliru.

Single-threaded: https://coliru.stacked-crooked.com/a/ba4ab9ed90d28352

Static  local: 5824ms
Static global: 4856ms (-16.62%)

Multi-threaded: https://coliru.stacked-crooked.com/a/ac8424b20ed46f5a

Static  local: 5573ms
Static global: 4738ms (-14.98%)

On my local machine I get even more dramatic results:

Single-threaded:

Static  local: 5983ms
Static global: 3992ms (-33.28%)

Multi-threaded:

Static  local: 3697ms
Static global: 2064ms (-44.17%)

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