Added resize capability to Image object #476
Conversation
| template <typename T> | ||
| void operator()(T & im2) const | ||
| { | ||
| bool demultiply = mapnik::premultiply_alpha(im2); |
There was a problem hiding this comment.
Let's not mutate the image passed in. Instead expect it is premultiplied: #470
…ion to the way that resize operates such that it requires premultiplied images.
| @@ -26,6 +27,7 @@ | |||
| #include <ostream> // for operator<<, basic_ostream | |||
| #include <sstream> // for basic_ostringstream, etc | |||
| #include <cstdlib> | |||
| #include <iostream> | |||
|
|
|||
There was a problem hiding this comment.
Is #include <iostream> left in from debugging and can be removed?
|
Before merging just wanted to post some performance results: Using the performance comparison tools from https://github.com/lovell/sharp, I created tests for mapnik associated with JPEG. The change to the script is located here: https://github.com/flippmoke/sharp/blob/perf_test/test/bench/perf.js The results show the following: As you can see the
|
|
While travis is now passing, noticing that one appveyor build (x86 with node v0.12.x) appears to be crashing. I see: And then the text exits. So it appears that the tests may be crashing (usually an abrupt exit like this is a crash) on the next scaling test which I think is We don't have any way on appveyor that I know of to get backtraces like on unix (https://github.com/springmeyer/travis-coredump). /cc @BergWerkGIS in case he has ideas. |
|
The only thing I can think of at the moment is enabling local dumps: But they would have to be downloaded and analyzed offline. |
|
Tried to replicate locally (node 0.12.0 and latest source of But another one fails (does too in master): |
|
@flippmoke Some great performance results for mapnik there! Well done. Feel free to submit your benchmark additions back to The near-parity with libvips means you've probably also hit the (de)compression IO limits of libjpeg(-turbo). Do you think Mapbox might consider sponsoring the work required for https://sourceforge.net/p/libjpeg-turbo/feature-requests/34/ to provide a ~30% reduction in JPEG decompression times, at least for AVX2 CPUs? Great P(ublic) R(elations) opportunity :) |
|
Found the problem |
|
REMOVED |
|
Couldn't get current mapnik built, so I went back to STACK TRACE: Exception Analysis LOCALS @agg::image_filter_lut::calculate:
|
|
Did a full rebuild of everything: deps, mapnik, node and node-mapnik. |
|
@BergWerkGIS THANK YOU SO MUCH! That dump pointed me in the right direction and found uninitialized value in cef4100. Should be fixed now! /cc @springmeyer |
|
🚀 teamwork @BergWerkGIS @flippmoke. @flippmoke can you open a new ticket on running Also: I would expected that |
|
Glad I could help. |
Great question. It's hard to know of course without being the compiler author and without inspecting the memory layout of the program in detail. But we do know that an uninitialized value is "undefined" behavior in C/C++. So it is not surprising this problem surfaced. It is curious that it did not impact 64 bit systems, other node versions, or other platforms. It may be that the compilers on those systems chose to initialize the value to something more reasonable. Or it could be that the memory layout was such that the value was still huge but just not so huge to surface a problem. I think this is a clear case where supporting windows and cross platform development in general is a big factor in making the software better for everyone. I was certainly temped to ignore this error, but so glad you all persisted in fixing the bug - which would have maybe impacted everyone. For more details on undefined behavior see http://blog.regehr.org/archives/213. For more specifics on how uninitialized variables work (or don't) see https://en.wikipedia.org/wiki/Uninitialized_variable. The key point is that undefined behavior like this exists intentionally for performance reasons:
|
|
tests now passing on appveyor (https://ci.appveyor.com/project/Mapbox/node-mapnik/build/1.0.620) so @flippmoke I think this is ready to merge! |
|
@flippmoke - looks like this will not merge automatically because of conflicts in the changelog. I tend to add to the changelog after pulls are merged to avoid this. |
|
|
||
| void operator()(mapnik::image_null &) const | ||
| { | ||
| throw std::runtime_error("Can not resize null images"); |
There was a problem hiding this comment.
Can not currently reach this line, add COVERALLS ignoring to section.
|
@springmeyer thanks for the explanation about uninitialized variables. @springmeyer @flippmoke
I can offer, to either take a look from time to time and ticket the list of warnings, or intro you on how to use the tools.
|
|
@springmeyer revert |
|
@BergWerkGIS - yes, would be great to 1) create a separate ticket on any warnings you see, 2) I can help triage them (for example the one above is harmless) @flippmoke :+1 |
Added resize capability to Image object
No description provided.