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 with_asm and with_opencl options in the x264 recipe #22432

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anthonyliot
Copy link

Specify library name and version: libx264/cci.20220602

Add in the two options with_asm and with_opencl. When building on Android this two flags can be required.


Signed-off-by: Anthony Liot <anthony.liot@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 2 (5a399b06c8feff8f7097f728151cea7bfbf1a6a0):

  • libx264/20191217:
    All packages built successfully! (All logs)

  • libx264/cci.20220602:
    All packages built successfully! (All logs)

  • libx264/20190605:
    All packages built successfully! (All logs)


Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

See details:

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

Comment on lines +28 to +29
"with_opencl": [True, False],
"with_asm": [True, False]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use enable_opencl and enable_asm, personally, since with_* is typically used for options controlling external dependencies on CCI. Matches the project option names better as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because OpenCL is enabled for bit_depth == 8, you should add a validation check for self.options.with_opencl and self.options.bit_depth != 10: https://github.com/mirror/x264/blob/eaa68fad9e5d201d42fde51665f2d137ae96baf0/configure#L1409

Copy link
Contributor

@valgur valgur left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor suggestions.

It's worth noting that the OpenCL feature does not require OpenCL libraries to be present. These are only loaded at runtime, if available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants