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 Android.bp for libva 2.22 #835

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JeevakaPrabu
Copy link

@JeevakaPrabu JeevakaPrabu commented Sep 25, 2024

Changes include:

  • Replace Android.mk with Android.bp for libva 2.22
  • Add pre-generated va/va_version.h and remove it from .gitignore

Copy link
Contributor

@XinfengZhang XinfengZhang left a comment

Choose a reason for hiding this comment

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

va_version.h is generated under meson and make, why change to pre-generated under android?

@JeevakaPrabu
Copy link
Author

va_version.h is generated under meson and make, why change to pre-generated under android?

Tried to generate the va/va_version.h from Android.bp but met with some road blocks. So for the time being added the pre-generated va_version.h

@XinfengZhang
Copy link
Contributor

XinfengZhang commented Sep 25, 2024

I still hope we could generate va/va_version.h from android.bp, then the behavior will be aligned between linux/win/android. @rosetta-jpn any comments?

@XinfengZhang
Copy link
Contributor

@xuguangxin

* Add Android.bp to replace mk files
* Add va_version.h for external reference

Tracked-On: OAM-117146
Signed-off-by: zhangyichix <[email protected]>
@JeevakaPrabu JeevakaPrabu force-pushed the android_bp branch 6 times, most recently from 4ae534d to 36836b6 Compare September 26, 2024 12:50
Android.bp Outdated Show resolved Hide resolved
Android.bp Outdated Show resolved Hide resolved
Copy link

@theandi666 theandi666 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andrescj-chromium andrescj-chromium left a comment

Choose a reason for hiding this comment

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

Looks essentially good to me. Thanks Jeevaka!

Android.bp Outdated Show resolved Hide resolved
Android.bp Show resolved Hide resolved
Android.bp Outdated
],

local_include_dirs: [
".",
Copy link
Contributor

Choose a reason for hiding this comment

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

The "." include doesn't seem necessary. I tested locally and libva still builds without it.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Android.bp Outdated Show resolved Hide resolved
Android.bp Show resolved Hide resolved
Android.bp Show resolved Hide resolved
Android.bp Outdated
cc_library_shared {
name: "libva-android",

static_libs: [
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you can remove this entire static_libs.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Android.bp Outdated Show resolved Hide resolved
Android.bp Outdated
cflags: [
"-Wall",
"-Werror",
"-Wno-error",
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it would cancel out the effect to -Werror.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Android.bp Show resolved Hide resolved
Copy link
Contributor

@andrescj-chromium andrescj-chromium left a comment

Choose a reason for hiding this comment

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

Approved from my end % changes.

"va/drm/va_drm_utils.c",
],

cflags: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Some questions/comments to maybe simplify these flags:

  • "-D_FILE_OFFSET_BITS=64": isn't this implied by the fact that this module is only enabled on x86_64? According to this: "On 64 bit systems this macro has no effect since the *64 functions are identical to the normal functions."
  • -Wall: this can be removed because it's globally enabled. See this.
  • -O2: this is also globally enabled. See this.
  • -fPIC: the fact that this is a shared library module should imply position-independent code. See also this.
  • -DANDROID: this can be removed because it's globally enabled. See this.

Additionally, you had -Werror before. Consider adding that back.

Copy link
Author

Choose a reason for hiding this comment

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

Recommended change done


arch: {
x86_64: {
cflags: ["-DVA_DRIVERS_PATH=\"/vendor/lib64\""],
Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

"va/drm/va_drm_utils.c",
],

cflags: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above about the flags.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Android.bp Show resolved Hide resolved
Changes include:
- Updated Android.bp to add license package and enable it only for
x86_64
- Added genrule to generate the va_version.h.
- Removed va_version.h

Signed-off-by: JeevakaPrabu <[email protected]>
Signed-off-by: Andreas Huber <[email protected]>
Copy link
Contributor

@andrescj-chromium andrescj-chromium left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Also, I tested building libva with this PR on AOSP main and it works fine.

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.

5 participants