Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

[egl] add additional check against eglcontext #723

Merged
merged 1 commit into from Jan 20, 2022
Merged

[egl] add additional check against eglcontext #723

merged 1 commit into from Jan 20, 2022

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jan 19, 2022

This PR adds an additional check against eglContext being null. This avoids having our GL context in an invalid state and as results run into an IllegalArgumentException on the OS level.

@tobrun tobrun added the bug label Jan 19, 2022
@tobrun tobrun self-assigned this Jan 19, 2022
@@ -432,6 +432,10 @@ boolean createSurface() {
Log.e(TAG, "mEglConfig not initialized");
return false;
}
if (mEglContext == null) {
Copy link

Choose a reason for hiding this comment

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

Wonder what the outcome would be, if we skip init here black map?
Also, to help debugging, I'd probably enriched logs a bit, like Could not create surface for drawing, mEglContext not initialized

Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

LGTM
Pretty curious when / why exactly this does happen.
Do I get it correctly that we will try to recreate whole EGL setup when returning false here? Might be pretty much imho, it should be sufficient try to re-acquire context but this solution should work

@yunikkk
Copy link

yunikkk commented Jan 20, 2022

Adreno-GSL: <gsl_ldd_control:553>: ioctl fd 143 code 0xc0080913 (IOCTL_KGSL_DRAWCTXT_CREATE) failed: errno 28 No space left on device the log just before failing context creation.
IOCTL_KGSL_DRAWCTXT_CREATE looks very much like we fail due to missing space (memory or GPU memory, I suppose)

Copy link

@yunikkk yunikkk left a comment

Choose a reason for hiding this comment

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

Seems ok, not sure if the lack of space error from driver will dissappear on the following iterations of the loop.
Otherwise suppose map will just be black, it seems.
Maybe would be a good idea to have some callback/status API that allows developers to show some error message if we fail to initialize the map

@tobrun
Copy link
Member Author

tobrun commented Jan 20, 2022

Maybe would be a good idea to have some callback/status API that allows developers to show some error message if we fail to initialize the map

Let's try this out for mapbox-maps-android? Solution here is to solidify what we have today for the v9 version of the SDK and not a total rewrite or do things differently.

@tobrun tobrun merged commit 85e0875 into main Jan 20, 2022
@tobrun tobrun deleted the tvn-egl-fix branch January 20, 2022 14:57
@yunikkk
Copy link

yunikkk commented Jan 21, 2022

Maybe would be a good idea to have some callback/status API that allows developers to show some error message if we fail to initialize the map

Let's try this out for mapbox-maps-android? Solution here is to solidify what we have today for the v9 version of the SDK and not a total rewrite or do things differently.

Right, that's what I suggested - definitely wouldn't want to introduce this changes to the Hydrogen

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

Successfully merging this pull request may close these issues.

None yet

3 participants