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

layers/android_ndk_types.h uses Pie symbols not present in Oreo #623

Closed
ShabbyX opened this issue Jan 24, 2019 · 15 comments

Comments

Projects
None yet
7 participants
@ShabbyX
Copy link
Contributor

commented Jan 24, 2019

This is causing build failures in ANGLE such as seen here: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8923432294563978896/+/steps/compile__with_patch_/0/stdout

Some examples of undefined symbols are AHARDWAREBUFFER_FORMAT_D16_UNORM and AHARDWAREBUFFER_USAGE_GPU_MIPMAP_COMPLETE.

hardware_buffer.h in Oreo: http://androidxref.com/8.1.0_r33/xref/frameworks/native/libs/nativewindow/include/android/hardware_buffer.h

hardware_buffer.h in Pie: http://androidxref.com/9.0.0_r3/xref/frameworks/native/libs/nativewindow/include/android/hardware_buffer.h

@mark-lunarg mark-lunarg added the bug label Jan 24, 2019

@mark-lunarg mark-lunarg added this to the P1 milestone Jan 24, 2019

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Sure enough, those symbols are not in the linked 8.1 header. It confuses me that we don't see this in our CI builds. We specify platform-26 (i.e. 8.0) in Application.mk, use target- and min-platform 26 in the manifest, and specify 26 to aapt in the build_all.sh script - which I assumed meant we were using the 8.0 header files. @cnorthrop, not so?

Anyway, those symbols are called out by name in the Vulkan spec, so it seems like the only way to resolve this will be to guard the AHB extension code with some sort of ANDROID_MIN_PLATFORM_28-ish compile symbol. Ping @critsec for any input on restricting to Pie or better, or is there another work-around to consider?

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

@karl-lunarg, its not the subject of this issue, but if you look at the linked build spew there are also some missing symbols in gpu_validation.cpp. Maybe just a chromium build config issue, but pinging you just as a heads-up.

@karl-lunarg

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

@daveh-lunarg That was discussed in #619.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

@ShabbyX , without a version symbol in the header file itself, what's the suggested method to guard against compilation with an pre-9.0 header?

It seems like #ifdef __ANDROID_API_P__ would be sufficient to guarantee the enum's existence in the header file, but at the risk of a run-time error if using P headers to compile for an older target. There's also an #define __NDK_MAJOR__ 18 symbol in ndk-version.h that could also guarantee the enum but probably has the same concern.

I also see some of this: #if __ANDROID_API__ >= 28, but that also seems risky given that the API symbol is set by compiler arguments that potentially know nothing about which headers are installed - could potentially set API 28 w/ 8.1 headers?

Assuming you hit this issue from time to time - what's the preferred fix?

@ShabbyX

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

I actually have little Android expertise. @tobine do you have any input?

@tobine

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

ANDROID_API is the target platform version. A recommended pattern that I found is something like:

#if defined(__ANDROID_API__) && __ANDROID_API__ >= __ANDROID_API_P__

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

I have a PR that should fix the issue for Android 8.0 / 8.1. We have a dependency on android/hardware_buffer.h, which I think first appeared in 8.0, so it's still not going to work on an NDK distribution that's older than 8.0.

@ShabbyX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Thank you!

@ShabbyX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

I would like to reopen this issue please. VVL unconditionally uses AHardwareBuffer_* functions which are non-existent on NDK version 24. In Chromium (ANGLE), we still build and test on Nexus 5X and we would need VVL to be able to build on that platform.

A possible course of action would be for VVL to detect whether AHardwareBuffer is supported and compile out code related to AHardwareBuffer extensions.

@null77

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

It would be much appreciated if we could continue to support compilation on API level 24 (Android O). The Chromium infrastructure is stuck on this version because of the complexity of upgrading to the Pixel 2 for testing. They're in the process of doing the upgrade but it will be some time before it is complete and we can drop 5x support.

An option to disable these tests at compile-time would be welcome so we can continue to update our versions of the layers and receive bug fixes while we use the 5X.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Ok, I think I can make API 24 (i.e. release 7.0, Android N) compile without adding a repugnant number of #ifdefs. There are already a full dummy set of all the required symbols in android_ndk_types.h for use with MockICD testing, so I think I can just enable them on Android for any pre-O API level. Unfortunately AFAICT the __ANDROID_API_N__ symbol didn't actually appear until API 26 (O), but I'll just assume API 24 if the O symbol is missing.

This approach will define some dummy functions for AHardwareBuffer_allocate, AHardwareBuffer_release and AHardwareBuffer_describe, but they won't be visible externally so don't think they should cause any problems.

@null77

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

OK, thanks Dave. I'm also looking at upgrading our SDK level to 26. You're right. "O" matches 26 and "N" matches 24. We should probably be able to upgrade to 26.

@null77

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Dave, we are now using SDK level 26 on our integration without issue. So I'm I think we're good to go without any further action on your part. It may still be worth providing backwards compatibility for N. But I think we're unblocked now.

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Ok, @null77. We'd previously agreed with @tobine, @cnorthrop that API 26 was as far back as we really need to go, as the Vulkan support on Android N is somewhat problematical anyway. If you guys are also happy with O-or-better, then I'll probably just let this lie.

@ShabbyX

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

I can confirm that we are ok with 26 as far as building ANGLE is concerned.

@shannon-lunarg shannon-lunarg modified the milestones: P1, sdk-1.1.101.0 Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.