I don't think this patch makes sense as is. ext/dtls can currently *only* work with openssl (port to optionally use gnutls pending). ext/hls can work with nettle or libgcrypt, but not openssl.

Created attachment 311722 [details] [review]
Patch that adds --with-hls-crypto=auto|nettle|libgcrypt option

This patch adds a --with-hls-crypto=auto|nettle|libgcrypt option in configure.ac.

Hi,

Sorry, I thought ext/hls would also work with openssl as the code shows it is used if nettle and libgcrypt are not found.

Anyways, since openssl is not used for ext/hls, it would be nice to have an option to build hls from source using either nettle or libgcrypt. Something like that:

--with-hls-crypto=auto|nettle|libgcrypt

I have updated another patch just with that option.

Sorry my fault, hlsdemux indeed has openssl support. But having this configure option called --with-hls-crypto definitely makes more sense.

Created attachment 314847 [details] [review]
Patch that adds --with-hls-crypto=auto|nettle|libgcrypt|openssl option

Hi,

Sorry for the delay. Since hlsdemux has support for openssl, I have updated the patch to also accept openssl as a valid value:

--with-hls-crypto=auto|nettle|libgcrypt|openssl

Will that be merged into master?

Comment on attachment 314847 [details] [review]
Patch that adds --with-hls-crypto=auto|nettle|libgcrypt|openssl option

Thanks for the new patch. Instead of AS_IF this should probably be an AS_CASE? The runaway indentation is a bit ugly :)

Also if you have suggestions how we can not duplicate each check for the auto case that would be great

Created attachment 314861 [details] [review]
Patch that adds --with-hls-crypto=auto|nettle|libgcrypt|openssl option

Hi Sebastian,

Thanks for your review. I totally agree that the indentation looks ugly with so many AS_IF, but unfortunately, the logic is quite complex in that particular case. I Added an AS_CASE as you suggested and it definitely looks better.

I also tried to remove duplicate checks by doing something like:

    if ( with_hls_crypto=="nettle" || (with_hls_crypto=="auto" && PKG_CHECK_MODULES("nettle")) ) {
    } else if...

but, as far as I can tell, I think it is not possible to *compose* an AS_IF with a PKG_CHECK_MODULES together. To be honest, I am not really familiar with M4 and there is not much documentation out there :(

Review of attachment 314861 [details] [review]:

::: configure.ac
@@ +3042,3 @@
+    which cryptographic library version to compile against for hls (default: auto)
+  ]),
+    [case "$with_hls_crypto" in

You could use AS_CASE here too

@@ +3060,3 @@
+    [nettle], [
+      AC_DEFINE(HAVE_NETTLE, 1, [Define if nettle is available])
+      HAVE_HLS="yes"

Each of these cases should also check for the actual availability of the corresponding library, and if it does not exist error out.

I don't see a way either to not have the checks twice (here and then again for the auto case), so let's just go with that for now.

Created attachment 314904 [details] [review]
Patch that adds --with-hls-crypto=auto|nettle|libgcrypt|openssl option

OK, Here is a updated version of the patch with error checks and AS_CASE macro for the first case statement.

Let me know if it looks OK or you want me to add/change more things.

commit 44a5fbe8e0fa70747c7d191828d4822f82cea38a
Author: Julian Bouzas <julian.bouzas@vcatechnology.com>
Date:   Thu Nov 5 10:12:41 2015 +0000

    configure.ac: Added --with-hls-crypto=auto|nettle|libgcrypt|openssl option to build the source using the desired cryptographic library for HLS
    
    https://bugzilla.gnome.org/show_bug.cgi?id=755319