GUACAMOLE-1729: Support for non-legally encumbered video codec format in guacenc

[robert-scheck]

Usage: guacenc [-c <mpeg4|libvpx>] recording-file

As of writing, guacenc unfortunately only supports the legally encumbered ("non-free") video codec format "mpeg4", which is, due to its nature, not part of Linux distributions such as Fedora as well as CentOS Stream including its downstreams like Red Hat Enterprise Linux or Rocky Linux. While my implementation might be far from perfect, it actually works for me and adds "libvpx" support in a fully backward-compatible way to guacenc. So maybe this implementation can be at least used as the very first step for possible future codec support?

See also: https://issues.apache.org/jira/browse/GUACAMOLE-1729

[necouchman]

@robert-scheck We definitely appreciate the contributions. All contributions need to have an associated Jira issue (you'll need to get a Jira account by e-mailing the [email protected] list), and then both the pull request and the commits need to be tagged with the Jira issue (see other open pull requests).

[robert-scheck]

@necouchman, I've sent the e-mail shortly after your heads up. And once there is a Jira issue, I'm happy to change this PR and the pull request accordingly.

[robert-scheck]

@necouchman, from my point of view, I've now performed all requested steps. Let me know in case something is missing or something else needs to be done.

[necouchman]

Beyond the one issue I see with the goto block, I'm wondering if we should do some verification that the underlying libav* libraries support the codecs we're trying to add. We currently do not do any checking to make sure that MP4 is supported - I don't know if MP4 is always supported in libav*, or if we're just assuming that, if you have it, you also have it compiled with MP4 support. It seems like maybe there's an opportunity, here, to make this a little more dynamic and verify that we actually support the codecs we're implementing by doing some build-time checks - at the very least to insure that support is included for the ones we have listed, here, if not to dynamically determine which ones we should include.

[robert-scheck]

Beyond the one issue I see with the goto block, I'm wondering if we should do some verification that the underlying libav* libraries support the codecs we're trying to add. We currently do not do any checking to make sure that MP4 is supported - I don't know if MP4 is always supported in libav*, or if we're just assuming that, if you have it, you also have it compiled with MP4 support. It seems like maybe there's an opportunity, here, to make this a little more dynamic and verify that we actually support the codecs we're implementing by doing some build-time checks - at the very least to insure that support is included for the ones we have listed, here, if not to dynamically determine which ones we should include.

Honestly, I'm not sure whether my C programming skills are good enough for this.

What I figured out during my tests:

• If libav* doesn't support MP4, guacenc simply fails with a libav* (?) specific error message (and same applies for possibly missing VPx support). Yes, maybe not great but just works.
• I would like to avoid build-time checks for libav* codec support, because 3rd-party repositories provide alternative libav* with MP4 support for Fedora (suited as run-time drop-in replacements).
• libav* (?) seems to be picky about the filename extension; just passing "libvpx" but still having the .m4v leads to a failure. Not sure where to get proper filename extensions per codec.

[mike-jumper]

Please update src/guacenc/man/guacenc.1.in to document the new option. Other than that, LGTM and I'm happy with things as they stand.

There are some other points I think are worth considering, but which are not deal-breaking:

1. Using a structure to cleanly and directly associate codec names with their suffixes, rather than parallel arrays.
2. Including a human-readable description within said structure such that the "Supported codecs" output doesn't rely on the user knowing the meaning of each codec's internal name.
3. Avoiding logging a redundant ERROR: Unsupported codec! immediately after ERROR: Invalid codec..
4. It might be better to include the supported codecs in the overall usage output, to avoid users having to guess at least one codec before they see a list of what's supported.

[necouchman]

• If libav* doesn't support MP4, guacenc simply fails with a libav* (?) specific error message (and same applies for possibly missing VPx support). Yes, maybe not great but just works.
• I would like to avoid build-time checks for libav* codec support, because 3rd-party repositories provide alternative libav* with MP4 support for Fedora (suited as run-time drop-in replacements).

I see your point, and I'm not hung up on this, but it seems like, since you're building guacd, it'd be better to check for supported codecs at build-time and warn the person building the software about it, and have them have to re-build it, versus silently continuing and getting into a situation where a particular codec just doesn't work and you have to go run down the root cause and then swap the library out.

Also, it matches how the rest of the guacd build process behaves - if you don't have the libssh2 build components available, SSH support is disabled and you have to go install it and then re-build.

Anyway, just my opinion and I'm not hung up about it - it sounds like @mike-jumper is okay with it without the other checks, so that's fine.

• libav* (?) seems to be picky about the filename extension; just passing "libvpx" but still having the .m4v leads to a failure. Not sure where to get proper filename extensions per codec.

As Mike suggested, it might be better to create a structure that associates codecs with extensions to properly track this.

[robert-scheck]

Please update src/guacenc/man/guacenc.1.in to document the new option. Other than that, LGTM and I'm happy with things as they stand.

Oh, good point - will do that.

There are some other points I think are worth considering, but which are not deal-breaking:

1. Using a structure to cleanly and directly associate codec names with their suffixes, rather than parallel arrays.
2. Including a human-readable description within said structure such that the "Supported codecs" output doesn't rely on the user knowing the meaning of each codec's internal name.

As mentioned earlier, I'm not a C programmer, thus I'm not sure how such a structure would like (nor whether I'm able to actually implement it) - sorry.

3. Avoiding logging a redundant ERROR: Unsupported codec! immediately after ERROR: Invalid codec..

What's your suggestion here? Just Supported codecs:?

4. It might be better to include the supported codecs in the overall usage output, to avoid users having to guess at least one codec before they see a list of what's supported.

Do you have any preference? Would [-c <mpeg4|libvpx>] instead be suitable?