-
Notifications
You must be signed in to change notification settings - Fork 98
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
Update HandBrake rock-on device description, Fixes #212: #213
Update HandBrake rock-on device description, Fixes #212: #213
Conversation
- remove unsupported <u> html tags - replace mentions of VAAPI with Intel Quick Sync - replace example device path to the Interl Quick Sync one - minor edit of "more_info" field
"VAAPI": { | ||
"description": "<u>Optional:</u> path to hardware transcoding device (/dev/dri/renderD128). Leave blank if not needed.", | ||
"label": "VAAPI device" | ||
"quicksync": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my only thing here is future support for AMD APU's. But I guess that can become a TODO:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikey0000 , thanks a lot for your prompt feedback!
Is VAAPI actually supported already? I could only see mention of QuickSync in the image's doc so my understanding was that only QuickSync was supported so far.
If VAAPI is supported as well, please accept my apologies and I'll revert these changes and put the QuickSync label in in addition to rather than instead of VAAPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I meant I'll need to look at adding support for AMD GPU/APU's as @phillxnet said can make the adjustment in a revised version.
Thanks for the adjustments, looks good to me. |
@FroggyFlox and @mikey0000 I vote for this being merged and releases as is, especially given the Rock-ons current limbo status of merged but not yet published. I.e. we merge this pr and publish the Rock-on so others are in a position to try it out and chip in where needed. @FroggyFlox Given @mikey0000 looks to have reviewed your changes (but with a potential future caveat) I think we are good to go on this one. Do you agree? If so I'll merge and publish right away. |
Sounds good to me! Thanks! |
@FroggyFlox and @mikey0000 This is very nice, and likely to be quite a welcome addition to the Rock-on family. I was quite surprised to see the interface pretty much just as it is on a linux desktop but in the browser. Fancy. Happy to merge. |
@FroggyFlox @mikey0000 Apologies for taking so long on the merging and publishing of this. But it's now published. Neat that it can also use the quick sync 'go faster strip'. Haven't tried this yet though. Thanks again for both of your efforts on getting this one in. Nice addition. |
Thanks @phillxnet for such a quick turn-around, and thanks @mikey0000 for your rock-on and feedback! |
Fixes #212 .
General information on project
This pull request proposes to update an existing rock-on for the following project:
Information on docker image
Summary of changes
Checklist
root.json
in alphabetical order (for new rock-on only)"description"
object lists and links to the docker image used"description"
object provides information on the image's particularities (advantage over another existing rock-on for the same project, for instance)"website"
object links to project's main website@phillxnet , @mikey0000 , ready for review.