gray fringing on edge between fully transparent and fully opaque pixels #1508

Closed
springmeyer opened this Issue Sep 28, 2012 · 21 comments

Comments

Projects
None yet
3 participants
Owner

springmeyer commented Sep 28, 2012

  • Impacts (at least) Mapnik 2.0.2 -> Master
  • Impacts raster display whenever any scaling is used other than near, in this case tested with bilinear
  • Impacts raster display even when NOT warping
  • can be worked around by not premultiplying the image before compositing

Similar to #1471, but instead of being at the raster edge it occurs within the bitmap.

Downstream report: http://support.mapbox.com/discussions/tilemill/1812-raster-scaling-artifacts-possible-bug

Testcase XML:

<Map srs="+init=EPSG:4326" background-color="white">
<Style name="test">
  <Rule>
    <RasterSymbolizer scaling="bilinear" />
  </Rule>
</Style>
<Layer name="test" srs="+init=EPSG:4326">
    <StyleName>test</StyleName>
    <Datasource>
        <Parameter name="file">test_wgs.tif</Parameter>
        <Parameter name="type">gdal</Parameter>
    </Datasource>
</Layer>
</Map>

test_wgs.tif is available at http://cl.ly/1E0l0G252y2V

actual:

expected:

Owner

springmeyer commented Sep 28, 2012

interestingly it does not occur in Mapnik master (but does in Mapnik 2.0.2) with this alternative source file: http://cl.ly/1u3W00033Q3R (this one was not previously reprojected with gdal). The test_wgs.tif was round triped using gdalwarp from wgs84 to merc and back. The problem first manifests in 900913 however - there is no need for the round trip - and thats the problem - that any wgs84 tiff like this warps to 900913 (to avoid mapnik raster reprojection bugs) will then hit this bug.

springmeyer pushed a commit that referenced this issue Sep 28, 2012

Owner

springmeyer commented Sep 28, 2012

expected image for the testcase was generated by turning off premultiplication of the dest (not a solution, but works around the problem to get the right general look)

springmeyer pushed a commit that referenced this issue Sep 28, 2012

more test fixes to set up to enforce desired behavior around tiff and…
… premultiplied alpha in source files - refs #1508 and #1511
Owner

springmeyer commented Sep 28, 2012

as @artem found, turns out this image is using premultiplied alpha, so we need to avoid premultiplying twice. Solved by 77e5858 for the raster.input, but the only way to fix for gdal is manually. So, closing this ticket and will track at #1512.

Owner

springmeyer commented Sep 28, 2012

re-opening. after testing with another image I'm not convinced this is properly solved (in other words I don't actually think the source image is premultiplied).

@springmeyer springmeyer reopened this Sep 28, 2012

Owner

springmeyer commented Sep 28, 2012

wrote a script to check for all unique colors in an image:

#!/usr/bin/env python

import sys
import mapnik

def pixel2rgba(pixel):
    alpha = (pixel >> 24) & 0xff
    red = pixel & 0xff
    green = (pixel >> 8) & 0xff
    blue = (pixel >> 16) & 0xff
    return '%s,%s,%s,%s' % (red,green,blue,alpha)

def get_unique_colors(im):
    pixels = []
    for x in range(im.width()):
        for y in range(im.height()):
            pixel = im.get_pixel(x,y)
            if pixel not in pixels:
                 pixels.append(pixel)
    return map(pixel2rgba,pixels)

if __name__ == '__main__':
    im = mapnik.Image.open(sys.argv[1])
    print get_unique_colors(im)

The new test image just has (as expected) two colors:

./unique-colors.py tests/data/raster/river_merc.tiff 
['0,0,0,0', '196,223,246,255']

GDAL is mis-reporting that the image is premultiplied (as per this as its not - it contains a faithful representation of the colormapping originally applied:

<ColorInterp>Palette</ColorInterp>
<ColorTable>
    <Entry c1="255" c2="255" c3="255" c4="0"/>
    <Entry c1="196" c2="223" c3="246" c4="255"/>
</ColorTable>

as per this.

Owner

springmeyer commented Sep 29, 2012

with further study it looks like to me that we need to premultiply before image scaling. So, this relates to the confusion noted in #1489

Contributor

lightmare commented Sep 29, 2012

@springmeyer Transparent white is lost, so it actually was premultiplied. The other color is preserved probably because the method was exact, not mapnik's fast approximation.

When comparing span_image_resample_agg_affine with original agg version, I noticed there are 2 changes. 1) mapnik protects against division by zero. 2) agg clamps output values to (A,A,A,255) (i.e. considers RGB premultiplied and ensures they don't overflow), while mapnik clamps to (255,255,255,255).

From reading the code, I have a feeling it can't work with premultiplied images, because it's not treating alpha specially. If you interpolate a pixel between (10,0,0,10) -> (20,0,0,40) you get (15,0,0,25) [premultiplied example], which after demultiplication is ~10% opaque, 60% red (153); but if you interpolate (255,0,0,10) -> (127,0,0,40) you get (191,0,0,25), which is ~10% opaque, 75% red. I'll post some notes I made about this a week or two ago to #1489.

Owner

artemp commented Oct 1, 2012

@springmeyer @lightmare - I agree we shouldn't pre-multiply before compositing ops.

Owner

springmeyer commented Oct 1, 2012

"I'd like to add that whenever bitmap
data is processed which contains transparent pixels, you absolutely need
to have this bitmap converted to premultiplied format to be able to filter
it in any way. Otherwise the semi transparent pixels will have wrong
weighting (too much influence), or you need to compensate this in your
filters (much more processing!). All the filters in AGG are written in a
way that they only work correctly with pre-multiplied image data. When you
retrieve your bitmap data from a file, you need to know wether the image
data is already pre-multiplied or not. For example, PNGs should not be
pre-multiplied while in TIFFs, it can be either way.
In AGG 2.4, most pixel_format classes have a function to pre-mutliply the
whole rendering buffer."

from http://permalink.gmane.org/gmane.comp.graphics.agg/3115

Contributor

lightmare commented Oct 1, 2012

Hm, seems like I added to the confusion thinking that alpha and color should be interpolated separately.

Owner

springmeyer commented Oct 2, 2012

Hey @lightmare no worries - I was confused before - your thoughts helped encourage me to research that a bit more. It seems like premultiplying before filtering may work for this specific case, but it breaks other cases. I'm working on getting better testcases set up now - will report more soon.

Owner

springmeyer commented Oct 2, 2012

okay, here is a reduced testcase that I don't think should have gray fringing (either with a solid background or a transparent background):

<Map srs="+init=epsg:4326" background-color="white">

<Style name="test">
  <Rule>
    <RasterSymbolizer scaling="bilinear"/>
  </Rule>
</Style>

<Layer name="test" srs="+init=epsg:4326">
    <StyleName>test</StyleName>
    <Datasource>
        <Parameter name="file">scaling.tiff</Parameter>
        <Parameter name="type">raster</Parameter>
        <Parameter name="extent">0,0,2,2</Parameter>
    </Datasource>
</Layer>

</Map>

source scaling.tiff is at http://cl.ly/0d1m3Z1a092x

nik2img.py scaling.xml -d 100 100 actual.png generates:

While changing the symbolizer to use premultiplied="true" can generate the expected image:

But the problem is that this workaround does not really fix the core bug - that is my hunch as least.

Owner

springmeyer commented Oct 2, 2012

realizing this misunderstanding goes way back. When I moved the bundled agg sources into the deps dir we lost the history, but the old trac site shows that craig tried/maybe did fix this with modifications to span_image that we have since reverted as per #1227 (because I did not know if they were correct and there was no clear testcase): http://173.255.217.246:8000/mapnik_trac/log/trunk/agg/include/agg_pixfmt_rgba.h.

My current sense now is that bilinear scaling in AGG produces premultiplied output, which is why we can fix this problem by avoiding premultiplying a second time before compositing. However, it should then also be fixable (in theory) by demultiplying after bilinear scaling and before premultipling again (before compositing), however demultiplying appears to segfault in agg.

Owner

springmeyer commented Oct 2, 2012

I noticed the segfault on demultiply when trying to save intermediate images to checkout output. Demultiplying should be needed to see the correct PNG of the intermediate image:

diff --git a/src/agg/process_raster_symbolizer.cpp b/src/agg/process_raster_symbolizer.cpp
index b828720..8ac7d9c 100644
--- a/src/agg/process_raster_symbolizer.cpp
+++ b/src/agg/process_raster_symbolizer.cpp
@@ -38,6 +38,10 @@
 // stl
 #include <cmath>

+// agg
+#include "agg_rendering_buffer.h"
+#include "agg_pixfmt_rgba.h"
+

 namespace mapnik {

@@ -98,6 +102,17 @@ void agg_renderer<T>::process(raster_symbolizer const& sym,
                                                    filter_radius);
                 }
             }
+            // oddly blank
+            mapnik::save_to_file(source->data_,"source.png");
+            // target that has been scaled - will have gray edges now if bilinear is used
+            mapnik::save_to_file(target.data_,"target_after_scaling.png");
+/*
+            agg::rendering_buffer buffer(target.data_.getBytes(),width_,height_,width_ * 4);
+            agg::pixfmt_rgba32 pixf(buffer);
+            // segfaults
+            pixf.demultiply();
+            mapnik::save_to_file(target.data_,"target_after_scaling_demultiplied.png");
+*/
             // handle whether to premultiply the source
             // data before compositing
             // first, default to what the data reports
Contributor

lightmare commented Oct 2, 2012

Shouldn't the buffer's dimensions be raster_width, raster_height?

On another note:

image_data_32 target_data(raster_width,raster_height);
raster target(target_ext, target_data);               

I think raster could have another constructor that would construct _data from width and height, instead of copying it from a dummy image_data instance, it's an extra new and memcpy.

Owner

springmeyer commented Oct 2, 2012

@lightmare - brilliant, totally missed that and had the wrong dimensions still locally - leading to crashes reading outside the image. thank you :) Will post my overall fix in a moment.

Owner

springmeyer commented Oct 2, 2012

This is my working fix:

remaining issues include:

  • src/warp.cpp also needs updated
  • test results are different for gdal/raster input in some cases, which I cannot explain right now
diff --git a/src/agg/process_raster_symbolizer.cpp b/src/agg/process_raster_symbolizer.cpp
index b828720..17d40d7 100644
--- a/src/agg/process_raster_symbolizer.cpp
+++ b/src/agg/process_raster_symbolizer.cpp
@@ -38,6 +38,10 @@
 // stl
 #include <cmath>

+// agg
+#include "agg_rendering_buffer.h"
+#include "agg_pixfmt_rgba.h"
+

 namespace mapnik {

@@ -70,6 +74,22 @@ void agg_renderer<T>::process(raster_symbolizer const& sym,
             raster target(target_ext, target_data);
             scaling_method_e scaling_method = sym.get_scaling_method();
             double filter_radius = sym.calculate_filter_factor();
+            // we need to premultiply alpha before scaling/compositing
+            // first, default to what the data reports
+            bool premultiply_source = !source->premultiplied_alpha_;
+            // then, allow the user to override
+            boost::optional<bool> is_premultiplied = sym.premultiplied();
+            if (is_premultiplied)
+            {
+                if (*is_premultiplied) premultiply_source = false;
+                else premultiply_source = true;
+            }
+            if (premultiply_source)
+            {
+                agg::rendering_buffer buffer(source->data_.getBytes(),source->data_.width(),source->data_.height(),source->data_.width() * 4);
+                agg::pixfmt_rgba32 pixf(buffer);
+                pixf.premultiply();
+            }
             if (!prj_trans.equal())
             {
                 double offset_x = ext.minx() - start_x;
@@ -98,20 +118,9 @@ void agg_renderer<T>::process(raster_symbolizer const& sym,
                                                    filter_radius);
                 }
             }
-            // handle whether to premultiply the source
-            // data before compositing
-            // first, default to what the data reports
-            bool premultiply_source = !source->premultiplied_alpha_;
-            // the, allow the user to override
-            boost::optional<bool> is_premultiplied = sym.premultiplied();
-            if (is_premultiplied)
-            {
-                if (*is_premultiplied) premultiply_source = false;
-                else premultiply_source = true;
-            }
             composite(current_buffer_->data(), target.data_,
                       sym.comp_op(), sym.get_opacity(),
-                      start_x, start_y, premultiply_source);
+                      start_x, start_y, false);
         }
     }
 }
