microsoft / MRLightingTools-Unity

A Unity library and MRTK extension for estimating and replicating the current environment's lighting on Mixed Reality devices.
MIT License
188 stars 25 forks source link

HoloLens 2 and Task Support #9

Closed jbienzms closed 4 years ago

jbienzms commented 4 years ago

This is a pretty big PR so I want to fully document the reason for each of the changes.

Main Work

I started this work because I could not get MRLT to work on HoloLens 2 without errors in the Unity log and on screen. The most prominent errors were:

HoloLens locatable camera has failed to set auto exposure off
HoloLens locatable camera has failed to set exposure to ## as requested
HoloLens locatable camera has failed to set auto WhiteBalance off
HoloLens locatable camera has failed to set WhiteBalance to #### as requested
[CameraCapture] Can't get camera matrix!!

It turned out that these errors are primarily due to HoloLens 2 wanting to use ExposureControl rather than Exposure and wanting to use WhiteBalanceControl rather than WhiteBalance.

My first attempts at fixing this looked promising, but seemed to only work randomly. Now I started seeing new errors in the log:

HoloLens locatable camera has failed to set exposure to 00:00:00.0101000. System.NullReferenceException: Object reference not set to an instance of an object.
HoloLens locatable camera has failed to set ISO to 80. System.ArgumentException: No capture devices are available.

These ended up stemming from two different core issues:

  1. In HoloLens 2, the Control interfaces can only be adjusted while the camera is streaming.
  2. It turned out there were several possible race conditions when setting these values (due to callbacks) and when shutting down or starting LightCapture back up.

Only adjusting the control values while streaming was a fairly easy fix. Apply them only if the camera is running, otherwise cache the values for the next time the camera is started.

The race conditions and callbacks ended up being responsible for the bulk of the refactoring. The root of the problem is that methods like Initialize, SetExposure and SetWhiteBalance all returned void, but internally they ran a bunch of callbacks. This made those methods function like async void. Now, I'm not going to say that async void should never be used, but it tends to be the source of many headaches, race conditions and threading bugs. This is because the caller has no way to know when the function has actually completed. For more information see Avoid Async Void.

It's also worth pointing out that any method that returns a Task can be treated as void, just not the other way around.

The following line:

var t = SetWhiteBalanceAsync(5000);

Is functionally equivalent to:

SetWhiteBalance(5000, myCallback);

If Task t is never awaited, it will run in the background and we'll never know whether it failed or completed. This would be equivalent to myCallback never getting called but the application being unaware it didn't happen.

So, every method in the library that previously used a callback was converted to return a Task. And every place that called one of those methods was also converted to a Task (whenever possible). There were a few cases where this wasn't possible. For example, when responding to a UI button click. In these "last mile" cases async void was used, but error handlers were employed.

Other Fixes and Changes

Those are the main things that were refactored. In addition there were also other fixes and improvements which I'll cover now:

maluoi commented 4 years ago

This is awesome work, and fantastic documentation! Thanks so much for doing this :)

I'll try and put in some time to check corners like AR Foundation, and put this all into a release.