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

fix incorrect comparison in BufferedOutputStream #1010

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

dllu
Copy link

@dllu dllu commented Jun 19, 2023

This bug was causing some unintuitive behavior such as a maximum file size of 2 GiB on POSIX systems which I mentioned in #1008 (reply in thread)

Intuitively, imagine if len = 5676725273 and bufSize = 131072. Previously, the loop would iterate once.

  • On the first iteration, n = MIN(bufSize - pos, len) gets evaluated to bufSize. After copying 131072 bytes to the buffer, it goes onto the next iteration.
  • On the second iteration, pos == bufSize. It writes the 131072 bytes and resets pos to zero. Now, len (which is 5676725273 - 131072 = 5676594201) is much bigger than bufSize so it calls this->s->write(b, len). This is an invalid operation since the len is much bigger than the buffer. In fact, the linux write function can only write 2147479552 bytes. So in File.h, the if (x < (ssize_t)len) fails with STREAM_ERROR.

The loop quits with STREAM_ERROR but we never check the return status of ostream->write here in PLY.cpp so we end up with a mysterious 2 GB file.

This PR fixes it to the intended behavior where we write the buffer size and then this branch only writes whatever straggler is left behind on the last iteration. I've verified that it works by printing out debug statements and running DensifyPointCloud --estimate-scale 1 it on a large point cloud.

With this fix, densified point clouds with scale can now go over 2^31 bytes. For example:

$ ls -al
total 17169804
drwxrwsr-x 1 dllu    dllu          0 Jun 19 18:04 .
drwxrwsr-x 1 dllu    dllu          0 Jun 15 17:53 ..
-rw-rw-r-- 1 dllu    dllu   42759713 Jun 15 21:40 asdf.mvs
-rw-rw-r-- 1 dllu    dllu 7482937566 Jun 15 22:55 asdf_dense.mvs
-rw-rw-r-- 1 dllu    dllu 4379289198 Jun 15 22:55 asdf_dense.ply
-rw-r--r-- 1 dllu    dllu 5676856345 Jun 19 18:07 asdf_dense_dense_scale.ply
$ head asdf_dense.ply
ply
format binary_little_endian 1.0
element vertex 162195887
property float32 x
property float32 y
property float32 z
property uint8 red
property uint8 green
property uint8 blue
property float32 nx
$ head asdf_dense_dense_scale.ply
ply
format binary_little_endian 1.0
element vertex 162195887
property float32 x
property float32 y
property float32 z
property uint8 red
property uint8 green
property uint8 blue
property float32 nx

@cdcseacave cdcseacave added the bug label Jun 20, 2023
@cdcseacave
Copy link
Owner

Thank you for finding and fixing this bug!

@cdcseacave cdcseacave merged commit 2d95bdc into cdcseacave:develop Jun 20, 2023
@cdcseacave cdcseacave mentioned this pull request Jul 4, 2023
cdcseacave added a commit that referenced this pull request Jul 4, 2023
 - add python API
 - COLMAP support in MvgMvsPipeline.py
 - interface for binary COLMAP
 - interface for Polycam scenes
 - tower mode #1017
 - estimate 3D points scale
 - transform scene by a given transform matrix
 - unify Docker scripts and add support for GUI
 - fix incorrect comparison in BufferedOutputStream #1010
 - add lines structure
 - compute the focus of attention of a set of cameras
 - add image mask support in mesh texturing
@cdcseacave
Copy link
Owner

cdcseacave commented Jul 16, 2023

@dllu can you pls add also a test for this issue (even a more general one for the buffer layers) in Tests?
I doing a large refactoring of the Streams functionality and I want to make sure I do not break something
it would be of great help if you can
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants