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

Update panorama pipeline for very large panoramas #1244

Merged
merged 19 commits into from
Feb 21, 2023

Conversation

servantftechnicolor
Copy link
Contributor

  • Panorama warping, compositing and imageprocessing fail when the final panorama is too large because of very large image usage.
  • Try to find ways to reduce memory consumption

@servantftechnicolor servantftechnicolor marked this pull request as ready for review September 30, 2022 07:32
@@ -270,27 +270,62 @@ int aliceVision_main(int argc, char** argv)
// Preprocessing per view
for(std::size_t i = std::size_t(rangeStart); i < std::size_t(rangeStart + rangeSize); ++i)
{
const std::shared_ptr<sfmData::View> & viewIt = viewsOrderedByName[i];
const std::shared_ptr<sfmData::View> & viewIt = viewsOrderedByName[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Usage of tabs for new code is inconsistent with the rest of codebase.

@@ -423,7 +595,7 @@ int aliceVision_main(int argc, char** argv)
boxes.push_back(localBbox);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded trailing space.

std::unique_ptr<oiio::ImageOutput> panorama = oiio::ImageOutput::create(outputPanoramaPath);
oiio::ImageSpec spec_panorama(panoramaWidth, panoramaHeight, 4, typeColor);
spec_panorama.tile_width = tileSize;
spec_panorama.tile_height = tileSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tabs vs spaces here leading to apparently bad indentation.

offsetX += panoramaWidth;
}

int left = std::floor(double(offsetX) / double(tileSize));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since offsetX and tileSize are both int, a simple division can be done here offsetX / tileSize as it truncates to integer result automatically.

int left = std::floor(double(offsetX) / double(tileSize));
int top = std::floor(double(offsetY) / double(tileSize));
int right = std::ceil(double(offsetX + width - 1) / double(tileSize));
int bottom = std::ceil(double(offsetY + height - 1) / double(tileSize));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to keep doing division in integers. Such upwards rounded division can be implemented like this:

int divideRoundUp(int a, int b)
{
    return (a + b - 1) / b;
}

Which could be used here as:

int right = divideRoundUp(offsetX + width - 1, tileSize);

Copy link
Contributor

Choose a reason for hiding this comment

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

#1254 introduces such function and fixes several bugs in division code (unrelated to this PR).

@p12tic
Copy link
Contributor

p12tic commented Oct 1, 2022

This PR could be 2 or 3 PRs which would be easier to merge than one large PR.

E.g. Vector2d -> Vector2f changes could be a single PR. This introduces larger precision error, so having the change as a single PR would make it easier to understand if some precision issue in the future is caused by this change or by something else.

std::to_string(viewCurrent) -> warpedPath changes could probably be another PR (though I'm not completely sure for this one).

Third PR - everything what's left.

@servantftechnicolor servantftechnicolor force-pushed the dev/largePanoramaMemoryClean branch 2 times, most recently from b763784 to 2600a31 Compare December 8, 2022 09:45
@fabiencastan fabiencastan added this to the 3.0.0 milestone Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants