Chore: Parenthesis in shell

[iLLiCiTiT]

Changelog Description

Escape parenthesis for shell subprocess when bash or sh are set as shell applications.

Additional info

Content of parenthesis is "executed" in bash so it is necessary to escape them. This was discovered when thumbnail extraction should change resolution and command contained something like (1920-iw).

Testing notes:

I don't know. It requires linux machine and specific settings and case of input image.

[ynbot]

Task linked: AY-6618 FFMPEG thumbnail creation issue for ftrack previews

[BigRoy]

The real question here is maybe also, why are we running this using shell=True instead of without that and passing it just via subprocess as separate arguments?

And is there a chance that shell on Linux may not be bash? nevermind, you are checking shell env var.

And, shouldn't shlex.quote be able to do this otherwise? or doesn't that escape ()?

[iLLiCiTiT]

The real question here is maybe also, why are we running this using shell=True instead of without that and passing it just via subprocess as separate arguments?

Many times asked and aswered question. Because we're receiving ffmpeg arguments from settings, and to be able to "safely" use them as list, we would have to force TDs to fill them correctly (single arg as single string value ["-c:v libx264"] -> ["-c:v", "libx264"], or hare 100% reliable way how to split them. I didn't find one, shlex is not really good at it. We would have to have own OS specific implementation, and probably few iterations of the implementation. Anyway that would break productions few times before we get it correctly (not fun). We tried in past BTW.

From our experience they test setup of the output using cli and then just paste to settings whatever they come to.

[BigRoy]

I didn't test it - but I can imagine this may fix the issue at hand. So fine with me.

I do think bash can be the shell even from another path then /bin/bash which may not be 'the usual standard' but as far as I know still reasonable usage on Linux. @martastain may potentially bash me though when saying that.

However, I personally dislike the code duplication for this. It's not crazy, yet at the same time still annoying.

And I still think it's likely worth looking for a solution to that darned joined string. 🗡️ But maybe not now as hotfix - but tracked as separate issue.

[iLLiCiTiT]

However, I personally dislike the code duplication for this. It's not crazy, yet at the same time still annoying.

Well, it probably can be moved to run_subprocess, not a big deal for me.

[BigRoy]

Code looks good to me. Didn't test it though (no linux)