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

refactor(server): modularize getFfmpegOptions #3138

Merged
merged 13 commits into from
Jul 9, 2023
Merged

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Jul 7, 2023

Description

Modularizes getFfmpegOptions into separate handlers for each codec and adds enums for several transcoding settings.

getFfmpegOptions has become too complex and monolithic to make major changes to it without either increasing its complexity further or introducing bugs. This change makes the intended behavior for each codec much more explicit, and makes adding a new codec as simple as adding a new handler without affecting existing logic.

Notably, the only major change is to the code structure. Existing behavior should be the same.

Testing

Tests are unchanged (except for the addition of an inputOptions property in TranscodeOptions) and all pass. I tried different transcoding settings and all were applied and transcoded correctly.

@vercel
Copy link

vercel bot commented Jul 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Jul 8, 2023 11:18pm

refactor transcoding, make separate service
@mertalev mertalev force-pushed the refactor/transcoding branch from fd9eab4 to 03e8b3b Compare July 7, 2023 05:19
@mertalev mertalev changed the title refactor(server): transcoding refactor(server): modularize getFfmpegOptions Jul 7, 2023
Copy link
Contributor

@brighteyed brighteyed left a comment

Choose a reason for hiding this comment

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

Awesome! getFfmpegOptions was a real pain!

server/src/infra/repositories/media.repository.ts Outdated Show resolved Hide resolved
server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
mertalev and others added 3 commits July 7, 2023 14:34
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

If you could add some more tests that would be good, otherwise LGTM. I think the fact the existing tests still pass gives high confidence that the code is working the same way as before.

@alextran1502 alextran1502 merged commit 8349a28 into main Jul 9, 2023
@alextran1502 alextran1502 deleted the refactor/transcoding branch July 9, 2023 02:43
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