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

fix(server): hevc tag being set when copying a non-hevc stream #8582

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Apr 7, 2024

Description

Fixes the issue described in #8567

Copy link

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: 32acd14
Status: ✅  Deploy successful!
Preview URL: https://f777fdfd.immich.pages.dev
Branch Preview URL: https://fix-server-hevc-tag.immich.pages.dev

View logs

Copy link

@Harry-Chen Harry-Chen left a comment

Choose a reason for hiding this comment

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

Thanks for your quick fix! Just some additional questions on tests.

if (
this.config.targetVideoCodec === VideoCodec.HEVC &&
(videoCodec !== 'copy' || videoStream.codecName === 'hevc')
) {

Choose a reason for hiding this comment

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

This looks good now.

outputOptions: [
'-c:v copy',
'-c:a aac',
'-movflags faststart',

Choose a reason for hiding this comment

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

I have a little bit of concern--not directly relevant to this PR.

These tests seem too strict in that they compare every flag passed to ffmpeg, not only the essential one. Assume one day some default options like preset is changed, all these tests must be updated.

Maybe (if feasible) it is more flexible to only check the existence (and non-existence) of some arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's very annoying haha. Even just changing the order of the flags breaks tests. I've been meaning to change them to be more targeted. The only thing is that the arguments to toHaveBeenCalledWith have to be a perfect match - you can't check certain fields or use objectContaining/arrayContaining. But I think checking the raw calls directly should work.

@mertalev mertalev merged commit 55b9acc into main Apr 7, 2024
24 of 25 checks passed
@mertalev mertalev deleted the fix/server-hevc-tag branch April 7, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants