Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Access violation using IBA::to_OpenCV() with an ImageCache-backed ImageBuf #3800

Closed
jorgemb opened this issue Apr 11, 2023 · 15 comments · Fixed by #4013
Closed

[BUG] Access violation using IBA::to_OpenCV() with an ImageCache-backed ImageBuf #3800

jorgemb opened this issue Apr 11, 2023 · 15 comments · Fixed by #4013
Labels
image processing Related to ImageBufAlgo or other image processing topic.

Comments

@jorgemb
Copy link

jorgemb commented Apr 11, 2023

Describe the bug
I get an Exception 0xc0000005 encountered at address ***: Access violation reading location 0x00000000 while trying to convert a HEIC image to OpenCV format. Here is the PoC I'm working with:

#include <algorithm>
#include <filesystem>
#include <format>
#include <iostream>
#include <ranges>
#include <string>
#include <vector>

#include <OpenImageIO/imagebuf.h>
#include <OpenImageIO/imagebufalgo.h>
#include <OpenImageIO/imageio.h>
#include <opencv2/core.hpp>
#include <opencv2/highgui.hpp>

#include "lib.hpp"

namespace fs = std::filesystem;
namespace rng = std::ranges;
namespace vws = std::ranges::views;

auto main() -> int
{
  // Get image paths
  auto images = std::vector<fs::path> {};
  std::transform(fs::directory_iterator("img/"),
                 fs::directory_iterator(),
                 std::back_inserter(images),
                 [](auto const& entry) { return entry.path(); });

  for (auto const& image : images) {
    auto loaded_image = OIIO::ImageBuf(image.string());
    if (not loaded_image.initialized()) {
      std::cout << loaded_image.geterror() << '\n';
      continue;
    }

    auto cv_image = cv::Mat {};
    try {
      OIIO::ImageBufAlgo::to_OpenCV(cv_image, loaded_image, {}, 1);
      cv::imshow(image.filename().string(), cv_image);
    }catch(const std::exception& e){
      std::cout << "Error when converting: " << e.what() << "\n";
      return -1;
    }
  }

  cv::waitKey();
  return 0;
}

To Reproduce

  1. I'm using vcpkg on Windows with openimageio[libheif,opencv] as dependencies
  2. In the folder I have .heic images taken with an iPhone

Expected behavior
I would expect to see the images shown via OpenCV highUI.

Evidence
When debugging I get a Exception 0xc0000005 encountered at address ***: Access violation reading location 0x00000000 exception.

Platform information:

  • OIIO branch/version: 2.4.9.0
  • OS: Windows 11
  • C++ compiler: Microsoft (R) C/C++ Optimizing Compiler Version 19.35.32215 for x64
  • Any non-default build flags when you build OIIO:
@lgritz lgritz added file formats Image file formats, ImageInput, ImageOutput good first issue Good one-day project for beginners without much knowledge of the code base. labels Sep 27, 2023
@lgritz
Copy link
Collaborator

lgritz commented Sep 27, 2023

Sorry this went so long without a response.

Do you run into this problem when reading any heic image? If only some of them trigger the exception, can you please supply one that can reproduce the problem?

@jorgemb
Copy link
Author

jorgemb commented Oct 5, 2023

Hi, no problem. Thanks for reaching out.

Yes, in general with heic images. I downloaded this test image (attached image1.zip) from here: https://github.com/tigranbs/test-heic-images.

I tried today with version 2.4.14.0#2 (using vcpkg as toolchain) and the error persists.

Here is the minimal code to reproduce:

#include <iostream>
#include <string>
#include <OpenImageIO/imagebuf.h>
#include <OpenImageIO/imagebufalgo.h>
#include <opencv2/core.hpp>
#include <opencv2/highgui.hpp>

using namespace std::string_literals;

auto main() -> int
{
  auto file_path = "image1.heic"s;

  auto loaded_image = OIIO::ImageBuf(file_path);
  if (not loaded_image.initialized()){
    std::cout << loaded_image.geterror() <<  'n';
    return -1;
  }

  auto cv_image = cv::Mat{};
  try{
    OIIO::ImageBufAlgo::to_OpenCV(cv_image, loaded_image, {}, 1);
  }catch(const std::exception& e){
    std::cout << "Error when converting: " << e.what() << '\n';
    return -1;
  }

  cv::imshow(file_path, cv_image);
  cv::waitKey();

  return 0;
}

@lgritz
Copy link
Collaborator

lgritz commented Oct 6, 2023

Thanks for the example. I'm unable to reproduce it on my end, it works fine. (In this case, I'm using OIIO master, on MacOS, with OpenCV 4.8.1).

The Access violation reading location 0x00000000 (that's a null pointer) has me suspicious that the cv::Mat never gets allocated because to_OpenCV is not doing anything at all.

I would like to suggest that you change the part of your program that calls to_OpenCV to include some error checking:

    bool ok = OIIO::ImageBufAlgo::to_OpenCV(cv_image, loaded_image, {}, 1);
    if (!ok){
        std::cout << "Error when converting: " << OIIO::geterror() << '\n';
        return -1;
    }

Does this print anything that might give us additional clues?

@jorgemb
Copy link
Author

jorgemb commented Oct 6, 2023

Ohh, that is strange 🤔
Funny thing is that the exception interrupts the program, it is not even caught by the try-catch statement.
image

I'll try again under WSL and see if I get different results using GCC. This could mean it is an environment issue with my Windows 11 and msvc configuration.

I'm attaching the full PoC project with vcpkg and cmake configuration. Probably I have something wrong with how I'm importing the packages.
test_heic.zip

I'll report back with my tests.

@jorgemb
Copy link
Author

jorgemb commented Oct 6, 2023

Got a Segmentation fault 👀
image

Built using gcc 11.4.0-1ubuntu1~22.04

@lgritz
Copy link
Collaborator

lgritz commented Oct 6, 2023

Can you build OIIO in debug mode, run this in gdb and get a stack trace that tells you the full call stack for where this is crashing?

@lgritz lgritz removed the good first issue Good one-day project for beginners without much knowledge of the code base. label Oct 8, 2023
@jorgemb
Copy link
Author

jorgemb commented Oct 10, 2023

Hi, sorry for taking so long, I had issues with my dev environment.
I run it as you said and got the following stacktrace:

Thread 1 "test_heic" received signal SIGSEGV, Segmentation fault.
__memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:317
317     ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
(gdb) backtrace
#0  __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:317
#1  0x00005555559e42cb in OpenImageIO_v2_4::copy_image (nchannels=3, width=3992, height=2992, depth=1, src=0x0,
    pixelsize=3, src_xstride=3, src_ystride=11976, src_zstride=0, dst=0x7ffff4d1b040, dst_xstride=3,
    dst_ystride=11976, dst_zstride=0)
    at /home/jorge/vcpkg/buildtrees/openimageio/src/v2.4.14.0-e861b9d6fc.clean/src/libOpenImageIO/imageio.cpp:842
#2  0x00005555559e396f in OpenImageIO_v2_4::convert_image (nchannels=3, width=3992, height=2992, depth=1, src=0x0,
    src_type=..., src_xstride=3, src_ystride=11976, src_zstride=0, dst=0x7ffff4d1b040, dst_type=..., dst_xstride=3,
    dst_ystride=11976, dst_zstride=0)
    at /home/jorge/vcpkg/buildtrees/openimageio/src/v2.4.14.0-e861b9d6fc.clean/src/libOpenImageIO/imageio.cpp:743
#3  0x00005555559e3f11 in OpenImageIO_v2_4::parallel_convert_image (nchannels=3, width=3992, height=2992, depth=1,
    src=0x0, src_type=..., src_xstride=3, src_ystride=11976, src_zstride=0, dst=0x7ffff4d1b040, dst_type=...,
    dst_xstride=3, dst_ystride=11976, dst_zstride=0, nthreads=1)
    at /home/jorge/vcpkg/buildtrees/openimageio/src/v2.4.14.0-e861b9d6fc.clean/src/libOpenImageIO/imageio.cpp:797
#4  0x00005555558f89fd in OpenImageIO_v2_4::parallel_convert_image (nchannels=3, width=3992, height=2992, depth=1,
    src=0x0, src_type=..., src_xstride=3, src_ystride=11976, src_zstride=0, dst=0x7ffff4d1b040, dst_type=...,
    dst_xstride=3, dst_ystride=11976, dst_zstride=0, nthreads=1)
    at /home/jorge/vcpkg/buildtrees/openimageio/src/v2.4.14.0-e861b9d6fc.clean/src/include/OpenImageIO/imageio.h:3060
#5  0x00005555558ef851 in OpenImageIO_v2_4::ImageBufAlgo::to_OpenCV (dst=..., src=..., roi=..., nthreads=1)
    at /home/jorge/vcpkg/buildtrees/openimageio/src/v2.4.14.0-e861b9d6fc.clean/src/libOpenImageIO/imagebufalgo_opencv.cpp:346
#6  0x000055555567d68f in main () at /home/jorge/test_heic/source/main.cpp:22

I'm running it within WSL2 on Windows 11 using GCC 11.4.0

@jorgemb
Copy link
Author

jorgemb commented Oct 11, 2023

Ok, so I'm going through it step by step. When it reaches the copy_image function the src parameter is already NULL. Tracing back into the library I found this:

On imagebufalgo_opencv.cpp:

bool
ImageBufAlgo::to_OpenCV(cv::Mat& dst, const ImageBuf& src, ROI roi,
                        int nthreads)
{
   ... 
   bool converted   = parallel_convert_image(
        chans, roi.width(), roi.height(), 1,
        src.pixeladdr(roi.xbegin, roi.ybegin, roi.zbegin, roi.chbegin),
        spec.format, spec.pixel_bytes(), spec.scanline_bytes(), 0, dst.ptr(),
        dstSpecFormat, pixelsize, linestep, 0, -1, -1, nthreads);
   ...
}

By this moment the source is still valid, but src.pixeladdr(...) is returning nullptr. Because of this part:

On imagebuf.cpp

void*
ImageBufImpl::pixeladdr(int x, int y, int z, int ch)
{
    validate_pixels();
    if (cachedpixels())
        return nullptr;
    x -= m_spec.x;
    y -= m_spec.y;
    z -= m_spec.z;
    size_t p = y * m_ystride + x * m_xstride + z * m_zstride
               + ch * m_channel_stride;
    return &(m_localpixels[p]);
}

cachedpixels() returns false, making the whole thing return nullptr and then the copy_image eventually fails.+

Nonetheless, adding loaded_image.read() before the conversion doesn't solve the issue.

@jorgemb
Copy link
Author

jorgemb commented Oct 11, 2023

The issue was solved adding this before the conversion step:

loaded_image.read(
      0,
      0,
      /*force=*/true
      );

For some reason the image is not being loaded on demand for HEIF in particular 🤔

@lgritz
Copy link
Collaborator

lgritz commented Oct 12, 2023

Yes, in retrospect, this all makes sense! What's happening is that the IB is read lazily, and in this case it's backed underneath by an ImageCache. For such images, the pixeladdr() will be null, but of course, to_OpenCV needs the whole thing in the buffer. Duh. I will have a patch submitted shortly.

@lgritz lgritz added image processing Related to ImageBufAlgo or other image processing topic. and removed file formats Image file formats, ImageInput, ImageOutput labels Oct 12, 2023
@lgritz lgritz changed the title [BUG] Access violation reading when converting a HEIC image to OpenCV format [BUG] Access violation using IBA::to_OpenCV() with an ImageCache-backed ImageBuf Oct 12, 2023
@lgritz
Copy link
Collaborator

lgritz commented Oct 12, 2023

I think this is the correct fix: #4013

@jorgemb
Copy link
Author

jorgemb commented Oct 12, 2023

Awesome! Thank you @lgritz 🙌

@lgritz
Copy link
Collaborator

lgritz commented Oct 12, 2023

@jorgemb Are you able to test that patch on your end to verify that it fully solves it for you?

@jorgemb
Copy link
Author

jorgemb commented Oct 14, 2023

Hi! Yes, tested it locally and fully solved the issue for me.

@lgritz
Copy link
Collaborator

lgritz commented Oct 14, 2023

Thanks, @jorgemb. This fix will be in the next OIIO release.

lgritz added a commit that referenced this issue Oct 14, 2023
We were using parallel_convert_image but not realizing that asking for
the IB::localpixels() would give us nullptr for an IC-backed image.

Instead, "wrap" the cv::Mat with an IB and then use IBA::copy(). This
handles both in-memory and IC-backed ImageBuf's, and also has many other
advantages (such as format conversion) we may wish to take advantage of
in the future.

Fixes #3800

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Oct 14, 2023
…reFoundation#4013)

We were using parallel_convert_image but not realizing that asking for
the IB::localpixels() would give us nullptr for an IC-backed image.

Instead, "wrap" the cv::Mat with an IB and then use IBA::copy(). This
handles both in-memory and IC-backed ImageBuf's, and also has many other
advantages (such as format conversion) we may wish to take advantage of
in the future.

Fixes AcademySoftwareFoundation#3800

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit that referenced this issue Oct 20, 2023
Testing to make sure we can convert an IC-backed IB to a cv::Mat.

Related to issue #3800 and PR #4013

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Oct 20, 2023
…reFoundation#4013)

We were using parallel_convert_image but not realizing that asking for
the IB::localpixels() would give us nullptr for an IC-backed image.

Instead, "wrap" the cv::Mat with an IB and then use IBA::copy(). This
handles both in-memory and IC-backed ImageBuf's, and also has many other
advantages (such as format conversion) we may wish to take advantage of
in the future.

Fixes AcademySoftwareFoundation#3800

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Oct 20, 2023
Testing to make sure we can convert an IC-backed IB to a cv::Mat.

Related to issue AcademySoftwareFoundation#3800 and PR AcademySoftwareFoundation#4013

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Oct 20, 2023
Testing to make sure we can convert an IC-backed IB to a cv::Mat.

Related to issue AcademySoftwareFoundation#3800 and PR AcademySoftwareFoundation#4013

Signed-off-by: Larry Gritz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
image processing Related to ImageBufAlgo or other image processing topic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants