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

Don't crash when attribute count is exceeded #11656

Merged
merged 3 commits into from
May 3, 2018

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Apr 11, 2018

With data driven styling, we are dynamically computing shaders with a varying number of vertex attributes. One caveat of that is that the minimum mandated number for vertex attributes in the OpenGL ES 2.0 spec is only 8, as we discovered. Most modern GPUs have a higher limit than that, but there are some older mobile GPUs that only supply the bare minimum (see #9331). @jfirebaugh reworked this in #9433 to limit the number of vertex attributes we're using.

We're throwing with a "too many vertex attributes" error in this case. This PR changes that so instead, we're printing a warning message when the user creates a style that has more attributes than the minimum limit, but we'll still bind them if the hardware supports it. If it doesn't, we won't bind the attributes beyond the hardware limit and instead accept the rendering errors. While this is unfortunate, it's still better than crashing.

@kkaefer kkaefer requested a review from jfirebaugh April 11, 2018 11:43
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Can we make the warning logic a bit more specific?

  • Logging the ID of the layer that has too many data-driven properties would be a big improvement.
  • Customize the message based on maxAttributes:
    1. Get GL_MAX_VERTEX_ATTRIBS value first.
    2. If location > maxAttributes, warn with:

    The layer <ID> uses more data-driven properties than the current device supports, and will have rendering errors. To ensure compatibility with this device, use fewer data driven properties in this layer.

    1. Else if location > MAX_ATTRIBUTES, warn with:

    The layer <ID> uses more data-driven properties than some devices may support. Though it will render correctly on this device, it may have rendering errors on other devices. To ensure compatibility with all devices, use fewer data-driven properties in this layer.

…ashing

Instead, we're printing a warning message and won't bind the attributes beyond the limit. This will cause rendering errors, but it's still better than crashing.
@kkaefer
Copy link
Member Author

kkaefer commented Apr 13, 2018

I made changes to accommodate your request. Since it'd be tricky to loop through the layer name and ensure that the warning would only be printed once per layer during the actual attribute binding, I chose a strategy that uses Attributes::Bindings to determine how many active attribute bindings we have when rendering a layer. This necessitated factoring out the AttributeValues so that they can be accessed while rendering a layer.

@lbud lbud force-pushed the attribute-binding-crash branch from 2183279 to 6133b2a Compare May 3, 2018 22:42
@lbud lbud merged commit c3346f4 into release-boba May 3, 2018
@lbud lbud deleted the attribute-binding-crash branch May 3, 2018 23:31
@lbud
Copy link
Contributor

lbud commented May 3, 2018

FYI @kkaefer I fixed a few compile errors and merged here per @lilykaiser's request 🙇‍♀️

@lilykaiser
Copy link

Thanks @lbud !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl crash rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants