C# Record and Playback APIs #822

Brent-A · 2019-10-08T22:01:06Z

Fixes #573

Description of the changes:

Added wrapper for Record and Playback APIs
Added a unit test to loop back a synthetic recording
Added native debug message hook to Record and Playback

Before submitting a Pull Request:

I reviewed CONTRIBUTING.md
I built my changes locally
I ran the unit tests
I ran the functional tests with a device
I ran the performance tests with a device

I tested changes on:

Windows
Linux

…ensor-SDK into csharp-record

wes-b

Wow that is a lot of exception handling. There is a ton of code here. You might also have Derek take a peek

DerekMa-WP · 2019-10-11T22:47:11Z

src/csharp/K4a.sln

 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
+		Debug|Any CPU = Debug|Any CPU


Not sure the best way to deal with this. The "Any CPU" in the solution can get confusing, but Visual Studio really likes adding it back in.

I'm not sure here either. Not supporting Any CPU in the nuget package is also pretty rough since downstream projects tend to have it by default.

I thought we did have an issue tracking Any CPU support but I can't find it at this moment.

.NET Core can actually do AnyCPU here. It's able to handle multiple native libraries with different architectures by deploying the native libs in the \runtimes\win-x64\ and \runtimes\win-x86\ sub folders.

DerekMa-WP · 2019-10-11T22:49:04Z

src/csharp/Examples/Recording/Program.cs

@@ -0,0 +1,102 @@
+using System;


Copyright header.

src/csharp/Record/Exceptions/AzureKinectAddAttachmentException.cs

src/csharp/Record/Exceptions/AzureKinectCreateRecordingException.cs

…on.cs Co-Authored-By: Derek M. <[email protected]>

DerekMa-WP · 2019-10-14T21:44:07Z

src/csharp/Record/Playback.cs

+        }
+
+        /// <summary>
+        /// Gets get the camera calibration for Azure Kinect device used during recording. The output struct is used as input to all transformation functions.


There are many places in the file where you have "Gets get", you can remove the duplicate "get".

DerekMa-WP · 2019-10-14T21:49:59Z

src/csharp/Record/Playback.cs

+                UIntPtr size = new UIntPtr(0);
+                if (NativeMethods.k4a_playback_get_raw_calibration(this.handle, null, ref size) != NativeMethods.k4a_buffer_result_t.K4A_BUFFER_RESULT_TOO_SMALL)
+                {
+                    throw new AzureKinectGetRawCalibrationException($"Unexpected result calling {nameof(NativeMethods.k4a_playback_get_raw_calibration)}");


If I understand the code correctly, this will throw if there is no calibration, however the exception will just say unexpected result. Can we add logging in here? Or somehow return a better result for when the calibration doesn't exist?

DerekMa-WP · 2019-10-14T22:03:45Z

src/csharp/Record/Playback.cs

+                    case NativeMethods.k4a_stream_result_t.K4A_STREAM_RESULT_EOF:
+                        return null;
+                    case NativeMethods.k4a_stream_result_t.K4A_STREAM_RESULT_FAILED:
+                        throw new AzureKinectGetCaptureException();


It might be nice to get the log here for this expcetion.

And the other similar functions, GetPreviousCapture, GetNextImuSample, GetPreviousImuSample, etc

DerekMa-WP · 2019-10-14T22:26:38Z

src/csharp/SDK/Device.cs

+        /// Azure Kinect objects.
+        ///
+        /// When using this handle value, the caller is responsible for ensuring that the
+        /// Device object does not become disposed.</remarks>


Would you want to add that locking on this object will gaurentee that it isn't disposed while you are using the handle?

DerekMa-WP · 2019-10-14T22:27:08Z

src/csharp/SDK/Device.cs

+        /// <returns>The native handle that is wrapped by this device.</returns>
+        /// <remarks>The function is dangerous because there is no guarantee that the
+        /// handle will not be disposed once it is retrieved. This should only be called
+        /// by code that can ensure that the Capture object will not be disposed on another


Suggested change

/// by code that can ensure that the Capture object will not be disposed on another

/// by code that can ensure that the Device object will not be disposed on another

DerekMa-WP · 2019-10-14T22:29:46Z

src/csharp/SDK/Logger.cs

@@ -47,6 +48,19 @@ public static class Logger
            }
        }

+        private static event Action<LogMessage> LogMessageHandlers;


Did StyleCop not require this to be before the public one?

DerekMa-WP · 2019-10-14T22:30:19Z

src/csharp/Record/RecordLogger.cs

+            }
+        }
+
+        private static event Action<LogMessage> LogMessageHandlers;


Did StyleCop not require this to be before the Public one?

DerekMa-WP · 2019-10-14T22:43:42Z

src/record/sdk/record.cpp

@@ -373,7 +380,10 @@ k4a_result_t k4a_record_add_tag(const k4a_record_t recording_handle, const char
        return K4A_RESULT_FAILED;
    }

-    add_tag(context, name, value);
+    if (NULL == add_tag(context, name, value))


Out of scope for this review, but do we need to care about the return value from any of the other add_tag calls?

DerekMa-WP · 2019-10-14T22:44:11Z

src/csharp/Tests/Record.UnitTests/LoopbackTests.cs

@@ -0,0 +1,216 @@
+using System;


Copyright header.

DerekMa-WP · 2019-10-14T22:47:49Z

src/csharp/Tests/Record.UnitTests/LoopbackTests.cs

+                record.AddTag("MYTAG1", "one");
+                record.AddTag("MYTAG2", "two");
+
+                record.WriteHeader();


You have a comment that WriteHeader must be called before any data is written, but does it need to be called after all tracks and tags are added?

DerekMa-WP · 2019-10-14T22:51:43Z

src/csharp/Tests/Record.UnitTests/LoopbackTests.cs

+
+                        // Not captured in recording
+                        // Assert.AreEqual(100, c.Color.ISOSpeed);
+                        Assert.AreEqual(0, c.Color.SystemTimestampNsec);


Why is this 0? Didn't you write the timestamp in above?

natelowry · 2020-02-19T00:53:31Z

any updates on this? would be greatly appreciated 😺

sagarpatel · 2020-03-06T08:04:43Z

really hoping this makes it in the v.1.4.0 release, waiting on this to develop some new features in my Unity3D project

UnaNancyOwen · 2020-03-27T09:16:49Z

What is the reason this pull request is stopped? Thanks,

natelowry · 2020-03-27T13:03:18Z

@UnaNancyOwen:

They mentioned in a comment on #1093 that they're working through some technical issues. I'm guessing they're probably working through any minor bugs on the 1.4 release as that was a pretty big one. They haven't forgotten about it 🙃

You can always build this branch or copy in the needed code to you project if you really need.

natelowry · 2020-05-07T19:01:26Z

bumping. i know it won't get missed, mainly wondering if it has a tentative date. didn't know if there was a normal release cadence or it's just as things get merged. if it'll be a while i'll just roll my own, wrapper - now with less quality!™️ :)

anything i can do to help on this?

sagarpatel · 2020-06-16T14:43:04Z

Re-bumping this.
Is the technical issue mentioned before still blocking the progress on this or is just that this feature is low priority/dropped?
Anything we can do to help?

jwvanderbeck-Unity · 2020-12-10T02:21:53Z

Hi,
Based on comments it sounds like this is just held back for logistical reasons? The code in this branch is actually complete and useable? I need to work with pre-recorded files in a project.

jwvanderbeck-Unity · 2021-01-07T14:57:31Z

I know this is sort of stalled, but I just wanted to comment that I have been using this branch now for a while to implement playback and recording inside of a Unity project I am working on and everything has been working like a charm. Thanks for this work!

jwvanderbeck · 2021-01-11T17:59:16Z

According to the C++ code here: https://github.com/microsoft/Azure-Kinect-Sensor-SDK/blob/develop/src/record/sdk/record.cpp#L85

The ColorFormat BGRA32 should be supported for recording, yet when attempting to use that format with the C# recording layer, I get an unsupported color format error.

jwvanderbeck · 2021-01-12T15:41:50Z

According to the C++ code here: https://github.com/microsoft/Azure-Kinect-Sensor-SDK/blob/develop/src/record/sdk/record.cpp#L85

The ColorFormat BGRA32 should be supported for recording, yet when attempting to use that format with the C# recording layer, I get an unsupported color format error.

I see my error here. This branch is actually using older C++ code that doesn't support that color format.

Brent-A added 15 commits October 3, 2019 14:26

Added begining of record/playback for c#

176e9e2

Added record methods

3f2241e

Added minmial playback APIs

6d1a588

Completed Playback API

7209221

Added documentation

310287a

Added some exception details

ad6f6b6

Added tracing redirection to record

c169698

Enabled implicit stride in k4a_image_create

f876d18

Removed default paramter in public API

1797758

Added note on default parameters to standards.md

c6025bb

Merge branch 'issue-623' into csharp-record

3768533

Tests passing, and build config

972324a

Merge branch 'develop' of https://github.com/microsoft/Azure-Kinect-S…

594d672

…ensor-SDK into csharp-record

Cleaned up C# style

0d50feb

Merge branch 'develop' of https://github.com/microsoft/Azure-Kinect-S…

09a6cf3

…ensor-SDK into csharp-record

wes-b previously approved these changes Oct 9, 2019

View reviewed changes

Style cleanup

2b49c26

Brent-A dismissed wes-b’s stale review via 2b49c26 October 9, 2019 18:03

Added more options for native interop

05265bf

Brent-A requested a review from DerekMa-WP October 9, 2019 20:17

Added a few tests cases and fixed failures

5999e19

DerekMa-WP reviewed Oct 11, 2019

View reviewed changes

src/csharp/Examples/Recording/Program.cs

@@ -0,0 +1,102 @@

using System;

Copy link

Contributor

DerekMa-WP Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright header.

DerekMa-WP reviewed Oct 11, 2019

View reviewed changes

src/csharp/Record/Exceptions/AzureKinectAddAttachmentException.cs Show resolved Hide resolved

DerekMa-WP reviewed Oct 11, 2019

View reviewed changes

src/csharp/Record/Exceptions/AzureKinectCreateRecordingException.cs Outdated Show resolved Hide resolved

Update src/csharp/Record/Exceptions/AzureKinectCreateRecordingExcepti…

3a3a059

…on.cs Co-Authored-By: Derek M. <[email protected]>

DerekMa-WP reviewed Oct 14, 2019

View reviewed changes

Brent-A mentioned this pull request Nov 7, 2019

Cannot reference Sensor SDK from .NET Class Library #894

Open

UnaNancyOwen mentioned this pull request Mar 6, 2020

Publish SDK 1.4.0 #1093

Closed

jpj625 mentioned this pull request Jul 23, 2020

ARM support for Azure Kinect #871

Open

thedewi mentioned this pull request Nov 26, 2021

C# Record and Playback APIs (SDK binary compatible version) #1711

Open

7 tasks

natelowry mentioned this pull request Aug 18, 2022

Expose the playback and record APIs through C# #573

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C# Record and Playback APIs #822

C# Record and Playback APIs #822

Brent-A commented Oct 8, 2019

wes-b left a comment

DerekMa-WP Oct 11, 2019

Brent-A Oct 11, 2019

DerekMa-WP Oct 14, 2019

dotMorten Nov 7, 2019

DerekMa-WP Oct 11, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

DerekMa-WP Oct 14, 2019

natelowry commented Feb 19, 2020

sagarpatel commented Mar 6, 2020

UnaNancyOwen commented Mar 27, 2020

natelowry commented Mar 27, 2020

natelowry commented May 7, 2020

sagarpatel commented Jun 16, 2020

jwvanderbeck-Unity commented Dec 10, 2020

jwvanderbeck-Unity commented Jan 7, 2021

jwvanderbeck commented Jan 11, 2021

jwvanderbeck commented Jan 12, 2021

	/// by code that can ensure that the Capture object will not be disposed on another
	/// by code that can ensure that the Device object will not be disposed on another

C# Record and Playback APIs #822

Are you sure you want to change the base?

C# Record and Playback APIs #822

Conversation

Brent-A commented Oct 8, 2019

Fixes #573

Description of the changes:

Before submitting a Pull Request:

I tested changes on:

wes-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natelowry commented Feb 19, 2020

sagarpatel commented Mar 6, 2020

UnaNancyOwen commented Mar 27, 2020

natelowry commented Mar 27, 2020

natelowry commented May 7, 2020

sagarpatel commented Jun 16, 2020

jwvanderbeck-Unity commented Dec 10, 2020

jwvanderbeck-Unity commented Jan 7, 2021

jwvanderbeck commented Jan 11, 2021

jwvanderbeck commented Jan 12, 2021