Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210

1ec5 · 2020-03-13T08:40:56Z

Upgraded MGLMapSnapshotter to be compatible with mbgl v1.4.0. For the iOS map SDK v5.8.0-beta.1 and macOS map SDK v0.15.0-beta.1 release notes, we’ll add:

MGLMapSnapshotter no longer keeps itself from being deallocated before its completion handler is called. To ensure that the completion handler passed into -[MGLMapSnapshotter startWithCompletionHandler:] is called, maintain a strong reference to the snapshotter from a longer-lived class, such as the class where you call -[MGLMapSnapshotter startWithCompletionHandler:]. (Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210)
The -[MGLMapSnapshotter cancel] method no longer calls the completion handler passed into -[MGLMapSnapshotter startWithCompletionHandler:]. (Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210)
Fixed an issue where the MGLMapSnapshotter.loading property always returned NO, even while loading a snapshot. (Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210)
Downloaded offline packs no longer reduce the storage space available for ambient caching of tiles and other resources. ([core] Calculate size of an ambient cache without offline region's resources mapbox-gl-native#15622)
Improved performance when continuously animating a tilted map. ([core] Bring more stability to the symbol placement mapbox-gl-native#16287)

Converted as many of MGLMapSnapshotter’s instance variables and properties as possible into local variables close to the point of use. -loading once again works and is used to avoid concurrent requests, now that the completion handler is no longer memoized. Snapshotter options are converted into an mbgl::MapSnapshotter on demand as late as possible.

The completion handler of -startWithCompletionHandler: and its variants is no longer called as part of -cancel, when the snapshotter is deallocated, or when the application is terminating, per the rationale in #200 (comment). To ensure that the snapshotter runs to completion, it is necessary to hold a strong reference to the snapshotter, ideally in the containing class.

Pushed some drawing and completion code up from inner completion blocks to outer completion blocks to avoid excessively passing state around. Converted a few class methods into standalone C functions to emphasize the avoidance of captured state.

More to come:

Upgrade mbgl to master
Regenerate runtime styling documentation
Update snapshotter to build against latest mbgl
Eliminate most snapshotter ivars
Simplify callbacks
Keep snapshotter alive while loading as with MKMapSnapshotter (Keep MGLMapSnapshotter alive through completion #211)
Alternatively, document the need to store a strong reference to the snapshotter in the containing class (would be undone by Keep MGLMapSnapshotter alive through completion #211)
Document the default completion handler queue

Fixes #209. Undoes much of mapbox/mapbox-gl-native#12355. Depends on mapbox/mapbox-gl-native#16268. Working towards #200.

/cc @mapbox/maps-ios @alexshalamov

1ec5 · 2020-03-13T09:11:34Z

platform/darwin/src/MGLMapSnapshotter.mm


+    __weak __typeof__(self) weakSelf = self;
    dispatch_async(workQueue, ^{


Do we really need to decorate the image on a worker thread? MGLAttributedSnapshot() doesn’t even show up in time profiles of a basic snapshot at 1512 × 1288 points. Removing this worker thread dance would greatly simplify the code: we could make this method synchronous and have it call a completion block that dispatches to the result queue.

Hi there. Just looking at https://github.com/mapbox/mapbox-gl-native-ios/blob/bdb2e61624b30b57fcbb9b9c38540526aeab364f/platform/darwin/src/MGLMapSnapshotter.mm#L274

It looks like this decoration is happening on the same queue that the snapshot is occurring on? Is that correct? If that's the case I don't see much of a reason unless that queue is expected to be busy.

Agreed - the only thing I wonder is if the overlayHandler rendering takes time...

Indeed, that comment is outdated. The block passed into MapSnapshotter::snapshot() is executed on the same queue that -[MGLMapSnapshotter startWithQueue:completionHandler:] is called from, though the actual resource fetching and rendering happens on a background thread as usual. -decorateImage:withOptions:attributions:pointForFn:latLngForFn:overlayHandler:completionHandler:queue: does the decoration on a different background queue, including running overlayHandler, and finally the results are dispatched to the queue passed into -startWithQueue:completionHandler:. In all, there are three queues involved just at the SDK level, not counting what happens internally to mbgl.

The overlayHandler could take arbitrary time depending on what the developer specifies. If we remove workQueue from the picture, would it make sense to run the overlayHandler on the initial queue that calls -startWithQueue:completionHandler: or on the queue passed into that method?

MGLAttributedSnapshot() doesn’t even show up in time profiles of a basic snapshot at 1512 × 1288 points.

I don't have context on what this is, but we're about to greatly expand what can be done to snapshots with runtime styling. It seems like this is the wrong time to move everything to the main thread.

Removing this worker thread dance would greatly simplify the code: we could make this method synchronous and have it call a completion block that dispatches to the result queue.

Then snapshot generation could potentially block the main thread, right? Not sure this is a good idea. There are use cases where 6-7 of these snapshots are being generated at the same time.

Oh, sorry if I responded to an outdated comment.

would it make sense to run the overlayHandler on the initial queue that calls -startWithQueue:completionHandler: or on the queue passed into that method?

I think this makes sense.

As of mapbox/mapbox-gl-native#16268, the block that’s literally defined inline inside of -[MGLMapSnapshotter startWithQueue:completionHandler:] does run on the queue on which -startWithQueue:completionHandler: is called, typically the main queue. That change took place so that the developer can safely implement the runtime styling hooks called for in #200 without accidentally creating race conditions between the main thread and some background thread they have no awareness of. However, the bulk of the work continues to occur on background threads.

What I’m still unsure of is which queue the developer expects the overlay handler to be called on:

The initial queue on which -startWithQueue:completionHandler: is called (think main queue)

The special-purpose worker queue that the developer has no control over (current behavior, could lead to race conditions or deadlocking)

The queue that the developer passed into -startWithQueue:completionHandler: ostensibly for the purpose of receiving the finished snapshot

Despite the amount of code involved, MGLAttributedSnapshot() and the methods it calls are so trivial that they can comfortably occur on any thread. But the overlayHandler callback is less certain because the developer has free rein to perform expensive tasks in that callback.

For now, I’ve kept the decorator queue but consolidated the dispatching into a single method, making the other methods synchronous on whatever queue they’re called on. This will simplify any potential change that would keep the snapshotter alive until completion.

Just my 2 cents:

I think it's fine to utilize the queue the developer passed in for the overlay handler work with a few caveats:

They could pass in the main queue. This, in my very opinionated view, would be a developer error in this context and I would assert on it. However given this is an existing API, it's hard to assert on it without probably generating support tickets 🙃. Plus many developers (myself included) use the main queue as a sync point for completion handlers.

Related to 1 - I'm unfamiliar with the queues mbgl is using but the interplay of multiple queues here has me looking at potential priority inversion issues. If the developer passes in a queue at the utility QOS, it'll slow the whole process down. Just something to keep in mind.

Tangentially related to this:
https://github.com/mapbox/mapbox-gl-native-ios/blob/master/platform/darwin/src/MGLMapSnapshotter.h#L241

That function should document what queue the completion handler is called on when developer's don't pass in a queue.

1ec5 · 2020-03-13T09:12:39Z

platform/darwin/src/MGLMapSnapshotter.mm

-
+            if (completion) {
+                __typeof__(self) strongSelf = weakSelf;
+                strongSelf.loading = NO;


This is happening on the result queue, but loading could’ve been set to YES on a different queue.

platform/darwin/src/MGLMapSnapshotter.mm

julianrex

Thanks - this looks a lot cleaner; getting rid of the cruft certainly makes it more readable/understandable.

My minor niggle is the completion-block-not-called-on-cancel/dealloc but I massively prefer the cleaner code that results.

Quoting from this comment:

As far as I can tell, we don’t document anywhere that the completion handler does get called in this situation, so it would be a backwards-compatible change. The developer has full control over what happens when they call -cancel; they’re free to call that completion block themselves.

Perhaps the solution here is to clearly document this requirement, as a test for this situation (maybe even update the examples), and update mapbox/mapbox-gl-native#12336 as I fear this change will break these developers' code.

With code this difficult (including the C++/Obj-C dance), it's hard to fully understand the logic; my hope is that the integration tests catch the bulk of the issues.

cc @chloekraw @knov

knov · 2020-03-13T20:37:31Z

@1ec5 mimicking @julianrex's concern in regards to the potential for the completion-block-not-called-on-cancel/dealloc breaking other developer's code. Wondering if we can evaluate the options here.

1ec5 · 2020-03-13T23:04:49Z

With code this difficult (including the C++/Obj-C dance), it's hard to fully understand the logic; my hope is that the integration tests catch the bulk of the issues.

The integration tests for this class cover a variety of concurrency scenarios. (I added a couple of my own.) This class has been a bugfarm in the past, so there’s some amount of risk involved with touching it in any way. But I think removing the expectation of always calling the completion handler reduces the class’s scope enough to be manageable. I’m not even sure we need to worry about the application termination case anymore, though I left it in there for consistency with MGLMapView.

Perhaps the solution here is to clearly document this requirement, as a test for this situation (maybe even update the examples), and update mapbox/mapbox-gl-native#12336 as I fear this change will break these developers' code.

This is a valid concern, given that we’ll be departing from the existing behavior of this class, not to mention MKMapSnapshotter. For starters, I’ve updated the changelog and MGLMapSnapshotter.h, including the example code in that header.

This example (Objective-C, Swift) happens to continue to function because it recommends capturing the snapshotter in the completion block. However, if the snapshot were ever to get canceled for any reason, the completion block won’t get called, causing the snapshotter to leak. For this reason, we should recommend a more straightforward ownership model where the class that uses the snapshotter holds onto it, at least until we figure out a safe, maintainable way to keep the snapshotter alive.

/cc @mapbox/maps-ios

1ec5 · 2020-03-14T01:13:44Z

The “Show Snapshots” command in iosapp seems to expose a hang in the snapshotter. For example, if MBXSnapshotsViewController looks like this:

then these two snapshotters haven’t finished:

https://github.com/mapbox/mapbox-gl-native-ios/blob/9936092d7c48d3eedc5b380d455389a1397d0fd5/platform/ios/app/MBXSnapshotsViewController.m#L40-L41

If I then push Back, the application attempts to cancel the snapshots, but instead the main thread is blocked until the snapshots run to completion:

#0	0x00007fff523b8ce6 in __psynch_cvwait ()
#1	0x00007fff52467185 in _pthread_cond_wait ()
#2	0x00007fff50202eda in std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&) ()
#3	0x00007fff50203801 in std::__1::__assoc_sub_state::__sub_wait(std::__1::unique_lock<std::__1::mutex>&) ()
#4	0x00007fff5020375a in std::__1::__assoc_sub_state::copy() ()
#5	0x00007fff5020399d in std::__1::future<void>::get() ()
#6	0x00000001060d4bd4 in mbgl::util::Thread<mbgl::SnapshotterRenderer>::~Thread() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/include/mbgl/util/thread.hpp:98
#7	0x00000001060d4b15 in mbgl::util::Thread<mbgl::SnapshotterRenderer>::~Thread() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/include/mbgl/util/thread.hpp:83
#8	0x00000001060d4abb in std::__1::default_delete<mbgl::util::Thread<mbgl::SnapshotterRenderer> >::operator()(mbgl::util::Thread<mbgl::SnapshotterRenderer>*) const at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2339
#9	0x00000001060d4a3f in std::__1::unique_ptr<mbgl::util::Thread<mbgl::SnapshotterRenderer>, std::__1::default_delete<mbgl::util::Thread<mbgl::SnapshotterRenderer> > >::reset(mbgl::util::Thread<mbgl::SnapshotterRenderer>*) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2652
#10	0x00000001060d49d9 in std::__1::unique_ptr<mbgl::util::Thread<mbgl::SnapshotterRenderer>, std::__1::default_delete<mbgl::util::Thread<mbgl::SnapshotterRenderer> > >::~unique_ptr() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2606
#11	0x00000001060d49b5 in std::__1::unique_ptr<mbgl::util::Thread<mbgl::SnapshotterRenderer>, std::__1::default_delete<mbgl::util::Thread<mbgl::SnapshotterRenderer> > >::~unique_ptr() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2606
#12	0x00000001060d4971 in mbgl::SnapshotterRendererFrontend::~SnapshotterRendererFrontend() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:110
#13	0x00000001060c82e5 in mbgl::SnapshotterRendererFrontend::~SnapshotterRendererFrontend() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:110
#14	0x00000001060da1d1 in mbgl::MapSnapshotter::Impl::~Impl() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:140
#15	0x00000001060c8305 in mbgl::MapSnapshotter::Impl::~Impl() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:140
#16	0x00000001060c8329 in mbgl::MapSnapshotter::Impl::~Impl() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:140
#17	0x00000001060da3ff in std::__1::default_delete<mbgl::MapSnapshotter::Impl>::operator()(mbgl::MapSnapshotter::Impl*) const at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2339
#18	0x00000001060da37f in std::__1::unique_ptr<mbgl::MapSnapshotter::Impl, std::__1::default_delete<mbgl::MapSnapshotter::Impl> >::reset(mbgl::MapSnapshotter::Impl*) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2652
#19	0x00000001060da319 in std::__1::unique_ptr<mbgl::MapSnapshotter::Impl, std::__1::default_delete<mbgl::MapSnapshotter::Impl> >::~unique_ptr() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2606
#20	0x00000001060b75f5 in std::__1::unique_ptr<mbgl::MapSnapshotter::Impl, std::__1::default_delete<mbgl::MapSnapshotter::Impl> >::~unique_ptr() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2606
#21	0x00000001060b75d5 in mbgl::MapSnapshotter::~MapSnapshotter() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:254
#22	0x00000001060b7615 in mbgl::MapSnapshotter::~MapSnapshotter() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/map/map_snapshotter.cpp:254
#23	0x0000000105eb4c2b in std::__1::default_delete<mbgl::MapSnapshotter>::operator()(mbgl::MapSnapshotter*) const at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2339
#24	0x0000000105eacaef in std::__1::unique_ptr<mbgl::MapSnapshotter, std::__1::default_delete<mbgl::MapSnapshotter> >::reset(mbgl::MapSnapshotter*) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2652
#25	0x0000000105eaca49 in ::-[MGLMapSnapshotter cancel]() at /path/to/mapbox-gl-native-ios/platform/darwin/src/MGLMapSnapshotter.mm:657
#26	0x0000000105ea7d5d in ::-[MGLMapSnapshotter dealloc]() at /path/to/mapbox-gl-native-ios/platform/darwin/src/MGLMapSnapshotter.mm:214
#27	0x00007fff514110d6 in objc_object::sidetable_release(bool) ()
#28	0x00000001020cf422 in -[MBXSnapshotsViewController .cxx_destruct] at /path/to/mapbox-gl-native-ios/platform/ios/app/MBXSnapshotsViewController.m:19

1ec5 · 2020-03-14T01:17:24Z

Intermittently, some snapshots randomly take a long time to appear, suggesting a deadlock. Once again, if MBXSnapshotsViewController looks like this:

then these two snapshotters haven’t finished:

https://github.com/mapbox/mapbox-gl-native-ios/blob/9936092d7c48d3eedc5b380d455389a1397d0fd5/platform/ios/app/MBXSnapshotsViewController.m#L40-L41

and the corresponding com.mapbox.mbgl.Snapshotter threads are stuck checking whether the last OpenGL call on that thread succeeded:

https://github.com/mapbox/mapbox-gl-native/blob/09d307db14514479a8f2fbbd6e6a98496b1f23c7/src/mbgl/platform/gl_functions.cpp#L10

#0	0x00007fff523b8ce6 in __psynch_cvwait ()
#1	0x00007fff52467185 in _pthread_cond_wait ()
#2	0x00007fff2a3c0ff0 in cvmRequestFunctionPointerWrite ()
#3	0x0000000104690d66 in gleSetVertexArrayFunc ()
#4	0x0000000104647893 in glDrawElementsInstanced_STD_ES2Exec ()
#5	0x0000000104646cf8 in glDrawElements_UnpackThread ()
#6	0x000000010465939f in gleCmdProcessor ()
#7	0x00000001024afd48 in _dispatch_client_callout ()
#8	0x00000001024be9bf in _dispatch_lane_barrier_sync_invoke_and_complete ()
#9	0x000000010461b327 in glGetError_ExecThread ()
#10	0x0000000106313d3c in mbgl::platform::glCheckError(char const*, char const*, int) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/platform/gl_functions.cpp:10
#11	0x000000010622d544 in mbgl::gl::Context::draw(mbgl::gfx::DrawMode const&, unsigned long, unsigned long)::$_45::operator()() const::__MBGL_CHECK_ERROR::~__MBGL_CHECK_ERROR() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/gl/context.cpp:626
#12	0x000000010622d515 in mbgl::gl::Context::draw(mbgl::gfx::DrawMode const&, unsigned long, unsigned long)::$_45::operator()() const::__MBGL_CHECK_ERROR::~__MBGL_CHECK_ERROR() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/gl/context.cpp:626
#13	0x000000010622a5a8 in mbgl::gl::Context::draw(mbgl::gfx::DrawMode const&, unsigned long, unsigned long)::$_45::operator()() const at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/gl/context.cpp:626
#14	0x000000010622a4c9 in mbgl::gl::Context::draw(mbgl::gfx::DrawMode const&, unsigned long, unsigned long) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/gl/context.cpp:626
#15	0x000000010638ee8e in mbgl::gl::Program<mbgl::LineProgram>::draw(mbgl::gfx::Context&, mbgl::gfx::RenderPass&, mbgl::gfx::DrawMode const&, mbgl::gfx::DepthMode const&, mbgl::gfx::StencilMode const&, mbgl::gfx::ColorMode const&, mbgl::gfx::CullFaceMode const&, mbgl::gfx::UniformValues<mbgl::TypeList<mbgl::uniforms::matrix, mbgl::uniforms::ratio, mbgl::uniforms::units_to_pixels, mbgl::uniforms::device_pixel_ratio, mbgl::InterpolationUniform<mbgl::attributes::blur>, mbgl::InterpolationUniform<mbgl::attributes::color>, mbgl::InterpolationUniform<mbgl::attributes::floorwidth>, mbgl::InterpolationUniform<mbgl::attributes::gapwidth>, mbgl::InterpolationUniform<mbgl::attributes::offset>, mbgl::InterpolationUniform<mbgl::attributes::opacity>, mbgl::InterpolationUniform<mbgl::attributes::pattern_to>, mbgl::InterpolationUniform<mbgl::attributes::pattern_from>, mbgl::InterpolationUniform<mbgl::attributes::width>, mbgl::uniforms::blur, mbgl::uniforms::color, mbgl::uniforms::floorwidth, mbgl::uniforms::gapwidth, mbgl::uniforms::offset, mbgl::uniforms::opacity, mbgl::uniforms::pattern_to, mbgl::uniforms::pattern_from, mbgl::uniforms::width> > const&, mbgl::gfx::DrawScope&, mbgl::gfx::AttributeBindings<mbgl::TypeList<mbgl::attributes::pos_normal, mbgl::attributes::data<unsigned char, 4ul>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::blur>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::color>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::floorwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::gapwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::offset>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::opacity>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_to>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_from>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::width> > > const&, mbgl::gfx::TextureBindings<mbgl::TypeList<> > const&, mbgl::gfx::IndexBuffer const&, unsigned long, unsigned long) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/gl/program.hpp:131
#16	0x000000010652e036 in void mbgl::Program<mbgl::LineProgram, (mbgl::gfx::PrimitiveType)2, mbgl::TypeList<mbgl::attributes::pos_normal, mbgl::attributes::data<unsigned char, 4ul> >, mbgl::TypeList<mbgl::uniforms::matrix, mbgl::uniforms::ratio, mbgl::uniforms::units_to_pixels, mbgl::uniforms::device_pixel_ratio>, mbgl::TypeList<>, mbgl::style::LinePaintProperties>::draw<mbgl::gfx::Triangles>(mbgl::gfx::Context&, mbgl::gfx::RenderPass&, mbgl::gfx::Triangles const&, mbgl::gfx::DepthMode const&, mbgl::gfx::StencilMode const&, mbgl::gfx::ColorMode const&, mbgl::gfx::CullFaceMode const&, mbgl::gfx::IndexBuffer const&, std::__1::vector<mbgl::Segment<mbgl::TypeList<mbgl::attributes::pos_normal, mbgl::attributes::data<unsigned char, 4ul>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::blur>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::color>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::floorwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::gapwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::offset>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::opacity>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_to>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_from>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::width> > >, std::__1::allocator<mbgl::Segment<mbgl::TypeList<mbgl::attributes::pos_normal, mbgl::attributes::data<unsigned char, 4ul>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::blur>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::color>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::floorwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::gapwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::offset>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::opacity>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_to>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_from>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::width> > > > > const&, mbgl::gfx::UniformValues<mbgl::TypeList<mbgl::uniforms::matrix, mbgl::uniforms::ratio, mbgl::uniforms::units_to_pixels, mbgl::uniforms::device_pixel_ratio, mbgl::InterpolationUniform<mbgl::attributes::blur>, mbgl::InterpolationUniform<mbgl::attributes::color>, mbgl::InterpolationUniform<mbgl::attributes::floorwidth>, mbgl::InterpolationUniform<mbgl::attributes::gapwidth>, mbgl::InterpolationUniform<mbgl::attributes::offset>, mbgl::InterpolationUniform<mbgl::attributes::opacity>, mbgl::InterpolationUniform<mbgl::attributes::pattern_to>, mbgl::InterpolationUniform<mbgl::attributes::pattern_from>, mbgl::InterpolationUniform<mbgl::attributes::width>, mbgl::uniforms::blur, mbgl::uniforms::color, mbgl::uniforms::floorwidth, mbgl::uniforms::gapwidth, mbgl::uniforms::offset, mbgl::uniforms::opacity, mbgl::uniforms::pattern_to, mbgl::uniforms::pattern_from, mbgl::uniforms::width> > const&, mbgl::gfx::AttributeBindings<mbgl::TypeList<mbgl::attributes::pos_normal, mbgl::attributes::data<unsigned char, 4ul>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::blur>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::color>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::floorwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::gapwidth>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::offset>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::opacity>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_to>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::pattern_from>, mbgl::ZoomInterpolatedAttribute<mbgl::attributes::width> > > const&, mbgl::gfx::TextureBindings<mbgl::TypeList<> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/programs/program.hpp:100
#17	0x00000001065107d1 in _ZZN4mbgl15RenderLineLayer6renderERNS_15PaintParametersEENK3$_0clINS_11LineProgramENS_3gfx13UniformValuesINS_8TypeListIJNS_8uniforms6matrixENS9_5ratioENS9_15units_to_pixelsENS9_18device_pixel_ratioEEEEEENS6_15TextureBindingsINS8_IJEEEEEEEDaRT_OT0_RKNSt12experimental8optionalINS_13ImagePositionEEEST_OT1_ at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/renderer/layers/render_line_layer.cpp:131
#18	0x000000010650f7f6 in mbgl::RenderLineLayer::render(mbgl::PaintParameters&) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/renderer/layers/render_line_layer.cpp:207
#19	0x00000001065c19d5 in mbgl::(anonymous namespace)::LayerRenderItem::render(mbgl::PaintParameters&) const at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/renderer/render_orchestrator.cpp:52
#20	0x00000001065ecf43 in mbgl::Renderer::Impl::render(mbgl::RenderTree const&) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/renderer/renderer_impl.cpp:172
#21	0x00000001065e6fc1 in mbgl::Renderer::render(std::__1::shared_ptr<mbgl::UpdateParameters> const&) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/renderer/renderer.cpp:32
#22	0x00000001060b4c4e in mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0::operator()() const at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/default/src/mbgl/gfx/headless_frontend.cpp:42
#23	0x00000001060b4b7d in decltype(std::__1::forward<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0&>(fp)()) std::__1::__invoke<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0&>(mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0&) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4361
#24	0x00000001060b4b2d in void std::__1::__invoke_void_return_wrapper<void>::__call<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0&>(mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0&) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__functional_base:349
#25	0x00000001060b4afd in std::__1::__function::__alloc_func<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0, std::__1::allocator<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0>, void ()>::operator()() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:1527
#26	0x00000001060b3789 in std::__1::__function::__func<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0, std::__1::allocator<mbgl::HeadlessFrontend::HeadlessFrontend(mbgl::Size, float, mbgl::gfx::HeadlessBackend::SwapBehaviour, mbgl::gfx::ContextMode, std::experimental::optional<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >)::$_0>, void ()>::operator()() at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:1651
#27	0x00000001060cc2d5 in std::__1::__function::__value_func<void ()>::operator()() const at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:1799
#28	0x00000001060cc205 in std::__1::function<void ()>::operator()() const at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2347
#29	0x00000001060978b1 in mbgl::util::AsyncTask::Impl::runTask() at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/darwin/src/async_task.cpp:45
#30	0x0000000106097855 in mbgl::util::AsyncTask::Impl::perform(void*) at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/platform/darwin/src/async_task.cpp:50

All four com.mapbox.mbgl.Worker threads are also waiting on something:

#0	0x00007fff523b8ce6 in __psynch_cvwait ()
#1	0x00007fff52467185 in _pthread_cond_wait ()
#2	0x00007fff50202eda in std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&) ()
#3	0x0000000106b2c451 in void std::__1::condition_variable::wait<mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0::operator()() const::'lambda'()>(std::__1::unique_lock<std::__1::mutex>&, mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0::operator()() const::'lambda'()) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__mutex_base:375
#4	0x0000000106b2c13b in mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0::operator()() const at /path/to/mapbox-gl-native-ios/vendor/mapbox-gl-native/src/mbgl/util/thread_pool.cpp:34
#5	0x0000000106b2bf3d in decltype(std::__1::forward<mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0>(fp)()) std::__1::__invoke<mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0>(mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0&&) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4361
#6	0x0000000106b2bed5 in void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0>&, std::__1::__tuple_indices<>) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/thread:342
#7	0x0000000106b2b7a6 in void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, mbgl::ThreadedSchedulerBase::makeSchedulerThread(unsigned long)::$_0> >(void*) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/thread:352
#8	0x00007fff52466e65 in _pthread_start ()
#9	0x00007fff5246283b in thread_start ()

1ec5 · 2020-03-14T09:32:17Z

#211 keeps the snapshotter alive until after the completion handler executes. It probably needs more scrutiny than the snapshotter changes in this PR, since it may reintroduce some edge cases that this PR had eliminated as part of limiting the snapshotter’s responsibilities.

1ec5 · 2020-03-16T15:40:22Z

The “Show Snapshots” command in iosapp seems to expose a hang in the snapshotter.

I just noticed the alexshalamov_styleable_snapshotter branch, which has a more targeted update to MGLMapSnapshotter. However, it also hangs when canceling. Sometimes it hangs indefinitely.

1ec5 · 2020-03-16T16:32:19Z

@alexshalamov was able to reproduce the hang on master, so we’ll track it in #214.

Upgraded MGLMapSnapshotter to be compatible with the latest mbgl. Converted as many of MGLMapSnapshotter’s instance variables and properties as possible into local variables close to the point of use. The completion handler of -startWithCompletionHandler: and its variants is no longer called as part of -cancel, when the snapshotter is deallocated, or when the application is terminating. -loading once again works and is used to avoid concurrent requests, now that the completion handler is no longer memoized. Snapshotter options are copied and converted into a snapshotter on demand as late as possible. Made the decoration step synchronous while continuing to perform it on a worker thread. Only access self on user-facing queues. Ensure MGLMapSnapshotter.loading is reset after snapshotting. Pushed some drawing and completion code up from inner completion blocks to outer completion blocks to avoid excessively passing state around. Converted a few class methods into standalone C functions to emphasize the avoidance of captured state.

…v1.4.0

1ec5 added bug Something isn't working dependencies Pull requests that update a dependency file ios build macOS 🛑 blocked labels Mar 13, 2020

1ec5 added this to the release-vanillashake milestone Mar 13, 2020

1ec5 requested review from fabian-guerra, julianrex, captainbarbosa and a team March 13, 2020 08:40

1ec5 self-assigned this Mar 13, 2020

1ec5 commented Mar 13, 2020

View reviewed changes

julianrex reviewed Mar 13, 2020

View reviewed changes

platform/darwin/src/MGLMapSnapshotter.mm Outdated Show resolved Hide resolved

julianrex approved these changes Mar 13, 2020

View reviewed changes

1ec5 force-pushed the 1ec5-snapshotter-parallel-state-209 branch from bdb2e61 to 360726e Compare March 13, 2020 20:55

1ec5 removed the 🛑 blocked label Mar 13, 2020

1ec5 changed the title ~~Streamline MGLMapSnapshotter~~ Streamline MGLMapSnapshotter; upgrade to mbgl v1.4.0 Mar 13, 2020

1ec5 changed the title ~~Streamline MGLMapSnapshotter; upgrade to mbgl v1.4.0~~ Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 Mar 13, 2020

This was referenced Mar 13, 2020

Remove “in” expression workaround #183

Merged

Allow predicates to test whether a feature lies within a given shape #184

Merged

This was referenced Mar 14, 2020

Keep MGLMapSnapshotter alive through completion #211

Merged

Platform bindings for runtime styling APIs on MapSnapshotter #200

Closed

1ec5 added 6 commits March 16, 2020 17:59

mbgl v1.4.0

48f034c

[ios, macos] Updated MGLSymbolStyleLayer.symbolSortKey documentation

5be63a0

[ios, macos] Documented performance and caching improvements in mbgl …

8998f7a

…v1.4.0

[ios, macos] Document changes to snapshotter memory management

8148cb7

[ios, macos] Clarify threading in snapshotter documentation

d357f8c

1ec5 force-pushed the 1ec5-snapshotter-parallel-state-209 branch from 055a482 to d357f8c Compare March 17, 2020 01:15

1ec5 merged commit d357f8c into master Mar 17, 2020

1ec5 deleted the 1ec5-snapshotter-parallel-state-209 branch March 17, 2020 02:38

This was referenced Mar 17, 2020

Uncaught exception starting MGLMapSnapshotter before using MGLMapView or MGLOfflineStorage #227

Closed

Snapshotter (runtime styling) delegates #235

Merged

Snapshotter overlay handler not called #236

Closed

1ec5 mentioned this pull request Apr 6, 2020

Initialize offline storage on main thread before snapshotting #259

Merged

1ec5 mentioned this pull request Apr 17, 2020

snapshot not correct #270

Closed

1ec5 mentioned this pull request May 28, 2020

Offline Directions errors-clarification mapbox/mapbox-navigation-ios#2374

Merged

1ec5 mentioned this pull request Sep 9, 2020

Hang when canceling in-progress snapshots mapbox/mapbox-gl-native#16530

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210

Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210

1ec5 commented Mar 13, 2020 •

edited

Loading

1ec5 Mar 13, 2020

msmollin Mar 13, 2020

julianrex Mar 13, 2020 •

edited

Loading

1ec5 Mar 13, 2020

chloekraw Mar 13, 2020

chloekraw Mar 13, 2020

1ec5 Mar 13, 2020 •

edited

Loading

1ec5 Mar 14, 2020

msmollin Mar 14, 2020

1ec5 Mar 13, 2020

julianrex left a comment

knov commented Mar 13, 2020

1ec5 commented Mar 13, 2020 •

edited

Loading

1ec5 commented Mar 14, 2020

1ec5 commented Mar 14, 2020

1ec5 commented Mar 14, 2020

1ec5 commented Mar 16, 2020 •

edited

Loading

1ec5 commented Mar 16, 2020


		__weak __typeof__(self) weakSelf = self;
		dispatch_async(workQueue, ^{

Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210

Streamline MGLMapSnapshotter; upgrade to Mapbox GL Native v1.4.0 #210

Conversation

1ec5 commented Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex Mar 13, 2020 • edited Loading

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

1ec5 Mar 13, 2020 • edited Loading

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

julianrex left a comment

Choose a reason for hiding this comment

knov commented Mar 13, 2020

1ec5 commented Mar 13, 2020 • edited Loading

1ec5 commented Mar 14, 2020

1ec5 commented Mar 14, 2020

1ec5 commented Mar 14, 2020

1ec5 commented Mar 16, 2020 • edited Loading

1ec5 commented Mar 16, 2020

1ec5 commented Mar 13, 2020 •

edited

Loading

julianrex Mar 13, 2020 •

edited

Loading

1ec5 Mar 13, 2020 •

edited

Loading

1ec5 commented Mar 13, 2020 •

edited

Loading

1ec5 commented Mar 16, 2020 •

edited

Loading