add handbrake rockon

[mikey0000]

To ease and accelerate the PR submission, please use the template below to provide all requested information.
You can find guidelines on which docker image to use in the Rockstor's documentation,
as well as examples of proper formatting in other rock-ons (here
and here)

Fixes # .

General information on project

This pull request proposes to add a new rock-on for the following project:

• name: Handbrake
• website:
• description:

Information on docker image

• docker image:
• is an official docker image available for this project?:

Checklist

• Passes JSONlint validation
• Entry added to 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

[mikey0000]

I'll work my way through the checklist but I have this running on my rockstor, its pretty awesome.

[phillxnet]

@mikey0000 Thanks for getting started on this. Would be a 'nice to have' and well done getting it up and running already.

[FroggyFlox]

Thanks @mikey0000 !

It is indeed a very good addition to make, and has actually been requested on our forum in the past. I'm linking two of these below for reference so that we can update those threads as needed.

• https://forum.rockstor.com/t/request-rockon-handbrake/1963
• https://forum.rockstor.com/t/handbrake-running-in-docker-container/6377

Thanks

I'll work my way through the checklist

Thank you for that! In case this helps, feel free to use other "up-to-date" rock-ons such as nextcloud-official.json or ecodms18-09.json as reference for the style formatting of the description and more_info fields.

Thanks again!

[mikey0000]

Hi Guys, updated and checklist checked, Minor change that I've added the VNC port as well since that is pretty nice to have in there by default. Please review and let me know if I've missed anything.

Oops need to update the description better. BRB
[EDIT:] I'm back, updated description and found you missed the end p tag in nextcloud.json, review away.

[phillxnet]

@mikey0000 Thanks for the permissions fixes. Been meaning to do this for ages. Although our deployment system sorts it on the fly it's best by way of example if the repo at least has consistency in this regard. It's generally better all around to try and avoid 'mission creep' within a pr, especially if we have to revert it's changes, but no worries; and will be nice to have this niggle finally sorted.

Nice find re the closing paragraph, and same goes for the 'mission creep' but again, nice to have this found and fixed.

Thanks for the various improvement, hopefully we can get this reviewed and in pretty soon.

Incidentally, you can tick, via GitHub web interface, the checklist: no need to edit manually. They then shows up as ticks and an associated progress meter is drawn in the pull requests overview.

[mikey0000]

Ah was wondering that, thanks. Yeah the mission creep was a oh I can see this needs changing, as I was working. Papercuts I'd call them. Wouldn't have been hard to turn into a separate pr, totally understand. I at least made the chmod a separate commit for that reason. Anyway, thanks for the quick responses. 25/01/2020 11:06:49 pm Philip Guyton <notifications@github.com>:

…

@mikey0000 [https://github.com/mikey0000] Thanks for the permissions fixes. Been meaning to do this for ages. Although our deployment system sorts it on the fly it's best by way of example if the repo at least has consistency in this regard. It's generally better all around to try and avoid 'mission creep' within a pr, especially if we have to revert it's changes, but now worries; and will be nice to have this niggle finally sorted. Nice find re the closing paragraph, and same goes for the 'mission creep' but again, nice to have this found and fixed. Thanks for the various improvement, hopefully we can get this reviewed and in pretty soon. Incidentally, you can tick, via GitHub web interface, the checklist: no need to edit manually. They then shows up as ticks and an associated progress meter is drawn in the pull requests overview. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [#210?email_source=notifications&email_token=AALDOPOZKVJNQFBFTRHFMF3Q7QFINA5CNFSM4KK4DMP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ4ZLBY#issuecomment-578393479] , or unsubscribe [https://github.com/notifications/unsubscribe-auth/AALDOPJCBFMCZGLBJV5UOTDQ7QFINANCNFSM4KK4DMPQ] . [https://github.com/notifications/beacon/AALDOPJHUYQD7QJ7ZCGWQX3Q7QFINA5CNFSM4KK4DMP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ4ZLBY.gif]

[FroggyFlox]

Hi @mikey0000 ,

Thanks a lot for writing this rock-on, it does look really nice!
As you can see, I made a few suggestions for minor style formatting as well as little quirk with the UI url slug. I also have another suggestion to make but I'll make a dedicated comment for this.

Thanks again!

[FroggyFlox]

Hi again, @mikey0000 ,

As mentioned in my review, I had another point I wanted to discuss: support for hardware acceleration via Intel QuickSync.

This point was brought up in one of the forum links I included in my earlier message, and I was thus thinking we may benefit from adding it to this rock-on. We can indeed support this by using the "devices" object. As this is optional, users without a compatible CPU can simply leave the corresponding field empty at installation and not be impacted. Those with compatible CPU, though, should be able to take advantage of their hardware.
What do you think?

It would simply require adding the device as we already do in the EmbyServer rock-on, for instance:

rockon-registry/embyserver.json

Lines 13 to 18 in de26799

	"devices": {
	"VAAPI": {
	"description": "<u>Optional:</u> path to hardware transcoding device (/dev/dri/renderD128). Leave blank if not needed.",
	"label": "VAAPI device"
	}
	},

[mikey0000]

Yeah I'll add that. Was also looking at the nvenc stuff too. But I'd have to put my gpu into my server to try it, not sure what kind of gain or how many actually want it. 26/01/2020 5:49:43 am FroggyFlox <notifications@github.com>:

…

Hi again, @mikey0000 [https://github.com/mikey0000] , As mentioned in my review, I had another point I wanted to discuss: support for hardware acceleration via Intel QuickSync. This point was brought up in one of the forum links I included in my earlier message, and I was thus thinking we may benefit from adding it to this rock-on. We can indeed support this by using the "devices" object. As this is optional, users without a compatible CPU can simply leave the corresponding field empty at installation and not be impacted. Those with compatible CPU, though, should be able to take advantage of their hardware. What do you think? It would simply require adding the device as we already do in the EmbyServer rock-on, for instance: https://github.com/rockstor/rockon-registry/blob/de2679906e4972468773d0bbc036fdf60775b96e/embyserver.json#L13-L18 [https://github.com/rockstor/rockon-registry/blob/de2679906e4972468773d0bbc036fdf60775b96e/embyserver.json#L13-L18] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [#210?email_source=notifications&email_token=AALDOPMLBCZWRUV35LKX7CLQ7RUPJA5CNFSM4KK4DMP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ5AKLI#issuecomment-578422061] , or unsubscribe [https://github.com/notifications/unsubscribe-auth/AALDOPITDYGVJ5QG6CVT5ULQ7RUPJANCNFSM4KK4DMPQ] . [https://github.com/notifications/beacon/AALDOPL6INEGC5RYIIWVZC3Q7RUPJA5CNFSM4KK4DMP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ5AKLI.gif]

[FroggyFlox]

Was also looking at the nvenc stuff too. But I'd have to put my gpu into my server to try it, not sure what kind of gain or how many actually want it

I agree with you about that: not sure it's worth the trouble. Plus, from memory, one would also need to run docker with the nvidia runtime which is not currently doable from Rockstor's UI so I don't think this is something we should try to support from the rock-on json. Note that we recently changed how we deal with the docker daemon (see rockstor/rockstor-core#2090), so this level of configuration is now more easily thinkable in the future, though (see rockstor/rockstor-core#2088 (comment)).

Thanks again for your contributions!

[mikey0000]

@FroggyFlox I added the devices json and re-installed handbrake and didn't see the devices option in the setup flow? Any ideas help. My Server CPU is a AMD 3400G so has integrated graphics so I can test this out.
EDIT: NVM my derp, all fine.

[FroggyFlox]

@FroggyFlox I added the devices json and re-installed handbrake and didn't see the devices option in the setup flow? Any ideas help.

Thanks a lot for implementing this. I do see the option displayed during the rock-on install wizard. As to why you do not, I can think of a couple things:

1. Have you un-installed the handbrake rock-on (if installed), and then clicked on the "Update" button in the rock-ons page? This is required to refresh the database from the newly-updated handbrake.json file.
2. What version of rockstor was this tested on? Source or rpm install? In any case, the support for devices was added in Rockstor 3.9.2-39, so you need to have this version minimum to see it. (see Rockstor release changelogs

Thanks again for your work, it does look good after a quick, but I'll need to double check myself later tonight.

Hope this helps,

[mikey0000]

Just needed to refresh 🤦‍♂

[mikey0000]

anything left to resolve here?

[FroggyFlox]

Hi @mikey0000 ,

Sorry for not getting back with you earlier than that.

I only have a couple minor suggestions left but that should be good after that.

Thanks a lot for your time and effort on this one. I'm sorry for seemingly nitpicking on these little things, but I believe they'll be much worth it in the end as your rock-on will represent a very good addition that I believe will be quite well received.

Thanks again,

[phillxnet]

@mikey0000 and @FroggyFlox I'll do a final test on this soon and hopefully get it merged and published directly thereafter.

[mikey0000]

@FroggyFlox I've updated the more info, I think this represents what you were asking, please review and let me know otherwise 🥇

[phillxnet]

@mikey0000 I think a force push has confused me a little on this pull request so me and @FroggyFlox have had a little look and think it best we merge as is as it's now a really long tail. Plus it's 99% there now. Apologies for that, we just want to get it right before publish so I'm going to merge as-is and hopefully @FroggyFlox can then more easily do a tidy as he sees fit on another pull request before we actually publish.

Thanks to both of you for getting this done. Merging as is to simplify applying the finishing touches before publishing. I'll update this pr to indicate when it's actually published.