DIPlib / diplib

Quantitative Image Analysis in C++, MATLAB and Python
https://diplib.org
Apache License 2.0
228 stars 50 forks source link

Access violation when indexing the dip image #92

Closed BoyuLyu closed 3 years ago

BoyuLyu commented 3 years ago

I was running a very simple test using C++ library diplib, but it returns an error at the CastSample function

Unhandled exception at 0x00007FF60C4044FA in debugx.exe: 0xC0000005: Access violation writing location 0x00007FFF00000009.

The code is very simple like below

#include <iostream>
#include <vector>
#include <diplib/math.h>
#include "diplib.h"
#include "diplib/file_io.h"
#include "diplib/linear.h"
#include "diplib/generation.h"
#include "diplib/mapping.h"
#include "diplib/statistics.h"
#include "diplib/distance.h"
using namespace std;
using namespace dip;

int main() {
    Image img({ 256, 256 }, 3);
    img.At(10, 20) = { 4, 5, 6 };
}

And how to convert from a dip.image into a normal type (like an array or vector in C++)? There is some information on how to use the Matlab interface, but in C++ how to convert from a vector to dip image, and then convert it back to a vector?

crisluengo commented 3 years ago

Your code looks fine, it should work. I’ll try to reproduce your issue later tonight. What OS and compiler are you using? Did the tests under make check pass?

There is no standard function to convert from an image to a vector, but it is relatively straightforward to encapsulate the data from a vector into an image, where the image points at the data in the vector and therefore modifying the image also modified the vector. This section of the documentation has an example: https://diplib.org/diplib-docs/dip-Image.html#external_data_segment

BoyuLyu commented 3 years ago

I am running on Windows 10, and I am using visual studio 2019. I didn't see any errors reported during the build using Cmake. But I got this error (shown below) while debugging. Screen Shot 2021-11-10 at 23 38 39

crisluengo commented 3 years ago

Does that happen debugging the program you posted at the top? Because there's no binary images in that program, so that line of code should never even be executed. Does this debugger also give you a stack trace? It would be informative to know how it got to that line of code. "Access violation writing location ..." means that it's trying to write to memory it is not supposed to write to.

I can run your program just fine on my machine.

You should have a "check" target in the DIPlib project, that you can build in VS. Please build it, it will run checks to see if the library was built correctly.

If that doesn't show any errors, then it may be the way that you build your program that causes this issue. Did you build a static or a dynamic library version of DIPlib? Are you building your program using CMake, or are you manually adjusting settings in the compiler?

BoyuLyu commented 3 years ago

Yes, that happened when I was debugging the program. I build the "check" target and it reports errors. image The errors come from the 'physical_dimensions.cpp' Also, I tried re-building DIPlib in VS and it also report errors, but they are all related to pyDIP. If I choose not to build PyDIP, there will be no errors while building the "INSTALL" target. image

Updates: I cleaned the target folder and the build folder. Rerun CMake and rebuild "INSTALL" without PyDIP. Then I build "check" and it reports the same error as the first image. Running the test program again gives me the same error. And by tracing the stack, I found that line of code for the binary image was called by Pixel::operator. image

BoyuLyu commented 3 years ago

Your code looks fine, it should work. I’ll try to reproduce your issue later tonight. What OS and compiler are you using? Did the tests under make check pass?

There is no standard function to convert from an image to a vector, but it is relatively straightforward to encapsulate the data from a vector into an image, where the image points at the data in the vector and therefore modifying the image also modified the vector. This section of the documentation has an example: https://diplib.org/diplib-docs/dip-Image.html#external_data_segment

Hi crisluengo,

Thanks for pointing out the part of the manual for creating the matrix. I am wondering if vector is the only way to transform from C++ to dipimage? Can C++ vector be converted to dipImage as well?

crisluengo commented 3 years ago

Many things here, let me address them one by one:

  1. The error in the "check" target is related to Unicode. There seems to be a bug in MSVC related to UTF-8 encoded strings. You can configure CMake with DIP_ENABLE_UNICODE turned off to avoid this issue.
  2. I still don't understand what is going on with your test program. How do you build it? Are you using CMake, or do you manually configure a project in MSVC? I think this is where the problem lies, we need to fix your program's configuration. Note that the "check" target tests very similar code to yours, and that worked just fine.
  3. If you don't need PyDIP, I guess we don't need to solve that problem, but I see "unresolved external symbol public: void __cdecl dip::viewer::Windows::setPosition(intent)". Why would that be __cdecl? That makes no sense, no wonder it cannot find that symbol. The automated test runner builds this just fine, and we have binaries on PyPI for Windows. This should work. Very weird!
  4. You can create a dip::Image around any data in memory, whether it's a std::vector<> or something else. You can also write code to copy data into an image or out of an image, if need be. But usually that is not necessary. What is the use case you're wondering about?
wcaarls commented 3 years ago

I could reproduce the PyDIP errors (in fact, I also get some from dip::FreeTypeTool), which can be solved by adding some DIPVIEWER_EXPORT (for FreeTypeTool: DIP_CLASS_EXPORT) macros. I don't know why we don't see these on the build server.

As for the test program, I copy-pasted it into the example independent project, and it works fine here.

crisluengo commented 3 years ago

@wcaarls DIP_CLASS_EXPORT is defined as nothing on Windows -- adding it should not change anything! The dip::FreeTypeTool class doesn't need to be exported because there's no inheritance. On POSIX systems classes in an inheritance hierarchy need to be exported for the hierarchy to match up across library boundaries. On Windows classes with the same name are always matched up properly, and we got all sorts of errors when exporting them. This is why, on Windows, we have #define DIP_CLASS_EXPORT DIP_NO_EXPORT.

I do see how dip::viewer::Window, not being exported on Windows, would need its members be explicitly exported if defined in the library and not the header.

wcaarls commented 3 years ago

I see! Must've been something with my build tree then. I reverted my changes and it still works.

BoyuLyu commented 3 years ago

Many things here, let me address them one by one:

  1. The error in the "check" target is related to Unicode. There seems to be a bug in MSVC related to UTF-8 encoded strings. You can configure CMake with DIP_ENABLE_UNICODE turned off to avoid this issue.
  2. I still don't understand what is going on with your test program. How do you build it? Are you using CMake, or do you manually configure a project in MSVC? I think this is where the problem lies, we need to fix your program's configuration. Note that the "check" target tests very similar code to yours, and that worked just fine.
  3. If you don't need PyDIP, I guess we don't need to solve that problem, but I see "unresolved external symbol public: void __cdecl dip::viewer::Windows::setPosition(intent)". Why would that be __cdecl? That makes no sense, no wonder it cannot find that symbol. The automated test runner builds this just fine, and we have binaries on PyPI for Windows. This should work. Very weird!
  4. You can create a dip::Image around any data in memory, whether it's a std::vector<> or something else. You can also write code to copy data into an image or out of an image, if need be. But usually that is not necessary. What is the use case you're wondering about?

Thanks for the reply! I was indeed using MSVC to build the test program. I just changed to CMake and it reports another error. image

crisluengo commented 3 years ago

That looks like it's the same problem as before, except it shows up in a different place. Please share your CMakeLists.txt file.

BoyuLyu commented 3 years ago

`cmake_minimum_required (VERSION 3.8)

project ("CMakeProject1")

list(APPEND CMAKE_PREFIX_PATH "C:/Users/boyu9/DIPlib") find_library(DIP_LIB DIP) find_path(DIP_INCLUDE diplib.h)

add_executable (CMakeProject1 "CMakeProject1.cpp" "CMakeProject1.h") target_link_libraries(CMakeProject1 PUBLIC "${DIP_LIB}") target_include_directories(CMakeProject1 PUBLIC "${DIP_INCLUDE}" )

` I am not quite familiar with CMake, there might be some errors.

update I also tried disenabled DIP_ENABLE_UNICODE. and it reports much fewer errors when building the "INSTALL" target.
image

crisluengo commented 3 years ago

Yes, this is not the right way to link DIPlib. The compiler needs to be configured properly, and just linking the library is not sufficient. In the repository there is an example CMake file at examples/independent_project/CMakeLists.txt.

You should do this:

cmake_minimum_required(VERSION 3.8)

project(CMakeProject1)

set(CMAKE_CXX_STANDARD 14)  # this might not be necessary?
set(CMAKE_CXX_STANDARD_REQUIRED ON)  # this might not be necessary?

find_package(DIPlib 3.0 REQUIRED DIP)

add_executable(CMakeProject1 CMakeProject1.cpp CMakeProject1.h)
target_link_libraries(CMakeProject1 PRIVATE DIPlib::DIP)

In the example file I mentioned above you can see what changes you need to make if you want to include DIPviewer as well.

By linking against the DIPlib::DIP imported target, you automatically configure the compilation of your program to match the requirements of the DIPlib library.

BoyuLyu commented 3 years ago

I might be wrong, but after changing this CmakeList.txt, the same error still exists in debugging. The error is related to DIP.dll. Does that mean DIP.dll was not well included? Also, what might be the reason that I got when building the "INSTALL" target? But the good news is building on the "check" target returns no error!

Update I tried using the Matlab interface, where I compiled the mex file using dipmex and it worked well. Does the matlab interface not call the DIP.dll?

BoyuLyu commented 3 years ago

Solved Just checked the error related to the Cmake build of the target "INSTALL", the 'setlocal' error turns out to be related to Admin rights. One way to fix is to change the "C:/users/[username]/DIPlib" folder to be not read-only. MSB3073 error

With the change, now the build process is error-free and the test program reports no error. Thank you Crisluengo for all the help!

crisluengo commented 3 years ago

@HHHit Fantastic! Thank you so much for reporting back.

wcaarls commented 3 years ago

@crisluengo It turns out the Windows test action does not build PyDIPviewer, which is why the unresolved symbols were not caught there. The deploy action fails without the extra DIPVIEWER_EXPORT macros. Apparently something changed since the last release.

crisluengo commented 3 years ago

@wcaarls Is a new (or different) version of the compiler being used? I don’t see any changes to dipviewer code except the extra checks you added recently.

wcaarls commented 3 years ago

@crisluengo I meant something changed in the build environment. Could be the compiler version, yes. Not sure.

crisluengo commented 3 years ago

@wcaarls I see the deploy_windows.bat script downloads freeglut sources and builds them. Is there no easy way to fetch binaries? Do we need to add that same build step to the test action to get it to build DIPviewer?

wcaarls commented 3 years ago

@crisluengo From what I remember, the Freeglut project itself doesn't make binaries available, and the standard(ish) place to get MSVC binaries from doesn't package the static libraries. I guess we could cache our own somewhere, but don't know whether it's worth it.

If we want to test building DIPviewer we could use the same step, although I think downloading the dynamic libraries should work as well.

wcaarls commented 3 years ago

@crisluengo I'm building it with GLFW now, since they have official binaries. Note: I tried to squash the commits, but cannot force-push.

crisluengo commented 3 years ago

@wcaarls Fantastic! Thanks!

Yes, force push is disabled on the master branch on purpose. Basic protection to avoid painful mistakes.