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

added a configuration option to select the dri node in transcoding #6376

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

t4keda
Copy link
Contributor

@t4keda t4keda commented Jan 14, 2024

This code adds a field in SystemConfigFFmpegDto to specify the node in /dev/dri used for VAAPI hardware transcoding.

The default value is auto, and in that case the VAAPIConfig config class works like up until now (it chooses the first available node in /dev/dri), but if the field is configured with a path to a dri node that exists (/dev/dri/renderD128 for example) it uses that node in the ffmpeg command in -init_hw_device parameter

Copy link
Contributor

@mertalev mertalev 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 working on this! We expose every config option in the settings UI, so this should be added there as well.

server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
@t4keda
Copy link
Contributor Author

t4keda commented Jan 14, 2024

Thanks for working on this! We expose every config option in the settings UI, so this should be added there as well.

I don't have a lot of spare time right now (hence only doing the config file part), but I will give it a shot :)

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 15, 2024

If you don't have time I'm sure one of us can help with that part, so just let us know.

@danieldietzler
Copy link
Member

I can try to get it done tomorrow if you'd like :)

@t4keda
Copy link
Contributor Author

t4keda commented Jan 16, 2024

Sorry for the delay in the response and thanks for the offering :D

Yesterday I started looking into the code of the web and I have already the option exposed, I will try to finish tonight the work and commit it.

Also I am thinking that maybe this option can be used also por QSV and other acceleration APIs, but I have to check it first

@mertalev
Copy link
Contributor

It can be used for QSV, but it would need to be tested to check that the command still works. Unlike VAAPI, we don't pass a device to the QSV command and let QSV decide what to use.

@t4keda t4keda force-pushed the add_accel_node_selection branch 3 times, most recently from 731f952 to 8be6653 Compare January 22, 2024 00:54
@t4keda
Copy link
Contributor Author

t4keda commented Jan 22, 2024

Sorry for the delay, but I haven't had any time to look into this the past week.

I have exposed the option in the settings UI, rebased the branch to be in sync with the latest main commits and I have also used the option in QSVConfig to select a dri node.

I have not been able to test QSV command (my hardware doesn't seems to support QSV in linux?), the ffmpeg command fails every time, but according to documentation the arguments are correct. If anyone can confirm that it is working it would be very helpful, if not, I will just revert the commit and move it to another branch.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

Looks good to me!

server/src/domain/media/media.util.ts Outdated Show resolved Hide resolved
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.

Nicely done!

@mertalev mertalev merged commit 76f8d03 into immich-app:main Jan 30, 2024
23 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.

None yet

4 participants