martin-marek / hdr-plus-swift

📸Night mode on any camera. Based on HDR+.
https://burst.photo
GNU General Public License v3.0
198 stars 9 forks source link

Merging with 32bit precision, reduced memory usage and improved performance #38

Closed chris-rank closed 9 months ago

chris-rank commented 9 months ago

Closes #36.

Main changes that led to reduction of system memory usage and cumulative memory allocation:

Some tests performed on a MacBook Air M1 with 7 GPU cores and 16 GB memory using the frequency merging:

Example burst Parameter Burst Photo V1.5 Current PR
1) Uniform exposure: 48 x 24.3 MPix peak system memory usage 7.3 GB 4.0 GB
cumulative memory allocation 55.8 GB 7.1 GB
processing time: align+merge 24.3 s 21.4 s
2) Bracketed exposure: 7 x 20.3 MPix peak system memory usage 3.0 GB 0.6 GB
cumulative memory allocation 11.7 GB 1.0 GB
processing time: align+merge 4.24 s 3.23 s
martin-marek commented 9 months ago

Wow, this is great Chris! Do you want to make this a v1.6 release?

chris-rank commented 9 months ago

Yes, this could be a version 1.6 release. But we should align with Alex first as he has also an open PR. While I already tested the code a lot, some review or testing of someone else would be great.

Alex-Vasile commented 9 months ago

Yes, this could be a version 1.6 release. But we should align with Alex first as he has also an open PR. While I already tested the code a lot, some review or testing of someone else would be great.

I don't really care about merging order or release number.

I would like to review this. I particularly want to see how this relates to #32. I imagine the cumulative memory allocation difference is from reusing textures.

Alex-Vasile commented 9 months ago

Re: dealing with the black level. I was thinking about this, but how come we don't subtract the black level once as part of reading the DNG from file into the first metal texture. Beyond that we shouldn't need it anymore.

chris-rank commented 9 months ago

I had a look into the description in Apple's documentation and did not find a way to explicitly trigger lossless texture compression. It might not even be supported properly for the macOS version 11 we currently build the app for. As the peak memory usage is much lower now, I thought that it may not be needed anymore. My interpretation of your issue was that some evaluation of that feature shall be done.

Alex-Vasile commented 9 months ago

I had a look into the description in Apple's documentation and did not find a way to explicitly trigger lossless texture compression. It might not even be supported properly for the macOS version 11 we currently build the app for. As the peak memory usage is much lower now, I thought that it may not be needed anymore. My interpretation of your issue was that some evaluation of that feature shall be done.

The peak is much better now, but the peak will still slowly increase until the limit set for the cache. The compression was for trying to increase the amount of photos that could be cached without increase the memory usage.

chris-rank commented 9 months ago

If you prefer, I can remove the #32 from this PR. You are right, a compression would enable more frames on the cache.

Alex-Vasile commented 9 months ago

If you prefer, I can remove the #32 from this PR. You are right, a compression would enable more frames on the cache.

I think you should. We can close that issue separately if we don't find a solution, but I don't believe that this PR (good as it is) closes it.

Alex-Vasile commented 9 months ago

Overall, could you please provide more information about how this PR reduces peak and cumulative memory usage?