Skip to content
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

Add libavif/0.9.3 #9225

Merged
merged 7 commits into from
Feb 27, 2022
Merged

Conversation

friendlyanon
Copy link
Contributor

Specify library name and version: libavif/0.9.3

Issue: #8618


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

#include <avif/avif.h>
#include <stddef.h>

int main(int argc, char const* argv[])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard prototype of the main() function does not have the "const":

int main(int argc, char* argv[])

See https://en.cppreference.com/w/c/language/main_function

Also, there is an alternative prototype that you can use here to avoid the (void)argc and (void)argv casts.

int main(void)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All true, I just had such a template I have been using to make recipes. Adding const is also inconsequential in C.
Regardless, I have edited my template and will avoid doing this in the future.

required_conan_version = ">=1.43.0"

codecs = {
"aom": "libaom-av1/3.1.2",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current libaom release is 3.2.0. Is libaom 3.2.0 available in Conan?

Similarly for dav1d. The current dav1d release is 0.9.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are the latest versions currently packaged in CCI.

def _configure_cmake(self):
cmake = CMake(self)
suffix = str(self.options.with_codec).upper()
cmake.definitions[f"AVIF_CODEC_{suffix}"] = True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For libaom, you can additionally pass -DAVIF_CODEC_AOM_DECODE=OFF to cmake so that libaom is only used as an AV1 encoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's an option worth exposing here. What is the value proposition of that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If -DAVIF_CODEC_AOM=ON is passed to cmake, libaom can be used as both the AV1 encoder and the AV1 decoder. If -DAVIF_CODEC_DAV1D=ON is also passed to cmake, both dav1d and libaom can be used as AV1 decoders, with dav1d preferred over libaom. It suffices to just have dav1d as the AV1 decoder. That's why I suggested passing -DAVIF_CODEC_AOM_DECODE=OFF to cmake.

The code disabled by -DAVIF_CODEC_AOM_DECODE=OFF is mainly in libavif/src/codec_aom.c, some in libavif/src/avif.c. You can evaluate it and see if it's worth making recipes/libavif/all/conanfile.py more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the value proposition here is that when possible, libavif prefers dav1d, because it's better in some way? If so, I guess it's no biggie to create separate with_encoder and with_decoder options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I had wrong assumptions. I implemented with_decoder only instead with the encoder being aom always.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@friendlyanon I somehow forgot to reply to your last question. Sorry. Yes, as an AV1 decoder, dav1d has much better performance than libaom.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I assumed as such, that's why I set it as the default decoder. Thanks for confirming!

@conan-center-bot

This comment has been minimized.

The part checking for the version is touching code that is too out of
date in the source tarball and it's not essential either.
@conan-center-bot

This comment has been minimized.

The previous option was implemented based on false assumptions. To
encode, there is only AOM in CCI, but the decoder can be swapped to
Dav1d, which upstream prefers when available, so it's implemented as the
default decoder in the recipe.
@conan-center-bot
Copy link
Collaborator

All green in build 6 (a2fb8771d6b55f6a4ff57001b5c4d5a1d15f2728):

  • libavif/0.9.3@:
    All packages built successfully! (All logs)

@functools.lru_cache(1)
def _configure_cmake(self):
cmake = CMake(self)
cmake.definitions["AVIF_CODEC_AOM"] = True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If self.options.shared is true, should we pass -DBUILD_SHARED_LIBS=ON to the cmake command for libavif?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's handled automatically by Conan.


self.cpp_info.libs = ["avif"]
if self.options.shared:
self.cpp_info.defines = ["AVIF_DLL"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<avif/avif.h> has the following comment:

// AVIF_DLL should be defined if libavif is a shared library. If you are using
// libavif as CMake dependency, through CMake package config file or through
// pkg-config, this is defined automatically.

Since you seem to be defining AVIF_DLL manually, does this mean you are not using libavif as CMake dependency or through pkg-config?

Copy link
Contributor Author

@friendlyanon friendlyanon Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake package configs generated by projects are discarded as per Conan Center Index policy. Conan regenerates CMake package configs at install time using its own generators.

def _configure_cmake(self):
cmake = CMake(self)
suffix = str(self.options.with_codec).upper()
cmake.definitions[f"AVIF_CODEC_{suffix}"] = True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@friendlyanon I somehow forgot to reply to your last question. Sorry. Yes, as an AV1 decoder, dav1d has much better performance than libaom.

Copy link

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@friendlyanon I reviewed the latest files (to the best of my abilities because I am not familiar with Conan). I have two questions.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@conan-center-bot conan-center-bot merged commit 92ac73c into conan-io:master Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants