Skip to content

fix: Don't attach missing or substituted fonts on submit #118

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

Open
wants to merge 43 commits into
base: mainline
Choose a base branch
from

Conversation

cilevitz
Copy link
Contributor

@cilevitz cilevitz commented Feb 1, 2025

What was the problem/requirement? (What/Why)

The submitter was collecting all font files in a project, including substituted fonts resulting from After Effects loading a project with missing or previous substituted fonts. Render tasks proceeded using the substituted fonts, producing incorrect output despite the tasks being marked as Succeeded.

What was the solution? (How)

We filter out any missing or substituted fonts from the list of used font files that gets processed when collecting job attachments. We also adjusted call_aerender.py so that it errors out if it detects the warning Project has missing fonts in the render log.

What is the impact of this change?

Users can use any custom fonts and cloud fonts they'd like, and all chosen fonts will render correctly on the farm. The user is alerted to any missing or substituted fonts before opening the submitter. Missing fonts will cause a render task to stop and be marked "Failed".

How was this change tested?

Testing was performed with SMF hosts running After Effects 24.0 and 25.1 to verify correct aerender output of a project containing custom and cloud fonts. The farm render output was compared to local renders from a machine with those fonts installed, and the farm output is as expected. The log output parser in call_aerender.py stops and fails the render task if a missing font warning is seen.

Was this change documented?

The README.md was updated with notes about missing and substituted fonts.

Is this a breaking change?

No.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eherozhao and others added 30 commits December 9, 2024 14:31
* feat: dockable submitter using aerender

Signed-off-by: Hao Zhao <[email protected]>
* docs: update README, CONTRIBUTING and DEVELOPMENT doc
---------
Signed-off-by: Hao Zhao <[email protected]>
…ion (aws-deadline#80)

* fix: provide a way for CMF customers with aerender exectuable env options

---------

Signed-off-by: Hao Zhao <[email protected]>
* chore: remove unused logging

---------

Signed-off-by: Hao Zhao <[email protected]>
…ne#83)

* fix: update the MacOS submit process to use bash as to avoid deadline path issue

---------

Signed-off-by: Hao Zhao <[email protected]>
* chore: remove unused code

---------

Signed-off-by: Hao Zhao <[email protected]>
feat: add hostRequirement windows for AE job template
---------

Signed-off-by: Hao Zhao <[email protected]>
…... (aws-deadline#86)

* fix: Remove CondaPackages parameter override to restore default parameter set in queue environment
---------
Signed-off-by: Vikram Nithyanandam <[email protected]>
…tachments

feat: Multi-level logging output in font_manager.py
docs: Update README
fix: Direct invocation of python font_manager.py install
fix: Update docstrings
fix: Loop index names
fix: f-string formatting
fix: Scan all TextLayer keys for font changes
fix: Remove .ttc support
docs: Update README
docs: Update documentation with font installation steps
chore: Update CODEOWNERS
fix: getFontPaths() error alert
chore: Refactor get_font_name()
Signed-off-by: Daniel Cilevitz <[email protected]>
fix: Update README.md with fonttools installation
fix: Add exception types to exception handlers

Signed-off-by: Daniel Cilevitz <[email protected]>
fix: Show error dialog if fatal error when finding fonts
fix: Fail render tasks with an error if missing fonts at render time
@@ -91,6 +91,11 @@ def main():
if "WARNING:After Effects warning" in line:
print(f"After Effects Warning: {line.strip()}", file=sys.stderr)

# Some warnings should be treated as errors, i.e. a fatal warning
if "Project has missing fonts" in line:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to make sure in the submitter side, we have enough warning to customers that the submitted project with missing fonts will fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to understand this behavior from customer perspective:

  1. If I am a customer wanting to submit a job with missing custom font and I don't really care about the missing fonts, the render should succeeded.
  2. If I am a customer wanting to submit a job with missing custom font and I want to make the change to update them before the job is rendered in the Deadline Cloud side.
    In this way, I think we can have a warning pop up for customers that "are you sure you are going to submit a job with missing fonts?" with a yes and no ask. If yes, our job attachment should auto detect the replaced fonts with the replaced font name (not the one shown in the application)" If no, we just cancel the job submission.

I don't think we always fail customers missing fonts in the worker with no ask is a good customer experience, especially the worker has to run all steps before this failure, which seems wasting their money.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added a confirmation dialog when submitting a composition containing missing or substituted fonts.

@@ -72,7 +72,7 @@ def get_font(font_path):
raw_table = {}
try:
raw_table = {i:str(names_table[i]) for i in range(0, len(names_table))}
except Exception:
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

q: if we don't print out error in the block, do we still need as e? I would suggest us either print the error out or delete as e.

Also, are we able to print out the python results from subprocess to the main submitter output to surface to customers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Font scan errors from Python are now surfaced in the AE UI.

for (var i = 0; i < fontsInProject.length; i++) {
var attachFont = true;
// If the font's fontPostScriptName contains a missing or substituted font name, don't attach it
if (app.fonts.missingOrSubstitutedFonts.toString().indexOf(fontsInProject[i].fontPostScriptName) !== -1) {
Copy link
Contributor

@viknith viknith Feb 2, 2025

Choose a reason for hiding this comment

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

Wouldn't the alternative behavior be to just name the files correctly with the fonts that will actually be used in place of the missing fonts? IIRC, that was the confusing part, since the substituted font files were being renamed to fonts that actually won't be included. It may be possible that customers are aware that their composition is missing the correct fonts for now, but don't want to modify the composition and allow Adobe to substitute in some default fonts for now and just get a job rendering for now.

This behavior could be toggle-able in case customers still want to submit the job with substituted fonts and want them to pass, and at the same time we can avoid misnaming files. Might be out of scope of this PR, bu it is worth discussing.

Copy link
Contributor

@viknith viknith Feb 2, 2025

Choose a reason for hiding this comment

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

Essentially, warnings are good, but I'm concerned about the job failing due to substituted fonts, and we should have a checkbox for customers on whether customers want substituted font jobs to pass or fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing or substituted fonts are now added to job attachments with the name of their substitution, e.g. CyTextBold substituted to ArialMT gives an attachment named ArialMT.

Missing fonts will not cause a job to fail on the farm, but will produce incorrect render output.

The user is warned when clicking Submit that they should install missing or substituted fonts to ensure correct render output.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am using a unsupported ttc font type, what will it behave? I didn't see the notification that the font was substituted to the default font so I think it might be a missing part when the font was not supported (but not missing). Wanna hear about your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitting a project from Mac with a TTC font unsupported by Windows (and no other missing or substituted fonts at submit time) will result in no warnings from the submitter. The font will not be installed by font_manager.py on a Windows render node, as unsupported fonts cannot be installed. aerender will warn about missing fonts at render time. Render output will have substituted fonts.

// Build a new array containing only attachable fonts.
var fontsInProjectFiltered = [];
for (var i = 0; i < fontsInProject.length; i++) {
var attachFont = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this code can be simplified to an if/else structure, no need for extra attachFont boolean:

if (app.fonts.missingOrSubstitutedFonts.toString().indexOf(fontsInProject[i].fontPostScriptName) !== -1) {
   logger.warning("Not attaching missing or substituted font: " + fontsInProject[i].fontPostScriptName, jobTemplateHelperFile);
} else {
    fontsInProjectFiltered.push(fontsInProject[i]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in c9c25ca

@viknith
Copy link
Contributor

viknith commented Feb 2, 2025

On Windows and MacOS for AE 24.6.4 and AE 25.1, I was able to confirm that the error messages popped up correctly:

Screenshot 2025-02-02 at 3 30 50 PM Screenshot 2025-02-02 at 3 32 26 PM

And that the job attachments did not include substituted files with the missing font names

Screenshot 2025-02-02 at 2 45 00 PM
Screenshot 2025-02-02 at 3 22 05 PM

And here was the job submission results when they were submitted, all failing.
Screenshot 2025-02-02 at 3 34 53 PM
Screenshot 2025-02-02 at 3 49 22 PM

Seems to be all in working order! Just need to address some of the comments from above, I think the one from Hao about letting customers know that their job will likely fail due to the missing font is important.

@eherozhao eherozhao self-requested a review February 4, 2025 17:29
Copy link
Contributor

@eherozhao eherozhao left a comment

Choose a reason for hiding this comment

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

Do you mind also fixing the SonarQube and signing? Thanks!

Daniel Cilevitz and others added 5 commits February 7, 2025 02:31
fix: Attach substituted fonts by their substituted font name
fix: Warn instead of error if font files are in use when being copied or removed
fix: Warn instead of error for "Project has missing fonts"

Signed-off-by: Daniel Cilevitz <[email protected]>
@cilevitz
Copy link
Contributor Author

cilevitz commented Feb 7, 2025

SonarQube issues have been resolved.

@eherozhao eherozhao marked this pull request as ready for review February 10, 2025 20:56
@eherozhao eherozhao requested a review from a team as a code owner February 10, 2025 20:56
README.md Outdated
Supported font types include: OpenType (`.otf`), TrueType (`.ttf`), and [Adobe Fonts](https://fonts.adobe.com/).
Windows bitmap fonts (`.fon`) are only supported on Windows machines.

If fonts are missing at render time, first check that they're installed (on the system or your user), and then check they're being included in the job attachments tab in the submitter.
If fonts are missing or substituted at render time, then render tasks will fail. You must add any required fonts to your system or your user profile before opening a project that needs them. First check that the missing fonts are installed (on the system or your user), and then check they're being included in the job attachments tab in the submitter.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we wouldn't fail the render job if the font was substituted if I understood the implementation correctly. Please confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation has been updated in ea25a1f to indicate a choice to submit a job with missing or substituted fonts, even though the render output will not be correct.

"-" +
f(this.getUTCMonth() + 1) +
"-" +
f(this.getUTCDate()) +

Check failure

Code scanning / CodeQL

Useless type test

The result of this 'typeof' expression is compared to [null](1), but the two can never be equal.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a spurious error.

winreg.DeleteValue(key, fontname)

try:
with winreg.OpenKey(registry_scope, FONTS_REG_PATH, 0, access= winreg.KEY_SET_VALUE) as key:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: access= winreg.KEY_SET_VALUE has extra space after equal sign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in ea25a1f

* Collects metadata from a given font and determines if it needs an override name.
* @return Object containing the override name or an error
**/
function getFontNameOverride(fontObject, fontPostScriptName, fontLocation, oldLocation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: oldLocation is an unused variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in ea25a1f

var fontPostScriptName = null;
try {
// Get user-installed fonts
var fontPaths = getFontPaths();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR, but we should cache the output of getFontPaths because the output is the same each time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fontPaths are now cached in ea25a1f

@@ -92,6 +92,17 @@ function SubmitSelection(selection, framesPerTask) {
var dependencies = findJobAttachments(rqi.comp); // list of filenames
var compName = dcUtil.removeIllegalCharacters(rqi.comp.name);

if (app.fonts.missingOrSubstitutedFonts != "") {
var submitConfirmation = confirm(
Copy link
Contributor

@viknith viknith Feb 10, 2025

Choose a reason for hiding this comment

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

Just tried submitting a job that had no missing or substituted fonts, all using just Amazon Ember and the GUI submitter CLI command was not reached and the submitter fails silently.

The root caused is happening around these lines of code.

Most likely it's because this variable is never created and set to true if there are no missing fonts. As a result, it defaults to false, returns, and then there's not notification to the customer. This needs to be set to default true initially and then have the if-check with confirmation. Requires code fix.

Copy link
Contributor

@viknith viknith Feb 10, 2025

Choose a reason for hiding this comment

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

Tried to nest the second if statement under the first with missing fonts but that didn't work, AE ends up loading and then crashes from not responding. This is AE 25.1 on MacOS btw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in ea25a1f

}
// Build a new array containing only attachable fonts.
var fontsInProjectFiltered = [];
for (i = 0; i < fontsInProject.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need it to be var i = 0 here?

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 is reused from earlier in the function and doesn't need redeclaration. Changed i to f for clarity.

cilevitz and others added 2 commits February 10, 2025 19:34
fix: Set submitConfirmation = true by default
docs: Warning on missing or substituted fonts
nit: Parameter spaces
nit: Variable names

Signed-off-by: Daniel Cilevitz <[email protected]>
Copy link
Contributor

@eherozhao eherozhao left a comment

Choose a reason for hiding this comment

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

image I tried to submit a job for a scene with unsupported font. For the existing mainline version, the font was substituted by AE to some default font. However, using this PR branch, I didn't see the confirmation introduced from this PR asking about if I should submit the job if there is a missing or substituted fonts, also the job attachment was not having the substituted fonts. This will make the customer experience worse.

}
}
} catch (e) {
logger.error(e.message, jobTemplateHelperFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this error logged in my log file.

2025-02-10 14:42:22 - [ERROR ] JobTemplateHelper.json: JSON.parse
2025-02-10 14:42:22 - [ERROR ] JobTemplateHelper.json: Object of type Object found where a Number, Array, or Property is needed

I think it was from this line. Can you please check if this was parsed correctly?

# Some warnings should be treated as errors, i.e. a fatal warning
if any([w in line for w in FATAL_WARNINGS]):
print(f"After Effects Fatal Warning: {line.strip()}", file=sys.stderr)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I submitted a job with missing fonts, but this didn't stop my job rendered. Here is the log.

2025/02/11 13:23:17-08:00 After Effects Warning: WARNING:After Effects warning: Grouped_Message_{This project contains references to missing effects. Please install the following effects to restore these references.|||This project contains a reference to a missing effect. Please install the following effect to restore this reference.###@0 missing effects.}"Saber"
2025/02/11 13:23:18-08:00 WARNING:After Effects warning: Project has missing fonts.
2025/02/11 13:23:18-08:00 After Effects Warning: WARNING:After Effects warning: Project has missing fonts.
2025/02/11 13:23:18-08:00  
2025/02/11 13:23:18-08:00 rqindex 1num 1
2025/02/11 13:23:18-08:00 PROGRESS:  2/11/2025 9:23:18 PM: Starting composition "test_comp".
2025/02/11 13:23:18-08:00  
2025/02/11 13:23:18-08:00 PROGRESS:  Render Settings: Best Settings

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