Hello, @SFN-eu .

Thanks for this huge PR !

Would you mind testing on https://github.com/alicevision/dataset_monstree (full) and put the output of the depthmap node somewhere ?

Thanks !

So apparently the monstertree dataset causes it to intermittently segfault on some (but not all) of the stages of depthMapEstimation, the joys of testing and it-works-on-my-{machine,dataset}. Will get back once I've fixed the issue.

So, the segfaults only happened a couple of times with the meshroom GUI , then disappeared. I have since been unable to replicate them, even with valgrind, so I'm tentatively putting them down to using development versions of both AliceVision and Meshroom. If anyone has any similar issues, please report.

In the meantime, here is the result of running the default "photogrammetry" pipeline on monstertree/full:

Here is the raw output of the DepthMap and DepthMapFilter stages, as generated by meshroom:
https://github.com/SFN-eu/tmp

There is a huge difference between the cuda result and your. The most visible is the depthmapfilter. The output is almost sparse in your output ?

Oops, missed that, apologies. Having had a closer look (and now knowing what to look for) there are two issues:

A brightness difference in DepthMap (CUDA on right)

Some glitches in DepthMapFiltered (CUDA still on right)

I'll fix these and see if I can get CI to work.

Digging into the problem more. Interestingly the SYCL implementation (top) reports a better sim map, despite performing worse:

Also, apparently device accelerated code is only ran in DepthMapFilter for normal map estimation. The exact same CPU code (src/aliceVision/fuseCut/Fuser.cpp) is being run to filter both depth maps, but the inaccuracies compound and end up getting rejected by the filter (as they should be, if the filter is doing it's job).

Hi! Cool work! Happy to see that another backend is in the works for this project. And once AdaptiveCpp has sufficient Apple Metal support, I could drop my Metal implementation whatsoever. Do you have an approximate timeline on when Metal can be used with AdaptiveCpp?

I would then consider dropping my upstreaming plans for MTL-AliceVision if the same could be achieved with a unified backend :).

Main questions:

• To avoid removing the existing implementation, the new one lives under src/aliceVision/depthMap_sycl/. Should this be moved?

And if I may chip in on this: If one asked me, I'd support a unified (and backend agnostic) DepthMap library, at least as long as multiple backends need to coexist. Because if they were exclusive to one another, a system with mixed GPU vendors could not utilize all hardware. I had started to think about how one could design that and came up with a concept similar to the ComputeOnMultiGPUs approach: something like a work-orchestrator pattern. The orchestrator class provides a unified API and internally handles dispatching to different backends (and potentially multiple devices). That would also allow for a more fine-grained device/vendor selection, if required. That would leave us with a few API classes (like DepthMapEstimatorOrchestrator, RefineOrchestrator, SgmOrchestrator), abstracting all backend implementation details away.

Hi! Cool work! Happy to see that another backend is in the works for this project. And once AdaptiveCpp has sufficient Apple Metal support, I could drop my Metal implementation whatsoever. Do you have an approximate timeline on when Metal can be used with AdaptiveCpp?

It already works on Apple CPU, Metal support is slowly being patched in at AdaptiveCpp/AdaptiveCpp#864; see also https://adaptivecpp.github.io/AdaptiveCpp/install-metal/#enabling-the-metal-backend. Tl;dr it's an experimental option that's still rather barebones and misses features that this port relies on (namely, USM pointers and the double type). You would probably have to ask them for a more detailed timeline, if it even exists.

And if I may chip in on this: If one asked me, I'd support a unified (and backend agnostic) DepthMap library, at least as long as multiple backends need to coexist. Because if they were exclusive to one another, a system with mixed GPU vendors could not utilize all hardware. I had started to think about how one could design that and came up with a concept similar to the ComputeOnMultiGPUs approach: something like a work-orchestrator pattern.

This is literally how the AdaptiveCPP backend works. The new DepthMap library is unified and, in some ways, "backend agnostic", because acpp abstracts everything away and just presents to the end programmer a list of all available devices on the system (and does in fact use OMP, CUDA, HIP, Intel ZE ecc. under the hood, handling synchronization and memcpy's between them at runtime). It also has an experimental multi-device queue (https://adaptivecpp.github.io/AdaptiveCpp/multi-device-queue/) for automatic work distribution, but currently "This extension should not yet be used for any production workloads" (testing how well it performs is something I want to do as part of the performance tuning, but correctness comes first).

P.s. does anyone know how to stop GitHub from spamming the full list of existing commits every time I rebase onto the tip of the develop branch? It's getting a bit old...

Understood, thanks. We'll see if the double problem will get resolved. Metal itself has no native support for 64-bit floating-point types, that is something I encountered in my port and just switched to floats.
And by unified library I meant the original CUDA library and the SYCL port. I think it'll be interesting to merge these into a single library with a unified interface - as long as the old CUDA code remains in the project. Of course that becomes irrelevant if the SYCL part is intended to replace the existing CUDA lib altogether.

I mean, it would be possible if the maintainers wished for it, but in other benchmarks AdaptiveCPP has shown itself to be better (i.e. faster) at running CUDA applications than CUDA itself (for those reading who are so inclined, you can check out the full paper: https://dl.acm.org/doi/full/10.1145/3731125.3731127).

So basically I don't see the point of using the CUDA implementation if you want to use the SYCL one ... which can also use CUDA. I kept it around because it's far better tested, and coming in with a PR that deletes an entire library didn't seem particularly helpful.

Narrowed down the issue to the initial similarity volume computation: SYCL (first image) implementation is in fact rather more sparse:

Ok, the problem is (at least in part) with my implementation of mipmap images (SYCL top), they are not getting filtered properly:

(apologies for the perhaps slow progress, I have been going through the code with a fine toothed comb and have found a whole host of other minor glitches, but this precise issue is sill escaping me ... 🙄)

Could you change the build system, to ensure that we can build depthMap (in cuda) and depthMap_sycl at the same time?
We will need to keep both implementations for a while to fully validate in production that there is no regression and expose an option to choose between one implementation and another.

Should now be done with #6881103! The option in question is "--backend", valid options are 0 (CUDA) and 1 (SYCL) depending on what backends where enabled with the ALICEVISION_DEPTHMAP_BACKEND compile option.