[core] Rework attribute binding (again) #9433

jfirebaugh · 2017-07-06T15:26:59Z

These changes are necessary for programs whose set of active attributes is not fixed at compile time by a template parameter pack, but rather varies based on the generated shader text at runtime. In such cases, the attribute location of a given named attribute may vary between instances of the same Program.

Previously, attribute bindings were implicitly associated with a location based on template parameter order, and -1 was used to indicate an inactive attribute. This left us unable to disable the appropriate attribute when it went from active to inactive. Now, the state tracker for bindings explicitly associates locations and state, and an empty optional is used to indicate an inactive attribute.

In addition, a gl::VertexArray class is now exposed, allowing more flexibility in the relationship between Programs, Segments, and attribute bindings. The final commit in this PR adjusts that relationship to match gl-js, reduce rebinds, and work around buggy VAO implementations.

VertexArray uses a pimpl idiom in order to support implementations that lack the VAO extension. In that case, all VertexArrays share global binding state, reflecting the platform reality in the absence of VAOs, while still providing a uniform API.

Fixes #4681
Fixes #8658
Fixes #9406

Compatibility test locally with Context::supportsVertexArrays body replaced with return false.
Fix CI
Verify Rendering artifacts with PowerVR SGX544 GPUs #8658 on device
Merge Add regression test for https://github.com/mapbox/mapbox-gl-native/issues/9406 mapbox-gl-js#4926
Port to release branch ~~(or at least a minimal fix for Rendering artifacts with PowerVR SGX544 GPUs #8658)~~: Backport attribute binding changes to release branch #9440

jfirebaugh · 2017-07-06T15:27:45Z

iOS bench results:

Result:
| paris      | 59.9 fps |
| paris2     | 60.1 fps |
| alps       | 60.0 fps |
| us east    | 60.4 fps |
| greater la | 60.2 fps |
| sf         | 60.3 fps |
| oakland    | 60.4 fps |
| germany    | 58.3 fps |
Total FPS: 479.7
Average FPS: 60.0

jfirebaugh · 2017-07-06T15:54:18Z

Rendering test failure on regressions/mapbox-gl-js#4860. @ChrisLoer is this getting an ignore or fix soon?

ChrisLoer · 2017-07-06T16:38:17Z

Oops, sorry about that, I noticed that was a failure on native yesterday but hadn't submitted a PR yet: mapbox/mapbox-gl-js#4947

lbud · 2017-07-08T00:05:24Z

😅

I don't want to pretend that my own ✅ represents anything more than a very vague understanding of the actual implementation here, but it sounds good to me.

Also, without it (on master) I'm getting this error in api-gl:

    [Fri, 07 Jul 2017 20:06:25 GMT] [info] [visual-test] 14/3616/6065 - tiles-traffic-night-v2
libc++abi.dylib: terminating with uncaught exception of type mbgl::gl::Error: glDisableVertexAttribArray(location): Error GL_INVALID_VALUE at mapbox-gl-native/src/mbgl/gl/attribute.cpp:55

(Rebasing on this branch fixes it.)

tobrun · 2017-07-10T07:04:33Z

tested this PR on a Samsung Galaxy J3 (2016) and hitting java.lang.Error: array::at when showing a map. Verfied that this device works on master. Emulator is not showing this issue. Below the full stacktrace though not very actionable.

7 - 10 08: 53: 25.839 5402 - 5402 / com.mapbox.mapboxsdk.testapp E / AndroidRuntime: FATAL EXCEPTION: main
Process: com.mapbox.mapboxsdk.testapp, PID: 5402
java.lang.Error: array::at
at com.mapbox.mapboxsdk.maps.NativeMapView.nativeRender(Native Method)
at com.mapbox.mapboxsdk.maps.NativeMapView.render(NativeMapView.java: 158)
at com.mapbox.mapboxsdk.maps.MapView.onDraw(MapView.java: 416)
at android.view.View.draw(View.java: 16536)
at android.widget.FrameLayout.draw(FrameLayout.java: 598)
at android.view.View.updateDisplayListIfDirty(View.java: 15466)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java: 3697)
at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java: 3676)
at android.view.View.updateDisplayListIfDirty(View.java: 15426)
at android.view.View.getDisplayList(View.java: 15488)
at android.view.ThreadedRenderer.updateViewTreeDisplayList(ThreadedRenderer.java: 309)
at android.view.ThreadedRenderer.updateRootDisplayList(ThreadedRenderer.java: 315)
at android.view.ThreadedRenderer.draw(ThreadedRenderer.java: 354)
at android.view.ViewRootImpl.draw(ViewRootImpl.java: 2956)
at android.view.ViewRootImpl.performDraw(ViewRootImpl.java: 2753)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java: 2339)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java: 1314)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java: 7057)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java: 829)
at android.view.Choreographer.doCallbacks(Choreographer.java: 606)
at android.view.Choreographer.doFrame(Choreographer.java: 576)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java: 815)
at android.os.Handler.handleCallback(Handler.java: 739)
at android.os.Handler.dispatchMessage(Handler.java: 95)
at android.os.Looper.loop(Looper.java: 145)
at android.app.ActivityThread.main(ActivityThread.java: 7007)
at java.lang.reflect.Method.invoke(Native Method)
at java.lang.reflect.Method.invoke(Method.java: 372)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java: 1404)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java: 1199)

kkaefer · 2017-07-10T09:18:02Z

src/mbgl/gl/attribute.cpp


    for (int32_t i = 0; i < attributeCount; i++) {
-        activeAttributes.emplace(getAttributeName(id, maxAttributeLength, i));
+        std::string attributeName;


We could hoist this to outside the loop.

kkaefer · 2017-07-10T09:26:55Z

src/mbgl/programs/segment.hpp

+
+namespace mbgl {
+
+template <class Attributes>


This class doesn't use this template parameter. Consider removing it?

It's still useful as a means of type safety -- prevents mismatching a segment for the wrong set of attributes to Program::draw.

Okay, do you think this affects code size?

Since there are no member functions, and the constructor is likely inlined, I don't think it will be affected. I'm hopeful this PR will reduce code size overall, since the template parameters were removed from AttributeBinding. I'll run bloaty and see.

No difference:

VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ +0.1% +1.95Ki __TEXT,__text +1.95Ki +0.1% +0.3% +48 __DATA,__data +48 +0.3% +0.0% +46 __TEXT,__cstring +46 +0.0% -------------- SHRINKING -------------- -0.6% -1.11Ki [None] -1.14Ki -0.6% -0.3% -836 __TEXT,__gcc_except_tab -836 -0.3% -0.0% -80 __TEXT,__const -80 -0.0% -0.1% -44 __TEXT,__unwind_info -44 -0.1% [ = ] 0 TOTAL -32 -0.0%

kkaefer · 2017-07-10T09:27:18Z

src/mbgl/programs/segment.hpp

+class SegmentVector : public std::vector<Segment<Attributes>> {
+public:
+    SegmentVector() = default;
+};


While we're at it, we can change this to a typedef.

kkaefer · 2017-07-10T09:27:59Z

src/mbgl/programs/symbol_program.hpp

@@ -155,7 +156,7 @@ class ConstantSymbolSizeBinder final : public SymbolSizeBinder {
    }

    SymbolSizeAttributes::Bindings attributeBindings() const override {
-        return SymbolSizeAttributes::Bindings { gl::DisabledAttribute() };
+        return SymbolSizeAttributes::Bindings { std::experimental::nullopt };


Can we alias this in optional.hpp similar to std::experimental::optional<T>?

kkaefer · 2017-07-10T09:30:29Z

src/mbgl/programs/segment.hpp

@@ -0,0 +1,40 @@
+#pragma once


I believe this should be in renderer, not programs. The latter only contains code that is specific to our shader programs, but Segment isn't.

I think it's better here. The division as I see it is:

programs -- code closely related to OpenGL, but specific to Mapbox GL in some way (so doesn't belong in gl). segment.hpp is similar to attributes.hpp and uniforms.hpp in this sense.

renderer -- code that works with the Mapbox GL domain model (sources, layers, etc.) to facilitate rendering.

kkaefer · 2017-07-10T10:27:49Z

src/mbgl/gl/vertex_array.hpp

+    State<value::BindElementBuffer> indexBuffer;
+
+    using AttributeState = State<value::VertexAttribute, Context&, AttributeLocation>;
+    std::array<AttributeState, MAX_ATTRIBUTES> bindings;


I'm a little bit concerned about the overall size of this struct (it's 672 bytes on a 64 bit platform). In particular, we're duplicating the reference to Context 8 times as part of this state.

Do you see a way to avoid duplicating the reference? It seems like it would require refactoring how we pass additional arguments to Set and probably replacing State::operator= with State::set. If we're going to do that, I'd rather do it in a separate refactor.

kkaefer · 2017-07-10T10:34:14Z

src/mbgl/gl/attribute.hpp

+
+    BufferID vertexBuffer;
+    std::size_t vertexSize;
+    std::size_t vertexOffset;


Can we reduce the overall size of this struct a little bit by using smaller types? OpenGL only uses 32 bit types for size + offsets anyway, so there's no need to use 64 bit types on a 64 bit platform. attributeSize can only be 1, 2, 3, or 4.

kkaefer · 2017-07-10T10:56:25Z

src/mbgl/gl/vertex_array.hpp

+    }
+};
+
+using UniqueVertexArrayState = std::unique_ptr<VertexArrayState, std::function<void (VertexArrayState*)>>;


Instead of storing a std::function, can we use something like this:

template <typename T> struct ConditionalDeleter { const bool destroy = true; void operator()(T* ptr) const { if (destroy) { std::default_delete<T>()(ptr); } } };

kkaefer · 2017-07-10T11:02:05Z

src/mbgl/gl/types.hpp

@@ -15,7 +15,7 @@ using VertexArrayID = uint32_t;
 using FramebufferID = uint32_t;
 using RenderbufferID = uint32_t;

-using AttributeLocation = int32_t;
+using AttributeLocation = uint32_t;


glGetAttribLocation returns signed integers. I know we aren't using that call, so a comment explaining the discrepancy would be useful.

kkaefer · 2017-07-10T11:07:09Z

src/mbgl/programs/program.hpp

+            optional<gl::VertexArray>& vertexArray = segment.vertexArrays[layerID];
+
+            if (!vertexArray) {
+                vertexArray = context.createVertexArray();


Instead of using optional here, can we use key presence as an indicator for whether we need to create the vertex array?

jfirebaugh · 2017-07-10T19:53:45Z

@tobrun I'm wondering if the issue you're experiencing is connected with program binary caching. Does it work on a fresh install?

tobrun · 2017-07-11T06:04:16Z

removing the app and reinstalling doesn't show the crash. Sorry for the noise!

kkaefer · 2017-07-11T10:48:46Z

@tobrun no that's a good call. It means there's an issue with our upgrade path. The binary program caching system tries to restore the programs (which also stores the attribute assignments), but the order/numbering doesn't match our code anymore. It restores the code because the hash is only based on the actual file content. This means whenever we modify anything about how we assign attribute bindings, we must also force a hash change.

These changes are necessary for programs whose set of active attributes is not fixed at compile time by a template parameter pack, but rather varies based on the generated shader text at runtime. In such cases, the attribute location of a given named attribute may vary between instances of the same Program. Previously, attribute bindings were implicitly associated with a location based on template parameter order, and -1 was used to indicate an inactive attribute. This left us unable to disable the appropriate attribute when it went from active to inactive. Now, the state tracker for bindings explicitly associates locations and state, and an empty optional is used to indicate an inactive attribute. In addition, a gl::VertexArray class is now exposed, allowing more flexibility in the relationship between Programs, Segments, and attribute bindings. In this commit, that relationship does not change, but the subsequent commit adjusts it to match gl-js, reduce rebinds, and work around buggy VAO implementations. VertexArray uses a pimpl idiom in order to support implementations that lack the VAO extension. In that case, all VertexArrays share global binding state, reflecting the platform reality in the absence of VAOs, while still providing a uniform API.

Reduces rebinding, matches gl-js, and works around the buggy VAO implementation on PowerVR SGX544 GPUs.

…mLocation

jfirebaugh · 2017-07-12T22:04:34Z

I've added a couple speculative commits to try to solve the issue with cached program binaries -- if those don't fix it, I'll continue the investigation in #9473.

Remove re-linking programs as redundant. It costs (cheaper to link once than twice) and is not that common GL API usage pattern (subjective). Initial idea was to enable work on optimization what would reduce number of attrib setup calls in case that VAO is not available (#9433). As such optimization is not implemented, we can remove re-linking. Related to closed PR #9433 and PR #11583.

Remove re-linking programs as redundant. It costs (cheaper to link once than twice) and (subjective) is not that common GL API usage pattern, although perfectly legal and permitted. Initial idea, behind the removed code, was to enable work on optimization that would reduce number of attrib setup calls in case when VAO is not available (as described in #9433). As such optimization is not implemented, and it is arguable if it makes sense to do it now, we can remove re-linking. Related to closed PRs #9433 and PR #11583. I have [measured the time spent just on relinking](https://gist.github.com/astojilj/29bd5a5c5dc0b2d9f29ecb660da07fbf) using release build on iPhone SE (A9, same as iPhone 6S): - 1st run after reboot or installation Total 37.14ms, average per program:1.86ms - reopening Total: 2.47ms, average per program: 0.12ms This time we save using the patch here.

jfirebaugh requested review from kkaefer, ansis and lbud July 6, 2017 15:27

jfirebaugh added the Core The cross-platform C++ core, aka mbgl label Jul 6, 2017

jfirebaugh force-pushed the attribute-location branch 2 times, most recently from 5998e07 to 353d1bf Compare July 6, 2017 15:43

jfirebaugh force-pushed the attribute-location branch 2 times, most recently from 581cc21 to 3dd30ef Compare July 6, 2017 16:14

jfirebaugh force-pushed the attribute-location branch 2 times, most recently from 575396c to 52a0f67 Compare July 6, 2017 16:54

jfirebaugh mentioned this pull request Jul 6, 2017

Backport attribute binding changes to release branch #9440

Merged

lbud approved these changes Jul 8, 2017

View reviewed changes

kkaefer reviewed Jul 10, 2017

View reviewed changes

jfirebaugh force-pushed the attribute-location branch from 52a0f67 to f32ec95 Compare July 10, 2017 19:32

jfirebaugh force-pushed the attribute-location branch 2 times, most recently from 58251be to 3566295 Compare July 10, 2017 21:16

tobrun mentioned this pull request Jul 11, 2017

Blacklist GL program binary caching Adreno GPUs #9473

Closed

jfirebaugh force-pushed the attribute-location branch 2 times, most recently from 91e52b8 to a28be5a Compare July 11, 2017 18:44

jfirebaugh added 3 commits July 12, 2017 14:33

[tests] Local ignores.json file for ignored integration tests

fbbc8f2

[core] Include what you use

8fb212d

[core] Inline getActiveAttributes details

fedfaa2

jfirebaugh added 4 commits July 12, 2017 14:48

[core] Per-segment-per-layer vertex arrays

358b030

Reduces rebinding, matches gl-js, and works around the buggy VAO implementation on PowerVR SGX544 GPUs.

[core] Use a type alias for SegmentVector

01aff01

[core] Remove segment.cpp

49726cd

jfirebaugh force-pushed the attribute-location branch from ab1c9a3 to 60a7e15 Compare July 12, 2017 21:49

jfirebaugh added 6 commits July 12, 2017 14:55

[core] Reduce memory requirements of VertexArrays

4c42132

[core] Add comments explaining typing of AttributeLocation and Unifor…

2691fe1

…mLocation

[core] No need for optional in map of VertexArrays

86ee821

[core] Pass correct sources to programIdentifier

c0d6a07

[android] Force previously cached program binaries to be regenerated

1176ead

Update mapbox-gl-js

11952db

jfirebaugh force-pushed the attribute-location branch from 60a7e15 to 11952db Compare July 12, 2017 21:55

jfirebaugh merged commit 11952db into master Jul 12, 2017

jfirebaugh deleted the attribute-location branch July 12, 2017 22:22

jfirebaugh mentioned this pull request Jul 13, 2017

Clip raster tiles #9468

Merged

2 tasks

tobrun mentioned this pull request Jul 20, 2017

Release Android v5.1.1 #9564

Closed

15 tasks

kkaefer mentioned this pull request Apr 11, 2018

Don't crash when attribute count is exceeded #11656

Merged

astojilj mentioned this pull request Apr 23, 2019

Remove re-linking programs approach #14482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Rework attribute binding (again) #9433

[core] Rework attribute binding (again) #9433

jfirebaugh commented Jul 6, 2017 •

edited

Loading

jfirebaugh commented Jul 6, 2017

jfirebaugh commented Jul 6, 2017

ChrisLoer commented Jul 6, 2017

lbud commented Jul 8, 2017

tobrun commented Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

jfirebaugh Jul 10, 2017

kkaefer Jul 10, 2017

jfirebaugh Jul 10, 2017

jfirebaugh Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

jfirebaugh Jul 10, 2017

kkaefer Jul 10, 2017

jfirebaugh Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

kkaefer Jul 10, 2017

jfirebaugh commented Jul 10, 2017

tobrun commented Jul 11, 2017 •

edited

Loading

kkaefer commented Jul 11, 2017

jfirebaugh commented Jul 12, 2017

[core] Rework attribute binding (again) #9433

[core] Rework attribute binding (again) #9433

Conversation

jfirebaugh commented Jul 6, 2017 • edited Loading

jfirebaugh commented Jul 6, 2017

jfirebaugh commented Jul 6, 2017

ChrisLoer commented Jul 6, 2017

lbud commented Jul 8, 2017

tobrun commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented Jul 10, 2017

tobrun commented Jul 11, 2017 • edited Loading

kkaefer commented Jul 11, 2017

jfirebaugh commented Jul 12, 2017

jfirebaugh commented Jul 6, 2017 •

edited

Loading

tobrun commented Jul 11, 2017 •

edited

Loading