-
Notifications
You must be signed in to change notification settings - Fork 302
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
Conversation
There was a problem hiding this 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?
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 |
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? |
* Add Android.bp to replace mk files * Add va_version.h for external reference Tracked-On: OAM-117146 Signed-off-by: zhangyichix <[email protected]>
4ae534d
to
36836b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
36836b6
to
d0a1c1e
Compare
There was a problem hiding this 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
], | ||
|
||
local_include_dirs: [ | ||
".", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Android.bp
Outdated
cc_library_shared { | ||
name: "libva-android", | ||
|
||
static_libs: [ |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Android.bp
Outdated
cflags: [ | ||
"-Wall", | ||
"-Werror", | ||
"-Wno-error", |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
d0a1c1e
to
85560aa
Compare
There was a problem hiding this 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: [ |
There was a problem hiding this comment.
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 onx86_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.
There was a problem hiding this comment.
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\""], |
There was a problem hiding this comment.
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: [ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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]>
85560aa
to
fce7b82
Compare
There was a problem hiding this 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.
Changes include: