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

Segfault when rendering large number of spheres with color #207

Closed
stukowski opened this Issue Jan 4, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@stukowski

stukowski commented Jan 4, 2018

OSPRay crashes in the Spheres_postIntersect function when rendering a spheres geometry containing a large number of spheres (e.g. >=144M) with per-sphere colors.

I attached a simple test program that reproduces the segfault. It should be run on computer with at least 24 gigs of memory.
sphere_test.tar.gz

The segfault does not occur for smaller numbers of spheres (e.g. <120M) or if the color field of the spheres geometry is not set at all. Most likely it occurs only if the size of the color memory buffer exceeds 2^31 bytes. The origin of the problem seems to lie here:

colorID = ray.primID;
uint32 colorAddr = self->color_offset+colorID*self->color_stride;
dg.color = *((vec4f *)(self->color+colorAddr));
if (!self->has_alpha)
dg.color.w = 1.f;

I am wondering whether this is a bug or an expected limitation of OSPRay.

@carsonbrownlee carsonbrownlee self-assigned this Jan 4, 2018

@carsonbrownlee

This comment has been minimized.

Show comment
Hide comment
@carsonbrownlee

carsonbrownlee Jan 4, 2018

Contributor

I see little reason we can't compute that as an int64 as we aren't storing it. Looking into it.

Contributor

carsonbrownlee commented Jan 4, 2018

I see little reason we can't compute that as an int64 as we aren't storing it. Looking into it.

@carsonbrownlee

This comment has been minimized.

Show comment
Hide comment
@carsonbrownlee

carsonbrownlee Jan 4, 2018

Contributor

Thanks for the detailed bug report. fix pushed to release-1.4.x branch. Please let me know if this fixes it for you.

Contributor

carsonbrownlee commented Jan 4, 2018

Thanks for the detailed bug report. fix pushed to release-1.4.x branch. Please let me know if this fixes it for you.

@jeffamstutz

This comment has been minimized.

Show comment
Hide comment
@jeffamstutz

jeffamstutz Jan 4, 2018

Contributor

FYI, it's only pushed to our internal Gitlab repo...once our CI passes it'll get pushed here to Github. I'll ping when it's up.

Contributor

jeffamstutz commented Jan 4, 2018

FYI, it's only pushed to our internal Gitlab repo...once our CI passes it'll get pushed here to Github. I'll ping when it's up.

@stukowski

This comment has been minimized.

Show comment
Hide comment
@stukowski

stukowski Jan 11, 2018

I've tested the fix. Both the test case and my application seem to work now without problems. Thank you.

This is off-topic now: I just would like to point you at some ospray extension modules that I wrote for my application. They add rendering support for cones, discs and quadric (or ellipsoid) geometries to ospray. Maybe you find this functionality to be of general interest and want to integrate something like this into upstream ospray. In this case, please feel free to make use my source code.

https://gitlab.com/stuko/ovito/tree/master/src/plugins/ospray/ospray_module/geometry

image

stukowski commented Jan 11, 2018

I've tested the fix. Both the test case and my application seem to work now without problems. Thank you.

This is off-topic now: I just would like to point you at some ospray extension modules that I wrote for my application. They add rendering support for cones, discs and quadric (or ellipsoid) geometries to ospray. Maybe you find this functionality to be of general interest and want to integrate something like this into upstream ospray. In this case, please feel free to make use my source code.

https://gitlab.com/stuko/ovito/tree/master/src/plugins/ospray/ospray_module/geometry

image

@jeffamstutz

This comment has been minimized.

Show comment
Hide comment
@jeffamstutz

jeffamstutz Jan 14, 2018

Contributor

Fix should be in the latest v1.4.3 release.

Thanks, we'll have a look! Do you by chance have any data sets in which we could demonstrate the new geometry types? The idea is that we would want to have some sort of geometry file format that we could load to demonstrate the geometry in our sample viewer (ideally hosted on the ospray.org website, too). Definitely not required for upstreaming, but would be nice if it's not too difficult.

Contributor

jeffamstutz commented Jan 14, 2018

Fix should be in the latest v1.4.3 release.

Thanks, we'll have a look! Do you by chance have any data sets in which we could demonstrate the new geometry types? The idea is that we would want to have some sort of geometry file format that we could load to demonstrate the geometry in our sample viewer (ideally hosted on the ospray.org website, too). Definitely not required for upstreaming, but would be nice if it's not too difficult.

@jeffamstutz

This comment has been minimized.

Show comment
Hide comment
@jeffamstutz

jeffamstutz Jan 26, 2018

Contributor

Unfortunately the fix used for this only works with AVX2+ ISA targets. Reopening this for awareness of a more robust solution.

Contributor

jeffamstutz commented Jan 26, 2018

Unfortunately the fix used for this only works with AVX2+ ISA targets. Reopening this for awareness of a more robust solution.

@Twinklebear

This comment has been minimized.

Show comment
Hide comment
@Twinklebear

Twinklebear Apr 25, 2018

Contributor

This change also affects the way color_offset and color_stride are interpreted, breaking the per-sphere colors in MegaMol. Instead of being treated as before as byte offsets, they're now treated as "# of floats" offsets in the array resulting in the wrong data being read for the color information.

I'm looking at adding some safe gather methods which take a byte stride instead of a component count stride to fix this. From my understanding the MegaMol method actually puts all the data in the same ospData and then uses the color_offset and color_stride to have the per-sphere colors look up properly in this single big array. For example their Sphere struct could be something like:

struct Sphere
    vec3f origin;
    vec4f color;
};

Then in the array it's something like:

Sphere spheres[] = {
    Sphere(vec3f(...), vec4f(...)),
    Sphere(vec3f(...), vec4f(...))
};
Contributor

Twinklebear commented Apr 25, 2018

This change also affects the way color_offset and color_stride are interpreted, breaking the per-sphere colors in MegaMol. Instead of being treated as before as byte offsets, they're now treated as "# of floats" offsets in the array resulting in the wrong data being read for the color information.

I'm looking at adding some safe gather methods which take a byte stride instead of a component count stride to fix this. From my understanding the MegaMol method actually puts all the data in the same ospData and then uses the color_offset and color_stride to have the per-sphere colors look up properly in this single big array. For example their Sphere struct could be something like:

struct Sphere
    vec3f origin;
    vec4f color;
};

Then in the array it's something like:

Sphere spheres[] = {
    Sphere(vec3f(...), vec4f(...)),
    Sphere(vec3f(...), vec4f(...))
};
@gralkapk

This comment has been minimized.

Show comment
Hide comment
@gralkapk

gralkapk Apr 25, 2018

We commit the data as a single interleaved array of "XYZRGB", all in float.
Ideally, we would like to commit an interleaved array where "RGB" would be chars.

gralkapk commented Apr 25, 2018

We commit the data as a single interleaved array of "XYZRGB", all in float.
Ideally, we would like to commit an interleaved array where "RGB" would be chars.

@Twinklebear

This comment has been minimized.

Show comment
Hide comment
@Twinklebear

Twinklebear May 16, 2018

Contributor

@gralkapk this is now out in 1.6, let us know how it's working

Contributor

Twinklebear commented May 16, 2018

@gralkapk this is now out in 1.6, let us know how it's working

@jeffamstutz jeffamstutz closed this Jun 6, 2018

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