diff --git a/src/image_scaling.cpp b/src/image_scaling.cpp
index f9c73e6..120698a 100644
--- a/src/image_scaling.cpp
+++ b/src/image_scaling.cpp
@@ -263,12 +263,12 @@ void scale_image_agg(Image & target,
                      double filter_radius,
                      double ratio)
 {
-    // TODO - should all types here be *_pre ?
     // "the image filters should work namely in the premultiplied color space"
     // http://old.nabble.com/Re:--AGG--Basic-image-transformations-p1110665.html
-    typedef agg::pixfmt_rgba32 pixfmt;
+    // "Yes, you need to use premultiplied images only. Only in this case the simple weighted averaging works correctly in the image fitering."
+    // http://permalink.gmane.org/gmane.comp.graphics.agg/3443
     typedef agg::pixfmt_rgba32_pre pixfmt_pre;
-    typedef agg::renderer_base<pixfmt_pre> renderer_base;
+    typedef agg::renderer_base<pixfmt_pre> renderer_base_pre;

     // define some stuff we'll use soon
     agg::rasterizer_scanline_aa<> ras;
@@ -278,15 +278,15 @@ void scale_image_agg(Image & target,

     // initialize source AGG buffer
     agg::rendering_buffer rbuf_src((unsigned char*)source.getBytes(), source.width(), source.height(), source.width() * 4);
-    pixfmt pixf_src(rbuf_src);
-    typedef agg::image_accessor_clone<pixfmt> img_src_type;
+    pixfmt_pre pixf_src(rbuf_src);
+    typedef agg::image_accessor_clone<pixfmt_pre> img_src_type;
     img_src_type img_src(pixf_src);

     // initialize destination AGG buffer (with transparency)
     agg::rendering_buffer rbuf_dst((unsigned char*)target.getBytes(), target.width(), target.height(), target.width() * 4);
     pixfmt_pre pixf_dst(rbuf_dst);
-    renderer_base rb_dst(pixf_dst);
-    rb_dst.clear(agg::rgba(0, 0, 0, 0));
+    renderer_base_pre rb_dst_pre(pixf_dst);
+    rb_dst_pre.clear(agg::rgba(0, 0, 0, 0));

     // create a scaling matrix
     agg::trans_affine img_mtx;
@@ -311,7 +311,7 @@ void scale_image_agg(Image & target,
     {
         typedef agg::span_image_filter_rgba_nn<img_src_type, interpolator_type> span_gen_type;
         span_gen_type sg(img_src, interpolator);
-        agg::render_scanlines_aa(ras, sl, rb_dst, sa, sg);
+        agg::render_scanlines_aa(ras, sl, rb_dst_pre, sa, sg);
         return;
     }
     case SCALING_BILINEAR:
@@ -348,9 +348,12 @@ void scale_image_agg(Image & target,
     case SCALING_BLACKMAN:
         filter.calculate(agg::image_filter_blackman(filter_radius), true); break;
     }
+    // http://old.nabble.com/Re%3A-Newbie---texture-p5057255.html
     typedef mapnik::span_image_resample_rgba_affine<img_src_type> span_gen_type;
+    //typedef agg::span_image_resample_rgba_affine<img_src_type> span_gen_type;
+    //typedef agg::span_image_filter_rgba<img_src_type,interpolator_type> span_gen_type;
     span_gen_type sg(img_src, interpolator, filter);
-    agg::render_scanlines_aa(ras, sl, rb_dst, sa, sg);
+    agg::render_scanlines_aa(ras, sl, rb_dst_pre, sa, sg);
 }

 template void scale_image_agg<image_data_32> (image_data_32& target,const image_data_32& source, scaling_method_e scaling_method, double scale_factor, double x_off_f, double y_off_f, double filter_radius, double ratio);

springmeyer pushed a commit that referenced this issue Oct 2, 2012

better output from pure white raster test checking since this is abou…
…t to start failing due to premultiplied rounding - refs #1508
Owner

springmeyer commented Oct 3, 2012

@lightmare - thanks for suggestion of more efficient raster ctor - added in 2e737e0

springmeyer pushed a commit that referenced this issue Oct 3, 2012

agg scaling assumes/requires a premulitiplied renderer (and optionall…
…y a source defined as premultiplied) - to avoid any confusion use purely premultiplied types - refs #1508 and #1489
Owner

springmeyer commented Oct 3, 2012

okay, 8674e46 fixes this problem for gdal.input in both the cairo and agg renderers. The problem mostly manifested in the agg renderer because we were effectively premultiplying twice - once given the result of scaling was output as premultiplied, and a second time (in the agg renderer) before compositing (because we assumed non-premultiplied source still).

The critical tests are now passing - gdal input based tests that show the gray fringing is avoided. However, one new test is failing and some existing raster.input tests are still failing. I will create new tickets to track these failures and possible reasons/fixes.

~/projects/mapnik5[master]$ make test
*** Running visual tests...
---------------------------------
Visual text rendering: 8 failures
---------------------------------
1) 40200 different pixels:
        /tmp/mapnik-visual-images/tiff-alpha-raster-600-agg.png (actual)
        tests/visual_tests/images/tiff-alpha-raster-600-reference.png (expected)
2) 40200 different pixels:
        /tmp/mapnik-visual-images/tiff-alpha-raster-600-cairo.png (actual)
        tests/visual_tests/images/tiff-alpha-raster-600-reference.png (expected)
3) 200 different pixels:
        /tmp/mapnik-visual-images/tiff-alpha-broken-assoc-alpha-raster-600-agg.png (actual)
        tests/visual_tests/images/tiff-alpha-broken-assoc-alpha-raster-600-reference.png (expected)
4) 200 different pixels:
        /tmp/mapnik-visual-images/tiff-alpha-broken-assoc-alpha-raster-600-cairo.png (actual)
        tests/visual_tests/images/tiff-alpha-broken-assoc-alpha-raster-600-reference.png (expected)
5) 42 different pixels:
        /tmp/mapnik-visual-images/tiff-nodata-edge-raster-600-agg.png (actual)
        tests/visual_tests/images/tiff-nodata-edge-raster-600-reference.png (expected)
6) 42 different pixels:
        /tmp/mapnik-visual-images/tiff-nodata-edge-raster-600-cairo.png (actual)
        tests/visual_tests/images/tiff-nodata-edge-raster-600-reference.png (expected)
7) 132 different pixels:
        /tmp/mapnik-visual-images/tiff-opaque-edge-raster-256-agg.png (actual)
        tests/visual_tests/images/tiff-opaque-edge-raster-256-reference.png (expected)
8) 132 different pixels:
        /tmp/mapnik-visual-images/tiff-opaque-edge-raster-256-cairo.png (actual)
        tests/visual_tests/images/tiff-opaque-edge-raster-256-reference.png (expected)
*** Running C++ tests...
C++ AGG blending: ✓ 
C++ CSV parse: ✓ 
C++ exceptions: ✓ 
C++ fonts registration: ✓ 
C++ label algorithms: ✓ 
C++ parameters: ✓ 
*** Running python tests...
Notice: skipping postgis tests (connection)
................grain_merge not validly pre-:
        690 pixels (rgba(253,144,0,248) at 74,112)
......................................................T.........................TT..............................................................................................................................T.................................................................................................................................................................................................F.......................T.....................
======================================================================
FAIL: python_tests.raster_symbolizer_test.test_raster_with_alpha_blends_correctly_with_background
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Python/2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/dane/projects/mapnik5/tests/python_tests/raster_symbolizer_test.py", line 154, in test_raster_with_alpha_blends_correctly_with_background
    eq_(contains_word('\xff\xff\xff\xff', imdata),True,'Image expected to contain true white, instead found %s' % get_unique_colors(mim))
  File "/Library/Python/2.7/site-packages/nose/tools.py", line 31, in eq_
    assert a == b, msg or "%r != %r" % (a, b)
AssertionError: Image expected to contain true white, instead found ['rgba(254,254,254,255)']

----------------------------------------------------------------------
Ran 464 tests in 7.791s

FAILED (TODO=5, failures=1)
make: *** [test] Error 1

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

better output from pure white raster test checking since this is abou…
…t to start failing due to premultiplied rounding - refs #1508

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

agg scaling assumes/requires a premulitiplied renderer (and optionall…
…y a source defined as premultiplied) - to avoid any confusion use purely premultiplied types - refs #1508 and #1489

PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment