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

Do not allow libyuv to find_package JPEG #2473

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

vrabaud
Copy link
Collaborator

@vrabaud vrabaud commented Oct 9, 2024

This is a better solution to the patch workaround

@vrabaud vrabaud merged commit df2f379 into AOMediaCodec:main Oct 9, 2024
34 checks passed
@@ -29,17 +29,26 @@ else()
if(ANDROID_ABI)
set(LIBYUV_BINARY_DIR "${LIBYUV_BINARY_DIR}/${ANDROID_ABI}")
endif()

# unset JPEG_FOUND so that libyuv does not find it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Thanks for the fix. It would good to explain why we need to do this.

@@ -29,17 +29,26 @@ else()
if(ANDROID_ABI)
set(LIBYUV_BINARY_DIR "${LIBYUV_BINARY_DIR}/${ANDROID_ABI}")
endif()

# unset JPEG_FOUND so that libyuv does not find it
set(JPEG_FOUND_ORIG ${JPEG_FOUND})
Copy link
Collaborator

Choose a reason for hiding this comment

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

If JPEG_FOUND is originally undefined, won't line 48 cause it to be set to an empty string (which is a false constant)?

If no code tests the DEFINED JPEG_FOUND condition, then this is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Internal CMake does not care about JPEG_FOUND being set to empty at least. If you look into FindJPEG.cmake on your machine, it looks for libraries and includes to fill the JPEG_FOUND variable, whether it has been set before or not using find_package_handle_standard_args.

So we are safe

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.

3 participants