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): avoid transcoding thumbnail streams #11603

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

mincrmatt12
Copy link
Contributor

Currently the only property of video streams used when picking one to transcode is their lengths (always trying to select the longest one). Certain video formats embed thumbnails as "video" streams but then don't report back their lengths in frames via ffprobe.

For example, some WMVs from an ancient camera of mine have an embedded thumbnail as stream 0 and the actual video as stream 1, but ffprobe doesn't report an nb_frames for either so they both get treated as length 0 and the first one is selected, which causes ffmpeg to explode.

This patch adds a second layer to the sort for "main video streams", where if two streams are of the same length but one was marked by ffprobe as having the disposition attached_pic (which is used for video thumbnails), the one not marked as attached_pic is preferred. In this way, ffmpeg is given the correct stream and successfully generates a transcoded version of my ancient WMVs on a test instance. I wouldn't be surprised if this is the problem behind #11100 as well, but there isn't enough information in that report to check.

Currently the only consumer of this new holdsThumbnail property is the transcoding logic; there is probably some more intelligent way to use it when generating our own video thumbnails but I have not explored that here.

@mincrmatt12 mincrmatt12 force-pushed the fix-transcode-thumbnails branch 2 times, most recently from aa5065c to 3ae9ee6 Compare August 6, 2024 23:28
@mincrmatt12
Copy link
Contributor Author

Just checking, am I supposed to do something about the CI failures on this? I don't see any useful error on the erroring job.

@mertalev
Copy link
Contributor

mertalev commented Aug 7, 2024

The GH runner ran out of storage apparently 😅

@mertalev mertalev merged commit fb68da2 into immich-app:main Aug 7, 2024
21 of 22 checks passed
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.

3 participants