[ASSETS-36125] Update SVG creation logic #216

tmathern · 2024-03-15T16:08:14Z

Description

We had some logic for default sizes of SVG files that only took width into account. This PR updates this logic to look at both width and height when picking (default) sizes for rendition generation.

Fixes # ASSETS-36125

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have ~~added~~ updated tests to cover my changes.
All new and existing tests passed.

…o ASSETS-36125

tmathern · 2024-03-15T17:45:45Z

test-worker/test/asset-compute/worker/svg-alpha-gif/rendition.gif

Seems there was no (visible) change in this rendition

tmathern · 2024-03-15T17:46:06Z

test-worker/test/asset-compute/worker/svg-aspect-ratio-height/rendition.png

Off-by-one change - logic change leads to width being 1 px smaller.

tmathern · 2024-03-15T17:46:10Z

test-worker/test/asset-compute/worker/svg-png-missing-width/rendition.png

Off-by-one change - logic change leads to width being 1 px smaller.

tmathern · 2024-03-15T17:47:19Z

test/api.test.js

@@ -1091,7 +1091,8 @@ describe("api.js", () => {
            assert.ok(!fs.existsSync(renditionDir));
        });

-        it('should send metrics - rendition and activation with cgroup metrics', async () => {
+        it.skip('should send metrics - rendition and activation with cgroup metrics', async () => {


Flaky: sometimes (most often) hangs, sometimes runs. Keeping for local testing.

is this hanging happening in prod too?

codecov · 2024-03-15T17:48:12Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 55.35%. Comparing base (49d52a1) to head (3361e8c).
Report is 1 commits behind head on master.

Files	Patch %	Lines
lib/postprocessing/image.js	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   56.95%   55.35%   -1.60%     
==========================================
  Files          12       12              
  Lines         834      840       +6     
==========================================
- Hits          475      465      -10     
- Misses        359      375      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tmathern · 2024-03-15T17:49:21Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 55.35%. Comparing base (49d52a1) to head (3361e8c).
Report is 1 commits behind head on master.

Files Patch % Lines
lib/postprocessing/image.js 0.00% 8 Missing ⚠️
Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

Covered in the worker tests!

tmathern · 2024-03-15T17:50:54Z

.github/workflows/node.js.yml

@@ -15,7 +15,7 @@ jobs:
    if: "!contains(github.event.head_commit.message, '[ci skip]')"
    strategy:
      matrix:
-        node-version: [14.17]
+        node-version: [18.4.0]


Even more tests hang/fail without this update, because when installing some libs warn of a bad (outdated) node engine.

tmathern · 2024-03-15T17:55:11Z

lib/postprocessing/image.js

@@ -634,13 +634,21 @@ async function imagePostProcess(intermediateRendition, rendition, directories) {

 function convertSvg(img, instructions) {
    // ImageMagick automatically keeps aspect ratio
-    // Only using width because ImageMagick will use the smallest value whether it be width or height
-    const width = instructions.width || SVG_DEFAULT_WIDTH;
+    let imageSize = SVG_DEFAULT_WIDTH;


Would be used only if nothing is set in instructions at all.

tmathern · 2024-03-15T17:56:26Z

lib/postprocessing/image.js

+        // image magick will keep aspect ratio
+        imageSize = `${instructions.width}x${instructions.height}`;
+    } else if(instructions.width) {
+        imageSize = `${instructions.width}x`;


The x is left for clarity in the command param, to know we got width only (width is considered to be by default the first value in the command param).

do we want to log wether we only got with or height or got both instead of having to examine the cmd?

tmathern · 2024-03-15T17:56:51Z

lib/postprocessing/image.js

+    } else if(instructions.width) {
+        imageSize = `${instructions.width}x`;
+    } else if(instructions.height) {
+        imageSize = `x${instructions.height}`;


The x is left for clarity in the command param to know we got height only, and to make sure aspect ratio is kept, since height is the second param (and the first param, width, is "empty").

jdelbick

a few qs, but LGTM :)

jdelbick · 2024-03-18T16:47:03Z

lib/postprocessing/image.js

+        // image magick will keep aspect ratio
+        imageSize = `${instructions.width}x${instructions.height}`;
+    } else if(instructions.width) {
+        imageSize = `${instructions.width}x`;


do we want to log wether we only got with or height or got both instead of having to examine the cmd?

jdelbick · 2024-03-18T16:48:15Z

test/api.test.js

@@ -1091,7 +1091,8 @@ describe("api.js", () => {
            assert.ok(!fs.existsSync(renditionDir));
        });

-        it('should send metrics - rendition and activation with cgroup metrics', async () => {
+        it.skip('should send metrics - rendition and activation with cgroup metrics', async () => {


is this hanging happening in prod too?

adobe-bot · 2024-03-18T16:55:03Z

🎉 This PR is included in version 4.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

CHange svg creation logic

507088f

tmathern self-assigned this Mar 15, 2024

tmathern and others added 12 commits March 15, 2024 09:20

Up node action version

8c8f263

Fix var name

ab04f5f

Skip flaky test

9d5e711

More correct rendition

435d5d0

More correct rendition

8ed9fec

Handle transparency properly

ffcc41d

Update gif rendition

213b3aa

Update renditipns

1343dfe

Delete test-worker/.nui/2024_03_15T17_14_15_238Z/in/file.svg

0c2a53a

Delete folder

e23f238

Merge branch 'ASSETS-36125' of github.com:adobe/asset-compute-sdk int…

4a1e17b

…o ASSETS-36125

Lint

3361e8c

tmathern commented Mar 15, 2024

View reviewed changes

tmathern marked this pull request as ready for review March 15, 2024 17:49

tmathern requested review from pheenomenon and sudheendrakatikar March 15, 2024 17:49

tmathern commented Mar 15, 2024

View reviewed changes

jdelbick approved these changes Mar 18, 2024

View reviewed changes

sudheendrakatikar approved these changes Mar 18, 2024

View reviewed changes

tmathern merged commit badd5ae into master Mar 18, 2024
5 checks passed

tmathern deleted the ASSETS-36125 branch March 18, 2024 16:51

adobe-bot added the released label Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASSETS-36125] Update SVG creation logic #216

[ASSETS-36125] Update SVG creation logic #216

tmathern commented Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024

tmathern Mar 15, 2024

tmathern Mar 15, 2024 •

edited

Loading

jdelbick Mar 18, 2024

codecov bot commented Mar 15, 2024

tmathern commented Mar 15, 2024

Codecov Report

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024

tmathern Mar 15, 2024 •

edited

Loading

jdelbick Mar 18, 2024

tmathern Mar 15, 2024 •

edited

Loading

jdelbick left a comment

jdelbick Mar 18, 2024

jdelbick Mar 18, 2024

adobe-bot commented Mar 18, 2024

[ASSETS-36125] Update SVG creation logic #216

[ASSETS-36125] Update SVG creation logic #216

Conversation

tmathern commented Mar 15, 2024 • edited Loading

Description

Types of changes

Checklist:

tmathern Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

tmathern Mar 15, 2024

Choose a reason for hiding this comment

tmathern Mar 15, 2024

Choose a reason for hiding this comment

tmathern Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

jdelbick Mar 18, 2024

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2024

Codecov Report

tmathern commented Mar 15, 2024

Codecov Report

tmathern Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

tmathern Mar 15, 2024

Choose a reason for hiding this comment

tmathern Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

jdelbick Mar 18, 2024

Choose a reason for hiding this comment

tmathern Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

jdelbick left a comment

Choose a reason for hiding this comment

jdelbick Mar 18, 2024

Choose a reason for hiding this comment

jdelbick Mar 18, 2024

Choose a reason for hiding this comment

adobe-bot commented Mar 18, 2024

tmathern commented Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading

tmathern Mar 15, 2024 •

edited

Loading