Android snapshotter

[ivovandongen]

Initial implementation for client side static maps (#9340).

This implementation uses a object on the main thread and manages an internal thread for rendering. #9340 mentions the ability to render on arbitrary threads, but I think this brings more complexity than it's worth as the result has to be forwarded to the main thread by the users. Instead, this API offers a simple callback that is called when the image is ready and handles the cross-thread communication internally.

Todo:

• Handle life-cycle: deal with the app going to the background
• Allow the snapshot to be cancelled
• Handle multiple starts
• Design the rest of the API (currently only size + style url)
• Attribution
• Merge pre-requisite: Ensure GeometryTile::pending state is only false in the last placement attempt #9842

[ivovandongen]

@tobrun Could you help out defining the initial API on the Android side here?

[tobrun]

For a initial implementation I believe changing the camera would be most usefull?

• camera
  • position
  • bounds
  • insets

This leaves us with a choice, we either expose the same models as the SDK (eg. CameraPosition and LatLngBounds) or we keep the api in line what core exposes?

A second iteration could bring support for Layer/Sources?

Sidenote: I'm not aware of any requirements around items as showing the mapbox logo?

[tobrun]

Have you thought about implementing this as a service?

• With a service you have complete control over the context and its lifecycle
• Bitmap implement the Parceable interface so they can be used for intent communication
• Configurable to show progress in a notification (= foreground service)

[tobrun]

A good benchmark for this feature would be to have a grid or a list which loads map images on the fly.

[ivovandongen]

@tobrun

Added:

• Visible region (LatLngBounds)
• Camera Options (-EdgeInsets as discussed as padding on the resulting bitmap makes more sense)
• A grid in the example

I like the idea of a Service or threadpool somehow. Maybe separate from this PR though as it could just re-use these building blocks?

Sidenote: I'm not aware of any requirements around items as showing the mapbox logo?

I assumed we need to add the logo somehow. cc @zugaldia

[zugaldia]

I assumed we need to add the logo somehow. cc @zugaldia

Yes. We should include the logo in a way that both a regular map and a snapshot render in exactly the same manner. That way it's easier to set up transitions between the two that flow naturally.

[jfirebaugh]

Is there a reason we can't reuse HeadlessFrontend, rather than SnapshotterRendererFrontend? I'm envisioning a platform-independent implementation like:

// .hpp

class SnapshotCallback {
public:
    void operator()(std::exception_ptr, PremultipliedImage);
};

class MapSnapshotter {
public:
    MapSnapshotter(const std::string& styleURL,
                   const Size&,
                   const float pixelRatio,
                   const CameraOptions&);

   void snapshot(ActorRef<SnapshotCallback>);

private:
    class Impl;
    Thread<Impl> impl;
};

// .cpp

MapSnapshotter::MapSnapshotter(...)
   : impl(...) {
}

MapSnapshotter::snapshot(ActorRef<SnapshotCallback> callback) {
   impl.invoke(&Impl::snapshot, callback);
}

class Impl {
public:
    Impl(...);

   void snapshot(ActorRef<SnapshotCallback>);

private:
    HeadlessFrontend frontend;
    Map map;
}

Impl::Impl(...)
   : frontend(...)
   , map(...) {
   map.setStyle(...);
   map.setCamera(...);
}

Impl::snapshot(ActorRef<SnapshotCallback> callback) {
    map.renderStill([callback, &] (std::exception_ptr error) {
        callback.invoke(&SnapshotCallback::operator(), error, frontend.readStillImage());
    });
}

In other words: put everything -- frontend, map, backend, renderer -- on the background snapshot thread, and invoke the callback when done. No thread pausing/resuming needed.

[ivovandongen]

@jfirebaugh Thanks, that makes it indeed a lot simpler. I've added a implementation for SnapshotCallback that takes a lambda so platforms don't have to implement it.

Besides some fixes, I also took the opportunity to enable destruction of the Java object through regular garbage collection. By making Thread<> destructible from any thread (which should be safe already as no-one should be calling it during destruction anyway) and adding a generic wrapper around the JNI object we can now destruct the JNI peer from the finalizer thread.

This makes the API much more natural on the Java side and there is no need for explicit destruction other than to abort prematurely (now called cancel).

The only pre-requisite now for this PR is #9816 as cd8eb13 prevents the still image generation from rendering.

[jfirebaugh]

You can use

mapbox-gl-native/include/mbgl/util/string.hpp

Line 52 in bd15e27

inline std::string toString(std::exception_ptr error) {

here.

But it seems like we should propagate errors to the caller, via an onSnapshotError method or something, no?

[ivovandongen]

Thanks. I will add an additional callback. I refrained from doing so initially as there is not much you can do in these situations.

[jfirebaugh]

You might be able to do using SnapshotCallback = std::function<void (std::exception_ptr, PremultipliedImage)> and avoid the wrapper class.

[ivovandongen]

I left the wrapper class as it makes clear on which thread the callback is going to happen when requiring an ActorRef in MapSnapshotter::snapshot. If we require only the lambda, we would still internally use an actor/mailbox to ensure the callback happens on the original thread right?

[jfirebaugh]

I mean keep ActorRef<SnapshotCallback> as it is, but have SnapshotCallback just be a std::function typedef. I believe that's possible with your change in 68f470f.

[ivovandongen]

Yes, you're totally right. Very nice!

[kkaefer]

In case we have a region set, can we avoid calling the first jumpTo?

[ivovandongen]

Besides the coordinate, this also sets the pitch, etc. Since the map doesn't do anything significant before the render is started in Still mode I didn't see it as significant. (the onUpdate does not trigger an actual update yet).

[kkaefer]

Curious, what prompted this removal?

[ivovandongen]

This is c607c03. Enables the thread to be destructed from other threads (eg, the Finalizer thread on Android). Otherwise the client side api is quite awkward as the Java object needs to be explicitly "destructed" on the creating thread.

[kkaefer]

Given that this is in a unique_ptr anyway, can we instead forward declare util::Thread with

namespace util {
template <typename T> class Thread;
} // namespace util

instead of exposing it publicly and including it here?

[kkaefer]

This destructor synchronously destroys the thread. If a render is in progress, this could potentially take a few dozen or even hundred milliseconds. We could introduce a similar system like the tile parsing obsolete atomic bool that we check after every layer and abort rendering in case we try to abort the rendering.

[ivovandongen]

Could be worth looking into. This doesn't add too much overhead in the "regular" case, it does add a check every frame for every layer in Still and Continuous mode.