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

Accept premultiplied raw inputs #2685

Merged
merged 1 commit into from
May 3, 2021
Merged

Conversation

mnutt
Copy link
Contributor

@mnutt mnutt commented Apr 29, 2021

Addresses #1599.

I'm using tileserver-gl which works around using premultiplied inputs like so: https://github.com/maptiler/tileserver-gl/blob/4036d528ec9e/src/serve_rendered.js#L280-L294. This works, but from some simple benchmarks I've run it looks like it's faster to do in libvips than in javascript, especially for large images.

I ran into a bit of trouble with the unit test using assertSimilar, as the tests passed whether or not the image was unpremultiplied first so I ended up comparing the raw output directly.

If this approach looks acceptable I can add public API documentation.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR, this looks great and it's always a pleasure to see unit tests. Please go ahead and add JSDocs for this, plus I've left one small comment inline.

src/common.cc Outdated
@@ -96,6 +96,10 @@ namespace sharp {
descriptor->rawWidth = AttrAsUint32(input, "rawWidth");
descriptor->rawHeight = AttrAsUint32(input, "rawHeight");
}
// Raw pixel input is premultiplied
if (HasAttr(input, "rawPremultiplied")) {
Copy link
Owner

Choose a reason for hiding this comment

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

If possible, it might be better to ensure inputDescriptor.rawPremultiplied is always set in the JavaScript code to avoid a conditional statement here.

@mnutt mnutt force-pushed the raw-premultiplied-input branch 2 times, most recently from 83cb4f3 to c0b892a Compare May 2, 2021 17:29
lib/input.js Outdated Show resolved Hide resolved
@mnutt mnutt requested a review from lovell May 2, 2021 17:31
docs/api-constructor.md Outdated Show resolved Hide resolved
lib/input.js Outdated Show resolved Hide resolved
@mnutt mnutt force-pushed the raw-premultiplied-input branch from c0b892a to c9d5eb1 Compare May 3, 2021 12:46
Some inputs may already be premultiplied, and it is more efficient to
handle it in libvips than in javascript.
@mnutt mnutt force-pushed the raw-premultiplied-input branch from c9d5eb1 to 87c38c1 Compare May 3, 2021 12:57
@mnutt mnutt requested a review from lovell May 3, 2021 12:58
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 87c38c1 on mnutt:raw-premultiplied-input into 309918a on lovell:master.

@lovell lovell merged commit 9a1e8ed into lovell:master May 3, 2021
@lovell lovell added this to the v0.28.2 milestone May 3, 2021
@lovell
Copy link
Owner

lovell commented May 3, 2021

Brilliant, thank you very much, this will be in v0.28.2.

lovell added a commit that referenced this pull request May 3, 2021
@dantovbein

This comment has been minimized.

@mnutt mnutt deleted the raw-premultiplied-input branch May 5, 2021 12:38
Caerbannog added a commit to liberty-rider/tileserver-gl that referenced this pull request Nov 20, 2023
Maplibre-native outputs premultiplied pixels values. The sharp library did not
support it so we added code to cancel the alpha premultiplication.
Note that this can only visible onr raster tiles (and probably static maps).

The sharp library now supports premultiplied pixels with the right config.
Let's use it: it should be faster and easie to maintain.

Feature announced here:
lovell/sharp#1599 (comment)

Feature developped here by @mnutt:
lovell/sharp#2685

Signed-off-by: Martin d'Allens <martin.dallens@liberty-rider.com>
Caerbannog added a commit to liberty-rider/tileserver-gl that referenced this pull request Nov 23, 2023
Maplibre-native outputs premultiplied pixels values. The sharp library did not
support it so we added code to cancel the alpha premultiplication.
Note that this can only visible onr raster tiles (and probably static maps).

The sharp library now supports premultiplied pixels with the right config.
Let's use it: it should be faster and easie to maintain.

Feature announced here:
lovell/sharp#1599 (comment)

Feature developped here by @mnutt:
lovell/sharp#2685

Signed-off-by: Martin d'Allens <martin.dallens@liberty-rider.com>
acalcutt pushed a commit to maptiler/tileserver-gl that referenced this pull request Nov 23, 2023
Maplibre-native outputs premultiplied pixels values. The sharp library did not
support it so we added code to cancel the alpha premultiplication.
Note that this can only visible onr raster tiles (and probably static maps).

The sharp library now supports premultiplied pixels with the right config.
Let's use it: it should be faster and easie to maintain.

Feature announced here:
lovell/sharp#1599 (comment)

Feature developped here by @mnutt:
lovell/sharp#2685

Signed-off-by: Martin d'Allens <martin.dallens@liberty-rider.com>
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.

None yet

4 participants