Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Also replace mask file extension to .png instead of appending#3611

Merged
ahojnnes merged 8 commits into
colmap:maincolmap/colmap:mainfrom
Dawars:mask_replace_extensionDawars/colmap:mask_replace_extensionCopy head branch name to clipboard
Sep 27, 2025
Merged

Also replace mask file extension to .png instead of appending#3611
ahojnnes merged 8 commits into
colmap:maincolmap/colmap:mainfrom
Dawars:mask_replace_extensionDawars/colmap:mask_replace_extensionCopy head branch name to clipboard

Conversation

@Dawars

@Dawars Dawars commented Sep 4, 2025

Copy link
Copy Markdown
Contributor

Sometimes the input images are a mix of .png and other extensions.
To handle the mask filenames, which are always .png, it is cleaner to replace the image extension instead of appending.
This would avoid filenames such as filename.jpg.png and filename.png.png

Change:
If checking fails for appending .png to the mask path, I also check by replacing the file extension by .png.

@Dawars

Dawars commented Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Is there any reason against merging this feature?

ahojnnes
ahojnnes reviewed Sep 24, 2025
View reviewed changes
Comment thread src/colmap/controllers/image_reader.cc Outdated
@ahojnnes

ahojnnes commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

Thanks and apologies for not looking at this earlier. I am surprised this doesn't already work and left a comment for you. If it's not already covered by a test, we should add a unit test to image_reader_test.cc to cover this new behavior. Thank you.

@Dawars
Unifying mask path checking
881dbe0
@Dawars
Add test case for reading .jpg images.
54c28fa 
 Checking mask path for appending and replacing image_name extension
@Dawars

Dawars commented Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

This wasn't discovered because the test is only testing for .png images.

  • I added an option to set the image extension
  • Now for the first image the mask path is appended and for the rest the extension is replaced.

When running the test on my machine I get the following error, not sure if it's relevant
E0924 15:20:55.321195 1165799 image_reader.cc:103] Check failed: image_index_ <= options_.image_names.size() (6 vs. 5)

Let me know if you need any changes, I don't have a lot of experience writing tests

@Dawars
Dawars requested a review from ahojnnes September 24, 2025 13:24
ahojnnes
ahojnnes approved these changes Sep 24, 2025
View reviewed changes

@ahojnnes ahojnnes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, LGTM.

Comment thread src/colmap/controllers/image_reader.cc Outdated
@ahojnnes
ahojnnes enabled auto-merge (squash) September 24, 2025 16:47
auto-merge was automatically disabled September 24, 2025 19:10

Head branch was pushed to by a user without write access

@Dawars

Dawars commented Sep 25, 2025

Copy link
Copy Markdown
Contributor Author

The test fails due to jpg compression being lossy as far as I can tell, how should we handle it?

[ RUN      ] ImageReaderTests/ParameterizedImageReaderTests.Nominal/3
I20250924 16:50:43.943231 10168 testing.cc:51] Creating test directory: C:\Users\RUNNER~1\AppData\Local\Temp\colmap_test_data\ImageReaderTests/ParameterizedImageReaderTests.Nominal/3
..\src\colmap\controllers\image_reader_test.cc(131): error: Expected equality of these values:
  bitmap.ConvertToRowMajorArray()
    Which is: { '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }
  test_bitmap.ConvertToRowMajorArray()
    Which is: { '\x1' (1), '\x1' (1), '\x1' (1), '\0', '\0', '\0', '\0', '\0', '\0' }

..\src\colmap\controllers\image_reader_test.cc(131): error: Expected equality of these values:
  bitmap.ConvertToRowMajorArray()
    Which is: { '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }
  test_bitmap.ConvertToRowMajorArray()
    Which is: { '\x1' (1), '\x1' (1), '\x1' (1), '\0', '\0', '\0', '\0', '\0', '\0' }

..\src\colmap\controllers\image_reader_test.cc(131): error: Expected equality of these values:
  bitmap.ConvertToRowMajorArray()
    Which is: { '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }
  test_bitmap.ConvertToRowMajorArray()
    Which is: { '\x1' (1), '\x1' (1), '\x1' (1), '\0', '\0', '\0', '\0', '\0', '\0' }

..\src\colmap\controllers\image_reader_test.cc(131): error: Expected equality of these values:
  bitmap.ConvertToRowMajorArray()
    Which is: { '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }
  test_bitmap.ConvertToRowMajorArray()
    Which is: { '\x1' (1), '\x1' (1), '\x1' (1), '\0', '\0', '\0', '\0', '\0', '\0' }

..\src\colmap\controllers\image_reader_test.cc(131): error: Expected equality of these values:
  bitmap.ConvertToRowMajorArray()
    Which is: { '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0' }
  test_bitmap.ConvertToRowMajorArray()
    Which is: { '\x1' (1), '\x1' (1), '\x1' (1), '\0', '\0', '\0', '\0', '\0', '\0' }

E20250924 16:50:43.952133 10168 image_reader.cc:103] Check failed: image_index_ <= options_.image_names.size() (6 vs. 5) 
[  FAILED  ] ImageReaderTests/ParameterizedImageReaderTests.Nominal/3, where GetParam() = (5, true, false, true, ".jpg") (10 ms)

@ahojnnes

ahojnnes commented Sep 27, 2025

Copy link
Copy Markdown
Contributor

You could use another lossess image format like bmp instead of jpg or cross fingers that explicitly setting a jpeg quality of 100 through the "flags" parameter in Bitmap::Write() makes the roundtrip lossess.

@ahojnnes
ahojnnes enabled auto-merge (squash) September 27, 2025 08:55
@ahojnnes

ahojnnes commented Sep 27, 2025

Copy link
Copy Markdown
Contributor

Thank you!

@ahojnnes
ahojnnes merged commit fd1fc1c into colmap:main Sep 27, 2025
13 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Nov 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahojnnes ahojnnes ahojnnes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dawars @ahojnnes
Morty Proxy This is a proxified and sanitized view of the page, visit original site.