Remove image sharp #2069

pgrawehr · 2023-04-12T20:04:09Z

Fixes #1767 (and probably a few others)

Removes all references to ImageSharp from Iot.Device.Bindings.dll
Replace standard types (Point, Color, Size, Rectangle) with the types from System.Drawing.Primitives.dll (these are supported on all platforms and even part of the core runtime)
Provide a new BitmapImage class, which is an abstract image type
Provide a ImageFactoryRegistry static class to create instances of images
Provide a SkiaSharpConnector as (one possible) adapter that provides the actual implementation.

I suggest to create a new package for the SkiaSharpConnector (maybe better name it SkiaSharpAdapter?), because other adapters are possible (e.g. plain old System.Drawing.Imaging would do if only used on Windows) and because SkiaSharp is a quite heavy dependency (~100MB with the native dependencies)

Rough testing done, but a quick test of all the heavier affected bindings would be good:

VideoDevice (@raffaeler ?)
RgbLedMatrix (@richlander ?)
Ws28xx (@krwq ?)
Ssd1351 (TBD)
BuildHat (@Ellerbach ?)

Microsoft Reviewers: Open in CodeFlow

joperezr · 2023-04-13T15:55:43Z

[Traige]: FYI @raffaeler and @krwq, we are going to do a design review next week to look into this one in more detail. @pgrawehr will go over the problem and design, so we can try to get this in for 3.0.

pgrawehr · 2023-04-13T16:08:29Z

[Triage]

With Jose's words, this PR tries to solve these problems:

We are removing our tight dependency from ImageSharp
We try to avoid just replacing all ImageSharp types we currently expose to whatever new API we use, to avoid further breaking changes in case we need to change APIs again
Instead, in the APIs we expose, we only use system types (e.g. from System.Drawing.Primitives) and some self-made abstraction types (e.g. BitmapImage)
We provide an implementation for this abstraction (using SkiaSharp),
Optionally, we might provide an implementation using other implementations, like the old one from System.Drawing.Common, for Windows only.
The abstraction layer sits on top of Iot.Device.Bindings, so that it doesn't need to be referenced when not needed.

joperezr · 2023-04-27T15:46:55Z

src/Iot.Device.Bindings/Iot.Device.Bindings.csproj

@@ -22,6 +22,7 @@
    <!-- Excluding samples and test projects when getting source files -->
    <_ExcludeProjectReferences Include="$(DeviceRoot)**/samples/**/*.csproj" />
    <_ExcludeProjectReferences Include="$(DeviceRoot)**/tests/**/*.csproj" />
+    <_ExcludeProjectReferences Include="$(DeviceRoot)SkiaSharpConnector/*.csproj" />


Can you explain why do we need to exclude this? This would make it so that the connector won't be part of the iot.device.bindings library.

Oh I see you created an extra package for that. Can you explain what is the reasoning for separating that piece?

That's by design. The intent is that the connector is in an extra nuget, so that it can be exchanged for another implementation without touching Iot.device.bindings or its API. That means that we can provide different connectors without affecting the API (or the user could write their own).

I'm not following why keeping it as part of IoT.Device.Bindings would not allow for an external implementation without touching Iot.device.bindings. The part that seems wrong to me, is that we are adding an abstract class with no default implementation on the main library, which seems wrong, as ideally we would want IoT.Device.bindings to not require additional packages on top for some bindings to work. Model of having an included implementation on the library, plus the ability to use a different one via a NuGet package seems to satisfy both goals.

I want to avoid that the iot.device.bindings again references large dependencies. SkiaSharp's native implementation file is ~10MB per platform, and one might not want to carry this dependency around if one doesn't need it. There are many bindings that don't need any imaging features, so if you only use those bindings, you would not need to have a large dependency lying around for nothing.

So either we have some bindings that require an additional package to work, or we have many bindings that pull a dependency they don't need. I prefer the first solution.

joperezr · 2023-04-27T15:57:10Z

src/devices/Arduino/ArduinoAnalogController.cs

@@ -35,60 +35,58 @@ public override int PinCount
            get;
        }

-        public override int ConvertPinNumberToLogicalNumberingScheme(int pinNumber)
+        /// <inheritdoc />


I'm failing to understand how all of the changes in the Arduino device are related to this PR. Are those just there because when working on the sample you made some adjustments? Does it make sense to separate these changes into a separate PR?

Yes, most of these changes are improvements for the example. I'm considering to split that into a separate PR, but I'm waiting until the design has been roughly finalized, to avoid diverging branches.

Got it, yeah, we'd probably want those in a separate PR. If anything, it'll make reviewing this PR easier 😃

pgrawehr · 2023-05-04T20:06:57Z

This is now mostly ready for review. A few points still:

The new package needs to be generated. Haven't found how to do that yet.
The example code and the actual graphics library changes should be separated to separate PRs or at least commits.

This is an adapter that uses SkiaSharp as Bitmap handling backend

pgrawehr · 2023-05-18T12:21:06Z

I have now rebased the PR and separated the changes into logical units. Even though they're not fully independent, it should be easier to review now.

I suggest to use Rebase or real merge instead of squash for this PR, to keep the stuff separated in history.

joperezr · 2023-05-18T15:21:49Z

eng/Versions.props

@@ -6,7 +6,7 @@
    <PreReleaseVersionLabel>prerelease</PreReleaseVersionLabel>
    <MicrosoftDotNetGenAPIPackageVersion>7.0.0-beta.23211.2</MicrosoftDotNetGenAPIPackageVersion>
    <!-- dotnet/corefx dependencies -->
-    <SystemDrawingCommonPackageVersion>5.0.3</SystemDrawingCommonPackageVersion>
+    <SystemDrawingCommonPackageVersion>6.0.0</SystemDrawingCommonPackageVersion>


Any reason why we are picking 6.0 as opposed to latest?

I just took the version matching the TargetRuntime version we currently use, but I suppose we could use the 7.0 version as well.

joperezr · 2023-05-18T15:22:58Z

src/Iot.Device.Bindings/CompatibilitySuppressions.xml

@@ -1,6 +1,20 @@
 <?xml version="1.0" encoding="utf-8"?>
 <!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
 <Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+  <Suppression>
+    <DiagnosticId>CP0001</DiagnosticId>
+    <Target>T:BuildHat.Models.BuildHatInformation</Target>


It's really hard to go through all of these, but I'm assuming they are pretty much all around removing APIs that exposed ImageSharp types, right?

There are mainly three kinds of changes:

Apis that were changed from ImageSharp to System.Drawing (e.g. ImageSharp.Color to System.Drawing.Color)

Apis that were using ImageSharp bitmap types now take Iot.Device.Common.BitmapImage

Individual types that were in incorrect namespaces

For the user, the changes typically only require adapting their using definitions.

src/Iot.Device.Bindings.SkiaSharpAdapter/Directory.Build.props

src/Iot.Device.Bindings.SkiaSharpAdapter/Iot.Device.Bindings.SkiaSharpAdapter.csproj

src/devices/Arduino/samples/ApiChecker/TestCases.cs

BitmapImage is the new abstract bitmap type. Changed most bindings to use the new type, and also changed bindings to use System.Drawing again for those types that will still be supported on all platforms (Color, Size, Rectangle, Point)

pgrawehr · 2023-05-31T18:13:29Z

Isolated the relevant changes now. The Ili9341 binding is temporarily broken now, though. The fix will be provided in a separate PR.

krwq · 2023-06-01T06:14:56Z

src/Iot.Device.Bindings.SkiaSharpAdapter/Directory.Build.props

+<Project>
+  <!-- Packaging related properties -->
+  <PropertyGroup>
+    <MajorVersion>2</MajorVersion>


do we actually care about those versions and settings? Presumably adapter is not getting shipped directly as nuget?

wait a sec.. looking at code later seems like this will be a new package, do we actually need it? could this be constructed similarly to other common projects for Iot.Device.Bindings? I might have missed the triage meeting where you discussed this

Yes, we want this as a separate package (to reduce dependencies for those who do not need any bitmap functionality or want to use another imaging library).

I'm not sure how the version for the release is actually computed in our setup though (question for @joperezr )

I think for version we either match Iot.Device.Bindings and System.Device.Gpio or we start from 1 and only update when needed (I'm personally not super opinionated on that - matching versions seems easier)

I think we should match, which is the case now (IIRC). But we should (in a separate PR) maybe improve the way the version number is calculated or defined, so we're sure we get the same number everywhere. That is burried somewhere in the arcade repo I guess, and I don't really understand that stuff.

krwq · 2023-06-01T06:22:05Z

src/devices/Arduino/samples/ApiChecker/RgbLedTest.cs

@@ -57,8 +55,7 @@ public void DoTest()
            float blueCorrection = 0.95f;
            while (!Console.KeyAvailable)
            {
-                var hsv = new Hsv(angle, 1.0f, 1.0f);
-                var rgb = converter.ToRgb(hsv);
+                Color rgb = Color.Blue;


isn't this sample just displaying blue rather than color changing over time?

Yes, this will be fixed with the next PR.

krwq · 2023-06-01T06:25:18Z

src/devices/BuildHat/Models/LedMode.cs

@@ -1,7 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.

-namespace BuildHat.Models
+namespace Iot.Device.BuildHat.Models


does this need sample update?

At the moment, it appears to build fine.

krwq · 2023-06-01T06:26:15Z

src/devices/CharacterLcd/samples/LcdConsole.Samples.cs

+        /// A test method
+        /// </summary>
+        /// <param name="lcd">The lcd driver</param>
+        public static void ValueTest(ICharacterLcd lcd)


where are you calling this new method?

krwq · 2023-06-01T06:30:33Z