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

add support for inputOptions and videoFilters in transcode plugin #3917

Merged
merged 7 commits into from Apr 9, 2021

Conversation

TheoLeCalvar
Copy link
Contributor

@TheoLeCalvar TheoLeCalvar commented Apr 3, 2021

Description

This PR adds support for inputOptions and videoFilters in transcode plugins.
Transcode plugins can be used to add support for new encoders.
However, some require inputOptions and thus cannot be used with current transcode plugin API.

For instance, vaapi hardware accelerated encoders require these input options -hwaccel vaapi -hwaccel_output_format vaapi -vaapi_device /dev/dri/renderD128

Related issues

Closes #3846

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 👍 yes, light tests as follows are enough
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

@TheoLeCalvar TheoLeCalvar changed the title add support for inputOptions in trancode plugins add support for inputOptions in videoFilters in transcode plugin Apr 3, 2021
@rigelk rigelk added Component: Transcoding Component: PeerTube Plugin 📦 Features that can be developed in a plugin, but require PeerTube plugin API development labels Apr 4, 2021
@TheoLeCalvar TheoLeCalvar changed the title add support for inputOptions in videoFilters in transcode plugin add support for inputOptions and videoFilters in transcode plugin Apr 4, 2021
Copy link
Owner

@Chocobozzz Chocobozzz left a comment

Choose a reason for hiding this comment

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

support/doc/plugins/guide.md Show resolved Hide resolved
@TheoLeCalvar
Copy link
Contributor Author

I'm wondering if this PR should also include a mechanism to mitigate #3919 .
Plugins could provide the name of the filter to use for scaling, with scale as default value.

With this info we could use the correct filter name here

filter: 'scale',

And we could replace the call to size() here

command = command.size(size)

by a manually written scale filter that would use the filter name provided by the transcode-plugin.

@Chocobozzz
Copy link
Owner

Chocobozzz commented Apr 8, 2021

Okay thanks, so to summarize if we do the following things, does it fix all the issues?

  • Add ability for plugins to register a custom scale function that should return { filterName: string, value: string }. The default in PeerTube would be something like { filterName: 'scale', value: 'w=trunc(oh*a/2)*2:h=360' } or { filterName: 'scale', value: 'w=-2:h=360' } (not sure what is the best)
  • Don't use .size() but a custom .outputOption in PeerTube core but the registered scale function (both for VOD and Live videos)
  • Don't provide .videoFilters param for plugins

@TheoLeCalvar
Copy link
Contributor Author

TheoLeCalvar commented Apr 8, 2021

That looks good for me, it enough for my use case (adding hardware transcode).

Add ability for plugins to register a custom scale function that should return { filterName: string, value: string }. The default in PeerTube would be something like { filterName: 'scale', value: 'w=trunc(oh*a/2)*2:h=360' } or { filterName: 'scale', value: 'w=-2:h=360' } (not sure what is the best)

I did a quick search in ffmpeg documentation and it looks like all scale filters support the w=-2:h=XX syntax.
I'm not sure why fluent-ffmpeg uses trunc because -2 does the same.
The ffmpeg documentation explains it here https://trac.ffmpeg.org/wiki/Scaling

So I think it is not strictly necessary for plugins to provide the arguments to the filter since they are always the same in our use case.

Don't provide .videoFilters param for plugins

👍

@Chocobozzz
Copy link
Owner

Okay thanks for the feedback. I'll update this PR.

@Chocobozzz Chocobozzz self-assigned this Apr 8, 2021
@TheoLeCalvar
Copy link
Contributor Author

Ok, thanks.

@Chocobozzz
Copy link
Owner

I updated the PR accordingly. @TheoLeCalvar Could you review & test please?

Copy link
Contributor Author

@TheoLeCalvar TheoLeCalvar left a comment

Choose a reason for hiding this comment

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

Except these small problems it looks good to me.
Tests look good.
I also ported my plugin to this version and it works great.

server/helpers/ffmpeg-utils.ts Show resolved Hide resolved
server/lib/video-transcoding-profiles.ts Outdated Show resolved Hide resolved
support/doc/plugins/guide.md Outdated Show resolved Hide resolved
server/tests/plugins/plugin-transcoding.ts Outdated Show resolved Hide resolved
@Chocobozzz
Copy link
Owner

Thanks!

@Chocobozzz Chocobozzz merged commit 21c917b into Chocobozzz:develop Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PeerTube Plugin 📦 Features that can be developed in a plugin, but require PeerTube plugin API development Component: Transcoding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support inputOptions in ffmpeg plugins
3 participants