search

CesiumGS / cesium-omniverse

Bringing the 3D geospatial ecosystem to Omniverse
https://cesium.com/platform/cesium-for-omniverse/
Apache License 2.0
55 stars 7 forks source link

asset accessor that uses libcurl directly #634

Closed timoore closed 6 months ago

timoore commented 7 months ago

This PR replaces the HttpAccessAccessor with the accessor from vsgCs, called UrlAssetAccessor. UrlAssetAccessor caches the CURL handle between requests, allowing libcurl to keep connections open to remote servers. This can be a big performance gain, especially for https connections. Apparently this was always problematic in the cpr library used by HttpAccessAccessor.

There was also some cleanup in the rest of the code to refer to CesiumAsync:IAssetAccessor instead of a concrete accessor type where possible.

weegeekps commented 7 months ago

So it turns out CPR doesn't autoresolve proxies like I thought it did, so my comments about the proxy support are moot. Probably needs to be fixed one day.

lilleyse commented 7 months ago

I'm seeing SSL errors on Windows:

2024-01-26 18:20:32 [24,003ms] [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.916] [cesium-omniverse] [error] [RasterOverlayCollection.cpp:127] Error while creating tile provider: curl: SSL peer certificate or SSH remote key was not OK

2024-01-26 18:20:32 [24,005ms] [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.918] [cesium-omniverse] [error] Error while creating tile provider: curl: SSL peer certificate or SSH remote key was not OK

2024-01-26 18:20:32 [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.922] [cesium-omniverse] [error] [TilesetContentManager.cpp:825] An unexpected error occurred when loading tile: curl: SSL peer certificate or SSH remote key was not OK

timoore commented 7 months ago

I'm seeing SSL errors on Windows:

I've seen reports of this on Windows with vsgCs, but it has always worked for me. I think it's sensitive to the exact version of curl+ssl that is downloaded. I will check if cpr has some explicit dependency on SSL / TLS that isn't being satisfied now.

timoore commented 7 months ago

Sean, can you try it on Windows with the latest commit? We thought that the local certificate was redundant, but maybe there are additional hoops to jump through to use OS provided certificates.

I would try it myself, but I'm in the airport and don't have any Windows environment installed.

lilleyse commented 7 months ago

The latest commit fixes it.

It would still be good to know why it's needed for Windows. I was hoping we could get away without shipping certs.

timoore commented 7 months ago

https://stackoverflow.com/questions/37551409/configure-curl-to-use-default-system-cert-store-on-windows looks promising.

weegeekps commented 7 months ago

Starting with curl 8.2.0 (July 19 2023) (issue), there is the option --ca-native

I wonder how that option translates from CLI to libcurl?

timoore commented 7 months ago

Another try, this time with the CURLSSOPT_NATIVE_CA option. @lilleyse can you give the latest a quick try?

lilleyse commented 7 months ago

@timoore looks like there's a merge conflict now that #626 is merged. I think the strategy would be to take the changes from main and re-do the smaller changes that you made in Context, etc.

Also, could you remove HttpAssetAccessor and anything that mentions cacert.pem including the update_certs script? We don't need to install the requests library any more either. That can be removed from the developer setup and main.yml.

r-veenstra commented 7 months ago

I did some load time testing on this branch and observed some very positive results compared to current main, and also the version of main prior to this branch fork (to ensure the massive refactor didn't impact anything)

Process:

Current main: f89fa4e4a33a2e68d6842fe46adf70065b0d06a2 Test 1: 92.85 Test 2: 85.75 Test 3: 87.22 Test 4: 88.51 Test 5: 86.78 Average: 88.222

This Branch: e602ddd5215330204d13f211bec74648555d7b3a Test 1: 52.79 Test 2: 51.25 Test 3: 52.5 Test 4: 51.07 Test 5: 51.7 Average: 51.862

Main pre-libcurl updates: d1b7ebcbdcd0020fca64eb21cc8a19ed3abe6a9d Test 1: 89.67 Test 2: 86.9 Test 3: 88.7 Test 4: 86.89 Test 5: 86.52 Average: 87.736

Unless I've made a mistake, that's a 35% load time reduction for a complex Google 3D tiles scene.

lilleyse commented 6 months ago

@timoore could you update CHANGES.md summarizing the performance improvements that Ryan noted: https://github.com/CesiumGS/cesium-omniverse/pull/634#issuecomment-1916288094