LoadPLYFile should be able to load large ply files correctly

[YouYue123]

Patch on top of #2145

Your Environment

• Operating System and version: Ubuntu 16.04
• Compiler: cmake 3.5.2 / make 4.1
• PCL Version: 1.8.1
• CPU: 36 cores / RAM: 64GB

Expected Behavior

pcl::io::LoadPLYFile should be able to load large ply files correctly

Current Behavior

While trying to load a 40 GB .ply point cloud using the following:

loadPLYFile<PointXYZ>(filePath, *object);

I get a point cloud which all points are (0, 0, 0)

Issue

In io/src/ply/ply_parser.cpp line 498, the ply parse function is trying to check whether end_header_callback_ is return true. Otherwise it will return from the ply parse procedure. And this return will remain all points as its initial value (0,0,0)

  // end_header
      else if (keyword == "end_header")
      {
        if (end_header_callback_)
        {
          if (end_header_callback_ () == false)
            return true;
        }
        break;
      }

And in io/src/ply/ply_io.cpp line 95, the endHeaderCallback is return a checking between data.size() with a expression cloud_->point_step * cloud_->width * cloud_->height which is not always true while cloud_->point_step * cloud_->width * cloud_->height is larger than 4,294,967,295(Max number of 32 bit integer)

bool
pcl::PLYReader::endHeaderCallback ()
{ 
  cloud_->data.resize (static_cast<size_t>(cloud_->point_step) * cloud_->width * cloud_->height);
  return (cloud_->data.size () == cloud_->point_step * cloud_->width * cloud_->height);
}

Resolution

As proposed in pull request, I changed the checking as below

bool
pcl::PLYReader::endHeaderCallback ()
{ 
  cloud_->data.resize (static_cast<size_t>(cloud_->point_step) * cloud_->width * cloud_->height);
  return (cloud_->data.size () == static_cast<size_t>(cloud_->point_step) * cloud_->width * cloud_->height);
}

It works in my env.

Context

We are trying to process large building information to be able to be processed by our computing engine

[taketwo]

Thanks for investigating this and proposing a fix.
I think this comparison is redundant, we should return true unconditionally. If vector resize failed to allocate enough space, then it will throw an exception anyway.

[YouYue123]

So will this fix already proposed and be provided in next release?

[taketwo]

Let's see what others have to say. But for sure we will merge this very soon and it will be in the next release.

[YouYue123]

That sounds cool. Anyway you can feel free to close the pull request

[YouYue123]

Nice to see this changes!