Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to add custom image transformation's for raster sources #11306

Closed
wants to merge 6 commits into from

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented Nov 23, 2021

This PR adds a new public API hook on a RasterSource instance that the user can get from map.getSource(id).

rasterSource.setImageTransformer(func: ImageTransformerFunction): void

Where the ImageTransformerFunction takes 2 inputs

  • img: ImageData: The pixels of the downloaded raster tile
  • tileId: {z: number, x: number, y: number}
    and is expected to return a Promise that resolves with
  • {width: number, height: number, data: Uint8Array of length = width * height * 4}
    I went with making the API async so that users can
    • post the transformation off to a WebWorker, use OffscreenCanvas etc
    • fetch additional metadata over the network, if needed using the tileId

One of the weird things that this does it makes the Tile objects retain a reference to the source ImageData this is necessary so that when the user updates the transformer because some external styling rules, we can update the map without a reload, we just rerun the transformer on the retained ImageData and reupload textures.

Screen.Recording.2021-11-23.at.8.38.42.AM.mov

So the weird gotcha is we need to tell the source to be retaining ImageData right from the get-go in map.load event. The way to accomplish this right now is to call setImageTransformer with a function that just passes the result through

map.addSource('dem-custom', {
        "type": "raster",
        "url": "mapbox://mapbox.terrain-rgb",
        "tileSize": 512,
        "maxzoom": 16
    });
    const rasterSource = map.getSource('dem-custom');
    rasterSource.setImageTransformer((img, tileId) => {
        return new Promise((resolve, reject) => {
            resolve(img);
        });
    });

Would love to discuss alternatives here, we could do something like:
it initially starts off at undefined so by default it doesn't retain just like the current implementation.
Calling setImageTransformer(null) sets it to a no-op, but with retention.

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

@arindam1993 arindam1993 requested a review from mourner November 23, 2021 17:04
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Haven't reviewed this in detail yet, but overall this look great! My main concern is whether the added overhead and complexity necessary to allow updating custom-transformed tiles without reload is worth it. Are most use cases we have in mind for this feature need fast transform updates? Could we accept the flicker/reload and simplify the code as a different kind of trade-off?

@arindam1993
Copy link
Contributor Author

Haven't reviewed this in detail yet, but overall this look great! My main concern is whether the added overhead and complexity necessary to allow updating custom-transformed tiles without reload is worth it. Are most use cases we have in mind for this feature need fast transform updates? Could we accept the flicker/reload and simplify the code as a different kind of trade-off?

I think we totally could, I went down the path of the TimeBudgetedTaskQueue because we've talked about spreading out texture uploads over multiple frames for a while
#7451
#7405
and this allows us to do that.

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

@arindam1993 results are great! Few things I'd like to discuss:

(1) Can you explain the reasoning of having it as an API on the source instead of a modifier on the raster layer (e.g. having a raster-kernel or raster-modifier on the raster layer)? Mainly to explore the idea of a GPU approach with expressions.
(2) Do we need padding data access (of a few pixels around the tiles) for kernels that aren't operations on a single pixel?
(3) Detail: Why is the tileId important to give in the callback?

For (1), I think raw access like proposed here has advantages, but the delay we introduce for processing things on the CPU could be eliminated by finding a way to move things to the GPU. The challenge is how to wrap things nicely for the user so that they don't have to think of the shading languages we have in the shaders, while still expressing fully a kernel/image modifier operation.

How I imagine a draft of this could be exposed:

'layers': [{
  'id': 'raster-layer',
  'type': 'raster',
  'source': 'raster-tiles',
  paint: {
    'raster-kernel': ['kernel': ...]: color
  }
}]

In the CPU we would then translate the kernel expression to GLSL, and have an injection point in our shader with the translated code:

void apply_kernel(vec4 inout color) {
    // injection point modifying 'color'
    #pragma mapbox: raster_kernel
}

void main() {
   ...
#ifdef RASTER_KERNEL
    apply_kernel(out_color);
#endif
    gl_FragColor = out_color;
}

@arindam1993
Copy link
Contributor Author

@mourner I think you're right, I just did that and replaced it all with a SourceCache.reload() its still works fine and is way less complex.

I kept the FrameBudgetQueue around in this branch https://github.com/mapbox/mapbox-gl-js/tree/fancy-raster-transform but I don't think we need it.

@rreusser
Copy link
Contributor

rreusser commented Nov 30, 2021

Looks great, and I'm exciting to see this! 👏

I hope I'm not ill-informed on the context or requirements, but I do think though that @karimnaaji makes a number of very good points. Would this eventually be superseded by GLSL code generation from expressions? Are there things this makes possible which shader evaluation would make difficult or impossible? I think paint-time evaluation would be performant enough (short of highly pathological inputs), and of course one of the biggest benefits would be portability via the style spec.

I'm also contemplating how people might use this. If we can't simply ask the user to plug d3-scale-chromatic into their custom callback, then GL JS would presumably have to make it reasonable to plug a tabulated colorscale into an expression and then sample it in such an expression.

Especially since I'm now learning about and contemplating raster stuff, I'm very curious to understand a bit more about the tradeoff of making this a platform-dependent runtime callback hook vs. part of the style spec.

@ryanhamley ryanhamley closed this Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants