Camera binding #2150

raffaeler · 2023-10-17T10:32:59Z

This PR adds a new binding to transparently capture still pictures or video streams using the apps installed by default in the OS.
One of the setting factories specifically targets the Raspbian OS. This allows to transparently use the legacy raspicam stack or the newest libcamera stack using the apps provided by the OS and capturing the stdio output containing the binary data.

The Camera.Samples project shows how to:

list the installed cameras (only on the libcamera stack)
capture a still using raspistill
capture a video using raspivid
capture a sequence of pictures at a regular interval (time-lapse) using raspistill
capture a still using libcamera-still
capture a video using libcamera-vid
capture a sequence of pictures at a regular interval (time-lapse) using libcamera-still

The unit tests use a console application to test the communication channel via stdout/stdin.
The sample application has been tested on both Buster (raspicam) and Bullseye (libcamera).

Reference issues:

Microsoft Reviewers: Open in CodeFlow

… external utilities that are part of the OS

…e legacy or libcamera stack. Various fixes

…Tested with all the configurations on both the legacy and libcamera stacks

krwq · 2023-10-17T11:03:52Z

src/devices/Camera/Camera/Camera.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>$(DefaultSampleTfms)</TargetFramework>


I think this should be $(DefaultBindingTfms)

Thank you for your help @krwq. Now everything is ok.

Ellerbach · 2023-10-17T12:07:31Z

src/devices/Button/tests/ButtonTests.cs

not sure why you have the button part of this PR :-)

