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

Noise(wrong intersection result) in curve test #17

Closed
syoyo opened this issue Jan 11, 2020 · 10 comments
Closed

Noise(wrong intersection result) in curve test #17

syoyo opened this issue Jan 11, 2020 · 10 comments
Labels
bug Something isn't working

Comments

@syoyo
Copy link
Contributor

syoyo commented Jan 11, 2020

At least it happens from v3.5.0

aarch64 result of curve test contains some noise, probably due to a NEON./fp math issue.
render
(x86_64 v3.7.0)

v3 5 0-aarch64
(aarch64 v3.5.0)

v3 6 1-aarch64
(aarch64 master(v3.6.1 + iOS patch))

@syoyo syoyo added the bug Something isn't working label Jan 11, 2020
@syoyo syoyo changed the title Noise in curve test Noise(wrong intersection result) in curve test Jan 11, 2020
@syoyo
Copy link
Contributor Author

syoyo commented Jan 22, 2020

neon-fix seems solved the issue https://github.com/lighttransport/embree-aarch64/tree/neon-fix

It looks the reason was some incorrect SIMD operation + missing Newton-Raphson refinement for some div, sqrt, rsrt, etc.

BUILD_IOS code path improves/fixes some NEON SIMD.

So neon-fix branch backports BUILD_IOS code path(iOS specific) to aarch64(Linux, Android) build by removing iOS clang dependency, primarily by explicitly adding vreintepret conversion-free casting.

@maikschulze
Copy link
Contributor

Hi Syoyo,
I can confirm the quality regression that you have noticed.

On Android arm64 many features become broken between July 2019 (left) and January2020 (right):

image

I've attached all image results for manual inspection:
comparison_github.zip

I also agree with your reasoning about the cause in the NEON SIMD code. I recall that I was able to fix these issues in the past by introducing two refinement iterations for Newton-Raphson.

@syoyo
Copy link
Contributor Author

syoyo commented Feb 29, 2020

@maikschulze Thanks for the comparison!

Please use neon-fix branch. I have successfully suppressed some noise in that branch by correct Newton-Raphson iteration. If things goes well, I will merge neon-fix branch to master soon.

Also, it would be nice if you contribute regression tests(i.e. build test scene and run it in batch manner) to reproduce comparison images.

@maikschulze
Copy link
Contributor

maikschulze commented Feb 29, 2020

Thanks for pointing me to the neon-fix branch, @syoyo .

I've run my tests with this branch and compare it to the state of master in July 2019:
comparison_github_2.zip

Identical: DynamicScene, GridGeometry, InstancedGeometry, LazyGeometry, PointGeometry, PointPrecision, UserGeometry

Different: CurveGeometry, DisplacementGeometry, HairGeometry, Interpolation, IntersectionFilter, MotionBlur, SubdivisionGeometry, TriangleGeometry

I've taken a look at the images and their differences in BeyondCompare. I consider all but one perceptually identical. I'm not able to state which one is "better".

Only one result shows a structural difference to me: Interpolation_003.png

Here, it seems the shadow test is slightly less precise in neon-fix (right) vs the older state of master (left):
image

@maikschulze
Copy link
Contributor

In terms of tests, I would gladly help out, or possibly contribute my setup after major cleanups. We should settle on the architecture of the tests first. I can briefly describe what I did:

I've created a second, private repository "embreetest" which contains a lot of the original tutorial code that has been refactored to create well-controlled image series and take some additional measurements. In addition, I'm using a different math library (GLM) to minimize the influence of Embree code changes onto my test case. This works very well, because Embree's API is so stable.

The down-side is the "duplication" of the tutorial code. I was trying to wrap the existing tutorial codebase, but gave up eventually and consider it more important to have the tests in a different repository. I avoided changing the original tutorial code to avoid code conflicts and minimize regression risk. Having the tests in a separate repo allows the easy execution of newly added tests retrospectively on older builds, which came handy already for me when I was tracking down numerical imprecisions. Naturally, this test repository depends on Embree. The other dependencies (such as GLM, lodepng) could be included.

The tests are compiled into a dynamic library with a simple C++ interface for a result monitor. This could be changed to a C interface based on callbacks for easier composition.

On top of that sits a command-line executable that implements a result monitor and writes text log files and png files. So far, I've manually inspected the results. I'm unfamiliar with automatic setups on Github. However automation would probably be best. This approach would also allow the integration of the test suite into GUI apps done by thirds.

What's your take on this? What else should be considered?

@maikschulze
Copy link
Contributor

I've briefly compared the image results with x64 as well and realize "my structural artifact" also appears when rendering on x64 SSE2. There's small differences between neonfix @ arm64 and the x64 SSE2 state in many images, similar to the above results comparing the two states on arm64. Nothing stands out.

Left: neonfix on arm64
Right: master July2019 on x64 SSE2
image

I would therefore argue that neonfix is in a good state and you've fixed the imprecisions. Thank you!

@syoyo
Copy link
Contributor Author

syoyo commented Mar 1, 2020

I've briefly compared the image results with x64 as well and realize "my structural artifact" also appears when rendering on x64 SSE2.

Does this artifact appear in original embree build(v3.8.0)? If so, it would be better to report this issue to original embree git repo.

@maikschulze
Copy link
Contributor

The artifact appears in the official 3.8.0 as well. I've modified the provided interpolation tutorial to match my camera setup:
camera.from = Vec3fa(0.0f, 5.0f, -5.0f);
camera.to = Vec3fa(0.0f, 0.0f, 0.0f);
and tested the ISAs with and without RTC_SCENE_FLAG_ROBUST.

The silhouette patterns vary slightly. I've come to realize that this kind of pattern only happens for even-sized image resolutions, not for odd image resolutions. If the camera rays were cast through the center of each pixel, I would expect the opposite. However, I can't find any evidence in the code that a half pixel offset to the center is performed.

This is the result of Embree 3.8.0 on x64 with "--isa avx2"

interpolation_003_diff

I don't think there's anything wrong with Embree. However, it might be worth verifying that a half pixel offset should be added throughout the tutorials to ensure there's no bias to a pixel corner. Visually, this is best done by rendering very low resolutions.
On the other hand, holes can nicely be detected when raycasting exactly along the axes. So both odd and even configurations might be worth testing in general.

@syoyo
Copy link
Contributor Author

syoyo commented Mar 1, 2020

On top of that sits a command-line executable that implements a result monitor and writes text log files and png files. So far, I've manually inspected the results. I'm unfamiliar with automatic setups on Github. However automation would probably be best. This approach would also allow the integration of the test suite into GUI apps done by thirds.

What's your take on this? What else should be considered?

Usually we use the following approach for testing our own renderer.

  • Run scenes or test rendering in unit tester(e.g. goole test, acutest) or python script.
  • Run image comparator(e.g. pdiff(perceptual diff), idiff from OpenImageIO or our own custom image comparator) for rendered images
  • Report result as HTML

It would be better to manage test codes and scenes in different git repo, so we can setup embree-aarch64-test(or similar) git repo. We may also be possible to contribute HTML reporter.

I can setup CI automation of running tests using Github Actions or Travis. Github Actions support aarch64 platform(through qemu emulation, which is slow to execute though)

@syoyo
Copy link
Contributor Author

syoyo commented Mar 8, 2020

Fixed in this commit:
4433618

But we still see some inconsistency in verify watertight test #23 and displacement test. Will investigate it further and open another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants