From 25c6576fddd14445b334665de074ce4dc9e8bd44 Mon Sep 17 00:00:00 2001
From: Blake Thompson <flippmoke@gmail.com>
Date: Mon, 18 May 2015 11:09:50 -0500
Subject: [PATCH] Updated the way that multiply_alpha works, such that it is
 not clamped at the multiplier, but rather at the result of the multiplier and
 alpha.

---
 benchmark/build.py                            |  1 +
 .../test_numeric_cast_vs_static_cast.cpp      | 87 +++++++++++++++++++
 src/image_util.cpp                            |  5 +-
 test/unit/imaging/image_multiply_alpha.cpp    |  2 +-
 4 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 benchmark/test_numeric_cast_vs_static_cast.cpp

diff --git a/benchmark/build.py b/benchmark/build.py
index 9b41cc748..e84f42136 100644
--- a/benchmark/build.py
+++ b/benchmark/build.py
@@ -43,6 +43,7 @@ benchmarks = [
     "test_font_registration.cpp",
     "test_rendering.cpp",
     "test_rendering_shared_map.cpp",
+#    "test_numeric_cast_vs_static_cast.cpp",
 ]
 for cpp_test in benchmarks:
     test_program = test_env_local.Program('out/'+cpp_test.replace('.cpp',''), source=[cpp_test])
diff --git a/benchmark/test_numeric_cast_vs_static_cast.cpp b/benchmark/test_numeric_cast_vs_static_cast.cpp
new file mode 100644
index 000000000..52865acd0
--- /dev/null
+++ b/benchmark/test_numeric_cast_vs_static_cast.cpp
@@ -0,0 +1,87 @@
+#include "bench_framework.hpp"
+// boost
+#include <boost/numeric/conversion/cast.hpp>
+
+static double STEP_NUM = 0.0000000001;
+static std::uint8_t START_NUM = 2;
+
+class test_static : public benchmark::test_case
+{
+    double step_;
+    std::uint8_t start_;
+public:
+    test_static(mapnik::parameters const& params)
+     : test_case(params),
+       step_(STEP_NUM),
+       start_(START_NUM) {}
+    bool validate() const
+    {
+        return true;
+    }
+    bool operator()() const
+    {
+        double value_ = 0.0;
+        std::uint8_t x;
+        for (std::size_t i=0;i<iterations_;++i) {
+            double c = static_cast<double>(start_) * value_;
+            if (c >= 256.0) c = 255.0;
+            if (c < 0.0) c = 0.0;
+            x = static_cast<std::uint8_t>(c);
+            value_ += step_;
+        }
+        return static_cast<double>(x) < (static_cast<double>(start_) * value_);
+    }
+};
+
+using boost::numeric::positive_overflow;
+using boost::numeric::negative_overflow;
+
+class test_numeric : public benchmark::test_case
+{
+    double step_;
+    std::uint8_t start_;
+public:
+    test_numeric(mapnik::parameters const& params)
+     : test_case(params),
+       step_(STEP_NUM),
+       start_(START_NUM) {}
+    bool validate() const
+    {
+        return true;
+    }
+    bool operator()() const
+    {
+        double value_ = 0.0;
+        std::uint8_t x;
+        for (std::size_t i=0;i<iterations_;++i) {
+            try {
+                x = boost::numeric_cast<std::uint8_t>(start_ * value_);
+            }
+            catch(negative_overflow&)
+            {
+                x = std::numeric_limits<std::uint8_t>::min();
+            }
+            catch(positive_overflow&)
+            {
+                x = std::numeric_limits<std::uint8_t>::max();
+            }
+            value_ += step_;
+        }
+        return static_cast<double>(x) < (static_cast<double>(start_) * value_);
+    }
+};
+
+int main(int argc, char** argv)
+{
+    mapnik::parameters params;
+    benchmark::handle_args(argc,argv,params);
+    {
+        test_static test_runner(params);
+        run(test_runner,"static_cast");
+    }
+    {
+        test_numeric test_runner(params);
+        run(test_runner,"numeric_cast");
+    }
+    return 0;
+}
diff --git a/src/image_util.cpp b/src/image_util.cpp
index f055416c1..bf1b792a4 100644
--- a/src/image_util.cpp
+++ b/src/image_util.cpp
@@ -713,7 +713,7 @@ namespace detail {
 struct visitor_multiply_alpha
 {
     visitor_multiply_alpha(float opacity)
-        : opacity_(clamp(opacity, 0.0f, 1.0f)) {}
+        : opacity_(opacity) {}
 
     void operator() (image_rgba8 & data) const
     {
@@ -724,7 +724,8 @@ struct visitor_multiply_alpha
             for (std::size_t x = 0; x < data.width(); ++x)
             {
                 pixel_type rgba = row_to[x];
-                pixel_type a = static_cast<uint8_t>(((rgba >> 24) & 0xff) * opacity_);
+                double new_a = static_cast<double>((rgba >> 24) & 0xff) * opacity_;
+                pixel_type a = static_cast<uint8_t>(clamp(new_a, 0.0, 255.0));
                 pixel_type r = rgba & 0xff;
                 pixel_type g = (rgba >> 8 ) & 0xff;
                 pixel_type b = (rgba >> 16) & 0xff;
diff --git a/test/unit/imaging/image_multiply_alpha.cpp b/test/unit/imaging/image_multiply_alpha.cpp
index 20bee304a..bc3a10e4d 100644
--- a/test/unit/imaging/image_multiply_alpha.cpp
+++ b/test/unit/imaging/image_multiply_alpha.cpp
@@ -71,7 +71,7 @@ SECTION("test rgba8 overflow") {
     CHECK(static_cast<int>(out.red()) == 128);
     CHECK(static_cast<int>(out.green()) == 128);
     CHECK(static_cast<int>(out.blue()) == 128);
-    CHECK(static_cast<int>(out.alpha()) == 128);
+    CHECK(static_cast<int>(out.alpha()) == 255);
 
 } // END SECTION