Skip to content

Conversation

@CrazyLegumes
Copy link
Contributor

@CrazyLegumes CrazyLegumes commented Mar 8, 2022

EDIT by @tbonfort: the discussion is left here for history's sake, the underlying root cause for the issue is due to the fact that native gdal sources did not produce premultipled alpha as expected by the rest of the codebase. This issue affects only non-fully-opaque gdal sources

This change would remove pre-multiplication of the pixels using the alpha channel. According to the libpng spec, non-pre-multiplied alpha is "the lossless and more general case" link: http://www.libpng.org/pub/png/spec/1.1/PNG-Rationale.html#R.Non-premultiplied-alpha

Currently, this functionality currently creates unexpected behaviour for images with transparency that do not use pre-multiplied alphas. When using the argb_to_rgba function, if an image does not use pre-multiplied alpha, the rgb values may sometimes overflow the unsigned byte value resulting in oddly colored pixels and wildly distorted images. For example, take a pure white pixel with an alpha value of 254. For the R, G, and B pixels that would result in ((255 * 255) + (254 / 2)) / 254= 258. This would overflow and result in a value of 2! That would turn a nearly white pixel to a pure black pixel. This overflow gets worse as the alpha decreases.

Perhaps in a future effort, one could specify a setting in the configuration to apply/rescind pre-multiplication based on the custom format fields.

@jratike80
Copy link

I believe that some GIS viewers consider that alpha band is a binary mask so that zero means nodata and pixels are totally transparent and all values >0 mean data pixels which are totally opaque. The author of the current code has obviously been thinking that kind of viewers and decided to use pre-multiplication. With such viewers alpha value 1 or 2 does not turn pixels nearly black but only makes them visible.
I wonder if another approach could be to guarantee that after pre-multiplication the alpha values can only be 0 or 255?

I apologize if my thinking does not make sense, I am not an expert on this area.

@CrazyLegumes
Copy link
Contributor Author

I may have misspoke. It is not the alpha values that end up as 1 or 2 but rather the red, green, and blue values that get modified as a result of the pre-multiplication that end up as 1 or 2, when normally they would be 255 for a white pixel. It is interesting that any alpha value above 0 would mean fully opaque. I would think with the use of PNG formats, one would allow for partial transparency within an image that is served (which is how I discovered this issue).

@jratike80
Copy link

I believe that I misread. When it comes to alpha values between 1 and 254 this may be somehow related https://gdal.org/development/rfc/rfc15_nodatabitmask.html#alpha-bands. But that deals more with processing. WMS servers actually do support partial transparency with png output, Mapserver with the "opacity" keyword https://mapserver.org/mapfile/composite.html.

It would be nice it you could make a test case for your fix.

@jbo-ads
Copy link
Member

jbo-ads commented Mar 9, 2022

I am unable to determine whether the current behaviour is a bug or a design choice. In such situation, I tend to favor a configuration option to let the user specify its needs, with the default being the legacy behaviour (at least during some transitional period). @tbonfort, any thoughts?

Anyway, in order to better understand the issue that this PR solves, could you provide us with some example tiles showing what you expect and what you get with the current version?

Thank you for contributing!

@CrazyLegumes
Copy link
Contributor Author

CrazyLegumes commented Mar 9, 2022

Sure!
The first image is the tile that we expect to get. All the pixels that are not completely transparent are white with varying levels of transparency.
expected-tile
In the resulting image of the current implementation, any part of the image that is not completely opaque or completely transparent gets turned into black or some sort of gray due to modifying the RGB channels based on alpha value pre-multiplication.
mapcache-result

@jratike80
Copy link

jratike80 commented Mar 9, 2022

And this is how the first tile appears on top of orange background

image

I wonder what should happen with JPEG tiles which obviously do not support this kind of source data.

@jratike80
Copy link

jratike80 commented Mar 9, 2022

For example, take a pure white pixel with an alpha value of 254. For the R, G, and B pixels that would result in ((255 * 255) + (254 / 2)) / 254= 258.

Is there some error in this calculation? I found an example https://microsoft.github.io/Win2D/WinUI3/html/PremultipliedAlpha.htm and it is using:

premultiplied.R = (byte)(straight.R * straight.A / 255);
premultiplied.G = (byte)(straight.G * straight.A / 255);
premultiplied.B = (byte)(straight.B * straight.A / 255);
premultiplied.A = straight.A;

Isn't the code starting from line 447 made to handle premultiplied pixels? See the comment
* png transform function to switch from premultiplied argb to png expected rgba
But your example pixel is (254,254,254,255) so if I understand right it is not pre-multiplied but with direct alpha and it should not be handled with that piece of code. What part of the code tries to recognize if the data that flows in is pre-multiplied?

@CrazyLegumes
Copy link
Contributor Author

CrazyLegumes commented Mar 9, 2022

This formula is definitely much less prone to overflow errors assuming that the RGBA values of straight are also the byte type. However, you'd still result in darker pixels across the data when the issue I'd like to solve here is maintaining the original image quality, color, and transparency as much as possible when it comes across through MapCache.

From my understanding, the argb_to_rgba function is used if the image received has any pixels with an alpha value below 255 regardless if the image uses pre-multiplied values or not. The pixel I used in my example was a pure white pixel with an alpha value of 254 so that would be:
(255, 255, 255, 254)
As a result we'd then hit the portion of the code that does the pre-multiplication math and modify the other channels as a result.

Using the Microsoft version of this function, we'd get
premultiplied.R = (byte)(255 * 254 / 255) = 254
premultiplied.G and premultiplied.B would also be 254 which is better than getting a value of 2 from overflow, but as alpha decreases, the pixel would gradually get darker even though its also getting more transparent. This makes sense since the idea behind pre-multiplication is to assume that the source image is being composited against a black background.

I think this is where I'd agree with @jbo-ads that a configuration option might be the best option for users to defer to use this method or a new second method that doesn't involve any pre-multiplication.

@jratike80
Copy link

But isn't there a conversion from direct to pre-multiplied on line 405 and then back to direct on line 447?

@CrazyLegumes
Copy link
Contributor Author

When running this through the debugger, it doesn't seem like the steps at 405 even happen. So there must be a set of conditions to be met before the function in question, mapcache_imageio_decode, gets called for that original switch to pre-multiplication to happen.

As a side note, I do wonder what design decisions were in mind when choosing to do pre-multiplication such that I could maybe have a better understanding on how crucial this step is.

@tbonfort
Copy link
Member

tbonfort commented Mar 11, 2022

@CrazyLegumes the png spec expects non-premultiplied rgba whereas mapcache internally works with premultiplied rgba to speed up blending operations. Thus the code to convert from one to the other. You may argue that pre-multiplied is lossy, and it may seem that way when looking at the raw pixel values, however using either version you will get the same result when performing a blending operation (e.g. when blending a non premultiplied pixel with alpha=1, the contribution to the resulting layer will be +1 for a pixel above 128, and +0 for pixels below that. the premultiplication does just that, it premultiplies each channel so that it is scaled between 0 and alpha).

Please let us know what kind of data and requests you are trying to serve, from source data, to stored tiles, to tiled-requests, to what you are doing with the tiled result.

This PR must not be merged as is

@CrazyLegumes
Copy link
Contributor Author

@tbonfort In our configuration, we use MapCache and Apache configured with various GDAL WMTS sources that connect to a server via a WMTS capabilities document. We then allow users to make single tile requests to any of these layers individually. We've also configured MapCache to cache the file on disk upon completing these requests.

As a sort of testing grounds, we have a leaflet map configured to request tiles from MapCache and be placed on the map in their corresponding geographic location. All of our layers have the ability to be enabled or disabled on the map.

The image that I provided above is from a layer providing data on the amount of cloud cover in a given area. This data is encoded into an image in which the R,G and B channels of a single pixel are all set to 255 to and the alpha channel is set to a value between 0 and 255 to represent intensity/density of the cloud cover. This layer is also the only one that uses transparency within the data and as such is usually the top-most layer on our leaflet map.

@CrazyLegumes
Copy link
Contributor Author

Upon further debugging into the source, I've come to find that there seems to be cases in which the initial pre-multiplication step in mapcache_imageio_decode upon reading in the source data does not occur. However, in the writing step when caching these images, the current implementation will always attempt to undo the pre-multiplication regardless if the image has gone under pre-multiplication or not.

The following isn't an exhaustive list of where this can take place but notes a few situations where mapcache_imageio_decode will not be called and as a result, pre-multiplication will not take place:

  • When generating a response, if a single tile is requested
  • When preparing the image to be cached to disk, if the tile's raw data pointer is valid (presumably set by GDAL after reading)
  • When splitting the tile into meta tiles, if the tile's raw data pointer is valid (GDAL again)

If the ideal situation is to always undo the pre-multiplication step on pixels that have an alpha within this range, there should never be a situation in which those same pixels do not get pre-multiplied.

@tbonfort
Copy link
Member

tbonfort commented Mar 12, 2022

I suspect this is due to the gdal source, that does not premultiply its returned buffer here:

map->raw_image->data = rasterdata;

Can you try that and submit an updated PR if if fixes your issue?

- Added pre-multiplication based on alpha to GDAL sources after reading source data.
- Reverted change to imageio_png that removed undoing of pre-multiplication.
@CrazyLegumes
Copy link
Contributor Author

Sorry for the delay. As it turns out, adding the pre-multiplication step to the GDAL source did end up resolving the issue. I made an additional commit here with the changes.

- Declared integers used in for-loops beforehand rather than inside of them to comply with C standards.
- Removed assignment to rowptr at declaration time and moved it further downward.
@tbonfort tbonfort changed the title Removal of pre-multiplication from PNG caching/decoding GDAL sources do not produce premultiplied RGBA Mar 22, 2022
- Moved declarations of variables next to other variable declarations following C90 standards
@tbonfort tbonfort merged commit f327c16 into MapServer:main Mar 23, 2022
@tbonfort
Copy link
Member

@jbo-ads this one can be backported

@jbo-ads
Copy link
Member

jbo-ads commented Mar 23, 2022

@tbonfort V1.12 has already been released. I'll see how to release a 1.12.1.

jbo-ads pushed a commit to jbo-ads/mapcache that referenced this pull request Mar 24, 2022
jbo-ads added a commit that referenced this pull request Mar 24, 2022
* Make GDAL sources produce premultiplied RGBA (#282)
* Prepare for 1.12.1 release
* Fix check failure caused by wrong HTTP header removal on Windows

Co-authored-by: Deandre Metoyer <deepwns@live.com>
bob-beck pushed a commit to openbsd/ports that referenced this pull request Mar 29, 2022
@jmckenna jmckenna added this to the 1.14.0 release milestone Jan 29, 2023
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