No idea, it was on a different branch :-(

raffaeler · 2023-10-19T06:51:51Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+public class ProcessRunnerTests
+{
+    private const string FakeVideocapture =
+        @"..\..\..\..\FakeVideoCapture\bin\Debug\net6.0\FakeVideoCapture.exe";


I am not sure this is the best way in our repo to reference the external process as this string contains the net6.0 tfm.

An alternative would be to reference the project from the unit tests so that the exe is copied in its binary folder.

Other ideas?

Yes, you can for example do the following:

search for this file in a recursive way up to the moment you'll find one.

Or, you can as well copy it into your test project. You can then set it to copy in the artifact. You just need to build the FakeVideoCapture project first.

Thank you @Ellerbach

I was going with the option 1 but there no Initialization phase in xunit

Not sure how to do the option 2 (both phases)

But I tried to simply add a reference to the exe and it gets copied to the unit test project binary folder.
Do you see problems with this solution?

BTW I just committed this solution as it looks good.

krwq · 2023-10-19T15:52:36Z

[Triage] See if we can reuse process runner

iot/src/devices/Rtc/SystemClock.cs

Line 234 in 2ed71a9

process.Start();

raffaeler · 2023-10-19T18:35:47Z

[Triage] See if we can reuse process runner

iot/src/devices/Rtc/SystemClock.cs

Line 234 in 2ed71a9

process.Start();

Yes, it can replace the code in the RTC as it use the same ProcessStartInfo settings.
The ProcessRunner does more:

deals with text or binary data
implements the async pattern
deals with continuous capturing that can be programmatically stopped
Expose an overload that takes a PipeWriter instead of a Stream to simplify consuming complex data

Given we can reuse the ProcessRunner, what's the most appropriate place for it? (Assembly / Namespace). /cc @krwq

pgrawehr · 2023-10-20T10:35:47Z

Given we can reuse the ProcessRunner, what's the most appropriate place for it? (Assembly / Namespace).

I'd just add it to the Iot.Device.Common namespace( devices\Common project)

Ellerbach

Nice binding! Comments mainly regarding intellisense and couple of other quite minor elements

Ellerbach · 2023-10-20T11:29:07Z

src/devices/Camera/Camera/ProcessRunner.cs

+            }
+        }
+        catch (Exception)
+        {


So can you then just add a note that there is nothing to do in the catch and it's on purpose for this reason?

Ellerbach · 2023-10-20T11:31:23Z

src/devices/Camera/Camera/ProcessRunner.cs

+    /// the Environment.CurrentDirectory.
+    /// The working directory is important when referring to files from the command line.
+    /// </summary>
+    public string GetFullCommandLine(string[] arguments)


you are missing the return type and description in the intellisense comment.
And the parameters as well.

I have found that we haven't enabled the "Return value must be documented" stylecop rule. I'm going to enable it, but I need to fix quite a bunch of comments to do so.

I hope I didn't forget any of them, but it was an easy but long work

When using Visual Studio, the params, the return and whatever else including exceptions are added automatically, so it makes life easier! Also, with copilot, most of the comments can be done automatically with looking at the result.
When developing, just disable the rules for comments and activate it back once you're done and create the intellisense comments at once.

Please note that stylecop does not warn about the missing comment for parameters in addition to the return type.

When using Visual Studio, the params, the return and whatever else including exceptions are added automatically

I know, but every company has different ruleset and when I write code sometimes I jump from one style to another. Since VS did not complain, I committed without the other set of comments.

Ellerbach · 2023-10-20T11:31:30Z

src/devices/Camera/Camera/ProcessRunner.cs

+    /// Execute the process with a number of arguments. The target
+    /// Stream receives the stdout of the process
+    /// </summary>
+    public Task ExecuteAsync(string[] arguments, Stream? target)


you are missing the return type and description in the intellisense comment.
And the parameters as well.

Ellerbach · 2023-10-20T11:31:35Z

src/devices/Camera/Camera/ProcessRunner.cs

+    /// If the process is not expected to return any output (for example when
+    /// the app directly writes one or more files), the stream can be null
+    /// </summary>
+    public async Task ExecuteAsync(string argsString, Stream? target)


you are missing the return type and description in the intellisense comment.
And the parameters as well.

Ellerbach · 2023-10-20T11:31:41Z

src/devices/Camera/Camera/ProcessRunner.cs

+    /// Execute the process with the given arguments and
+    /// returns the output as a string, decoded as UTF8
+    /// </summary>
+    public async Task<string> ExecuteReadOutputAsStringAsync(string[] arguments)


you are missing the return type and description in the intellisense comment.
And the parameters as well.

src/devices/Camera/tests/TestCamera/CommandOptionsBuilderTests.cs

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

Ellerbach · 2023-10-20T12:01:23Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+    [Fact]
+    public async Task TestBinary2()
+    {
+        /* CreateTestFile(Video1); */


why is this part commented?

It is the code that I used to create the test.bin file that is added in the project.
With this code the file can be re-created.

Can you then please add a little comment about is in the code?

Ellerbach · 2023-10-20T12:01:34Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+    [Fact]
+    public async Task TestText1()
+    {
+        /* CreateTestFile(Video1); */


Why is this code commented?

Same as before.

Ellerbach · 2023-10-20T12:01:45Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+    [Fact]
+    public async Task TestText2()
+    {
+        /* CreateTestFile(Video1); */


why is this code commented?

Same as before.

So please add the same comment :-D

Made the required review changes on the comments.

Fixed the SkiaSharpAdapter in Media.sln

raffaeler · 2023-10-20T15:21:42Z

@Ellerbach apart the comments where I responded differently, I completed the requested changes.
As for the triage, I also moved the ProcessRunner and ProcessRunnerSettings into the Common assembly.

src/devices/Camera/Camera/CameraInfo.cs

Ellerbach · 2023-11-03T07:44:41Z

src/devices/Camera/Camera/Settings/LibcameraAppsSettings.cs

+public class LibcameraAppsSettings
+{
+    /// <summary>
+    /// The default options for the Libcamera-apps


Very minimal: in theory, properties comments should start with "Gets or sets" ("Gets" only in this case")

Ellerbach · 2023-11-03T07:46:07Z

src/devices/Camera/README.md

@@ -0,0 +1,293 @@
+# The Camera binding


You may have skip this feedback but I would definitely do 2 different pages. 1 simple README for the usage and a more detailed one for the technicalities of the choices. So for a user it stays simple and straight forward.

Ellerbach

Very little things and then good to go! Approving already :-)

Ellerbach · 2023-11-03T07:48:31Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+    [Fact]
+    public async Task TestBinary2()
+    {
+        /* CreateTestFile(Video1); */


Can you then please add a little comment about is in the code?

src/devices/Camera/tests/TestCamera/CommandOptionsBuilderTests.cs

Ellerbach · 2023-11-03T07:49:32Z

src/devices/Camera/tests/TestCamera/ProcessRunnerTests.cs

+    [Fact]
+    public async Task TestText2()
+    {
+        /* CreateTestFile(Video1); */


So please add the same comment :-D

raffaeler · 2023-11-03T08:20:22Z

@Ellerbach I splitted the README.md in two documents. The other one is called CameraInsights.md

Ellerbach · 2023-11-03T09:32:50Z

@Ellerbach I splitted the README.md in two documents. The other one is called CameraInsights.md

All good for me! It's much better from a user point of view. All good for the rest and good to go for me (once the build will be fixed)!

raffaeler · 2023-11-03T10:29:55Z

@Ellerbach I splitted the README.md in two documents. The other one is called CameraInsights.md

All good for me! It's much better from a user point of view. All good for the rest and good to go for me (once the build will be fixed)!

@Ellerbach now it's ok. The markdown linter complained a microsoft url not reachable (on another binding) and a couple of line feed at the end of the documents (my visual editor always modifies them).

krwq · 2023-11-07T10:39:30Z

src/devices/Camera/Camera/Settings/CommandOptionsBuilder.cs

+    /// </summary>
+    /// <param name="ms">The length of the capture operation in milliseconds.</param>
+    /// <returns>A reference to this instance.</returns>
+    public CommandOptionsBuilder WithContinuousStreaming(int ms = 0)


Nit: It would be really nice to convert these args to TimeSpan where possible but feel free to do it separately or ignore this since it's minor feedback

If you don't mind I would discuss this in a later PR because I could not find a way to specify the default values as const
(TimeSpan.Zero is not const and 1 ms as well)

The following should work as default arguments: new TimeSpan(0) or new TimeSpan(valueAs100nsTicks). As an alternative, you could use overloading instead of default arguments.

But I agree that's something that could be done separately.

Ok, let's discuss it for later then :)

krwq · 2023-11-07T10:50:40Z

src/devices/Camera/Camera/Settings/CommandOptionsBuilder.cs

+    /// </summary>
+    /// <param name="quality">The quality of each MJPEG picture.</param>
+    /// <returns>A reference to this instance.</returns>
+    public CommandOptionsBuilder WithMJPEGVideoOptions(int quality)


nit: we don't do fluent APIs here elsewhere (I think), feel free to ignore this comment

sure, I did it just for uniformity ... also it felt easier when using it in the sample/test code

src/devices/Camera/Camera/Settings/CommandOptionsBuilder.cs

krwq · 2023-11-07T13:45:55Z

src/devices/Camera/tests/TestCamera/CameraInfoTests.cs

+    /// </summary>
+    /// <returns>The test task operation.</returns>
+    [Fact]
+    public async Task TestCameraList()


this test seems camera specific, will this work with any camera?

The test is meant to validate the parser using the text.txt content grabbed from my RPi.
The output you obtain with --list-cameras is always formatted using the same pattern (as far as I could see).

raffaeler added 22 commits August 23, 2023 14:07

Holding button test change to measure effective time elapsed

6c3974a

Raising the timeout for holding tests

00c792d

Fix the other holding test and shorten the time of all the other tests

c2f91a5

Making the test itself async

ba24a74

Merge branch 'main' into button_tests

fee9625

changed strategy for signalling the holding event

01c7dbc

adding logging to the button tests

da29b97

New Camera binding

8749300

Provides support to catpure still images or video streaming using the…

8ea83f9

… external utilities that are part of the OS

Added the command enumeration to retrieve the command line string easily

a5f681b

Added fluent api to build the command options

d2dc432

README revised

031bb9b

Creating the string array for the arguments

7f26763

Comand line builder test

9a41d10

Testing the uniqueness of the options

7201ea4

Diagnostics

5adb360

doc fix

0997049

Created the samples to capture still, video, timelapse either with th…

cf8dd1d

…e legacy or libcamera stack. Various fixes

Minor fixes and cleanup

9dac082

Added the last missing commands. Added the configuration for stderr. …

e1b2e60

…Tested with all the configurations on both the legacy and libcamera stacks

cleanup

c29158c

Doc update

1f44e3e

ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Oct 17, 2023

raffaeler added 2 commits October 17, 2023 12:41

doc fixes

9c96d76

doc fix

4044d99

krwq reviewed Oct 17, 2023

View reviewed changes

raffaeler added 3 commits October 17, 2023 13:19

Fixed the tfms in the projects

f735bca

'Moved the projects

704fe8e

Added System.IO.Pipelines to Iot.Device.Binding

fcf9a7e

Ellerbach reviewed Oct 17, 2023

View reviewed changes

raffaeler commented Oct 19, 2023

View reviewed changes

Referenced the FakeVideocapture from tests to get the exe copied

16f4574

Ellerbach reviewed Oct 20, 2023

View reviewed changes

raffaeler added 2 commits October 20, 2023 16:09

Moved the process runner and settings into the Common assembly.

3137d6f

Made the required review changes on the comments.

Fixed the piplines version on MjpegStream project

664b033

Fixed the SkiaSharpAdapter in Media.sln

Added a more verbose comment

f294a7d

pgrawehr approved these changes Nov 2, 2023

View reviewed changes

Ellerbach reviewed Nov 3, 2023

View reviewed changes

Ellerbach approved these changes Nov 3, 2023

View reviewed changes

Comments and docs changes based on the review

1f7ac04

markdown fixes

20b893d

krwq reviewed Nov 7, 2023

View reviewed changes

src/devices/Camera/Camera/Settings/CommandOptionsBuilder.cs Outdated Show resolved Hide resolved

comment fix as for review

1939728

krwq reviewed Nov 7, 2023

View reviewed changes

krwq approved these changes Nov 7, 2023

View reviewed changes

krwq merged commit a5181f5 into dotnet:main Nov 9, 2023
9 checks passed

pgrawehr added this to the v3.1.0 milestone Nov 9, 2023

This was referenced Nov 10, 2023

Capture images from Raspberry Pi Camera Module #2086

Closed

VideoDevice issues with color accuracy. #1688

Closed

Libcamera stack support (features, APIs and discussions) #1875

Closed

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

This pull request was closed.

Camera binding #2150

Camera binding #2150

Conversation

raffaeler commented Oct 17, 2023 • edited Loading

Microsoft Reviewers: Open in CodeFlow

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

krwq commented Oct 19, 2023 • edited Loading

raffaeler commented Oct 19, 2023

pgrawehr commented Oct 20, 2023

Ellerbach 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

raffaeler commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ellerbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raffaeler commented Nov 3, 2023

Ellerbach commented Nov 3, 2023 • edited Loading

raffaeler commented Nov 3, 2023

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

raffaeler commented Oct 17, 2023 •

edited

Loading

krwq commented Oct 19, 2023 •

edited

Loading

Ellerbach commented Nov 3, 2023 •

edited

Loading