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 Plane2D step after resetDimensions #58

Merged
merged 2 commits into from
Apr 13, 2019
Merged

Conversation

yann-lty
Copy link
Member

@yann-lty yann-lty commented Mar 26, 2019

Plane2D's step is directly related to effective width and needs to be updated when dimension changes.
This fixes an issue when mixing images with different resolutions that can result in wrong buffer accesses, as shown here:
popsift_plane2D
Re-using a previously allocated buffer (first figure) from the pool to work on another image with different width/height leads to erroneous results (second figure); updating the step fix this (third image).
Probably related to #56.

@yann-lty yann-lty changed the title Update Plane2D step after resetDimensions [WIP] Update Plane2D step after resetDimensions Mar 27, 2019
@yann-lty
Copy link
Member Author

yann-lty commented Mar 27, 2019

This can not be merged as is; we need to know the real buffer size in Plane2D to be able to check if resetDimensions to something larger than current useful size can be done.
At the moment, the sanity check if( w*sizeof(T) > this->getPitch() ) { is broken (getPitch returns step which is now based on the useful width of Plane2D and not its full capacity).

@griwodz
Copy link
Member

griwodz commented Apr 2, 2019

If you change step in Plane2D, you must do it in compliance with Texture alignment rules. Since this is a card-specific value, you are not supposed to guess. So "this->step = w * this->elemSize();" is dangerous just because of that.

If you dare doing this, you should round up to the next multiple of 256 (there are cards that need 32, most need 128, but a few new 256).

@griwodz
Copy link
Member

griwodz commented Apr 2, 2019

The new commit adds a size_t _total_bytes to PlaneBase to hold the allocated memory. That is somewhat hackish because PlaneBase objects are instantiated on the GPU side, but this _total_bytes will only be initialized correctly on the CPU side.
The resetDimension test in s_image has been changed to ->canResetDimensions(), which uses a conservative heuristic for pitch estimation. This is not fully safe, but NVidia has published neither documentation nor API for safe pitch computation.

@yann-lty
Copy link
Member Author

yann-lty commented Apr 5, 2019

I've made some tests but this does not to solve the issue of buffer access when a Plane2D was "resized".
image
I think that at some point (still have to find where), we are using step instead of width * elemSize() and it leads to incorrect index computation/buffer access. This is probably why my (hackish) first commit seemed to fix the problem.

Host (CPU) Plane2D references contiguous memory and doesn't have pitch. Its step must be updated when it changes dimensions.
* split resetDimensions into resetDimensionsHost and resetDimensionsDev
  * resetDimensionsHost: reference contiguous memory => update Plane2D's step to width * elemSize()
  * resetDimensionsDev: reference pitched memory => let the step untouched
Copy link
Member

@fabiencastan fabiencastan left a comment

Choose a reason for hiding this comment

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

LGTM

@yann-lty
Copy link
Member Author

yann-lty commented Apr 9, 2019

The problem was not about updating the step of device Plane2Ds, but host ones.
We use plain memcpy to load the image on the CPU. If host Plane2D's step is not updated, the memcpyToDevice, which relies on it, will not copy the buffer to the device properly.

  • Host: contiguous memory without pitch, must always remains width * elemSize() => step should be updated by resetDimensions
  • Device: pitched GPU memory, step must be set at allocation and never be modified (at least until re-allocation) => step should NOT be updated by resetDimensions

Introducing resetDimensionsDev and resetDimensionsHost to handle this.
Also, made "step" a protected member and renamed it to _pitchInBytes to simplify naming (I can rollback this if it's not wanted).

@fabiencastan fabiencastan changed the title [WIP] Update Plane2D step after resetDimensions Update Plane2D step after resetDimensions Apr 9, 2019
@fabiencastan
Copy link
Member

This looks fine to me now.
@griwodz Can you review and merge?

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

Successfully merging this pull request may close these issues.

4 participants