Update System.Drawing to reflect GDI+ changes #32873

qmfrederik · 2020-02-26T16:56:13Z

This is the last part of the work done in dotnet/corefx#40181 which was pending API approval (#30543).

Add support ValueTypePointer encoder parameters (doc)
Support ColorSpace, ImageItems and SaveAsCmyk encoders (doc)
By virtue of 1., fixes a bug where GetEncoderParameterList was broken for the JPEG encoder, which now returns (an empty list of) parameters of type EncoderParameterValueTypePointer. This was broken in .NET Framework, too (see e.g. this StackOverflow report)

Dotnet-GitSync-Bot · 2020-02-26T16:56:17Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

safern · 2020-03-10T18:43:46Z

src/libraries/System.Drawing.Common/tests/ImageTests.cs

@@ -157,6 +157,23 @@ public static IEnumerable<object[]> GetEncoderParameterList_ReturnsExpected_Test
                    new Guid(unchecked((int)0xa219bbc9), unchecked((short)0x0a9d), unchecked((short)0x4005), new byte[] { 0xa3, 0xee, 0x3a, 0x42, 0x1b, 0x8b, 0xb0, 0x6c }) /* Encoder.SaveAsCmyk.Guid */
                }
            };
+
+#if !NETFRAMEWORK


Nit: for tests and test data, we usually use, if (!PlatformDetection.IsFullFramework)

Hmm,

ImageTests.cs(161,36): error CS0117: (NETCORE_ENGINEERING_TELEMETRY=Build) 'PlatformDetection' does not contain a definition for 'IsFullFramework'

What am I missing here?

IsFullFramework, obviously :)

It was removed in dotnet/corefx#37972.

Ugh 😄 .

I'm a bit confused, because dotnet/corefx#37972 and #28619 seem to indicate tests would no longer run on full framework, yet they do for System.Drawing.Common.

@safern What's the way forward here?

The test should run on netfx only if the library targets netstandard2.0. In that case they should test the functionality of the netstandard2.0 library running on NetFX, not testing NetFX code itself.

It's somewhat weird to have new APIs that are not present on certain platform. What will happen if you call them? I'd except something like PlatformNotSupportedException but that only works for new types and only if partial facade is generated. In this case it's new members on existing type.

You will not be able to reference them anymore as for ref we only build live for netcoreapp3.0 and netcoreapp5.0. The netfx assets harvested. So now that you mention it we no longer build netfx assets for System.Drawing.Common anymore as we just get them from the latest package and re-ship them in the new package. Maybe it is fine removing the netfx tests for this library.

cc: @ericstj @ViktorHofer @danmosemsft @JeremyKuhne -- I'm leaning towards removing testing netfx for S.Drawing.Common. What do you guys think?

What do you guys think?

My only concern is that surfacing unintentional deviation in behavior from Framework becomes more difficult without runs there. If we had better test coverage I'd worry less, but I think we're pretty far from adequately covering GDI+ behavior in our current tests.

Yeah that's a fair point. I would suggest leaving the tests on this PR and using the NetFramework property in PlatformDetection; then we can discuss further if we want to stop running tests for netfx in this project.

So, unfortunately we went the full circle on this one - just using PlatformDetection does not work, because there's a reference to Encoder.ImageItems which was added as part of this PR, but is not present in NetFx.

You're right. Ok, well this at least was a good discussion. Then we will need to go back to the compile time condition, sorry about that.

safern

Other than a comment looks good to me.

cc: @JeremyKuhne

stephentoub · 2020-03-13T15:21:41Z

@qmfrederik, if you can address @safern's feedback, seems like we can then get this merged. Thanks.

stephentoub · 2020-03-13T15:22:56Z

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs

@@ -16,6 +16,9 @@ public sealed class Encoder
        public static readonly Encoder LuminanceTable = new Encoder(new Guid(unchecked((int)0xedb33bce), unchecked((short)0x0266), unchecked((short)0x4a77), new byte[] { 0xb9, 0x04, 0x27, 0x21, 0x60, 0x99, 0xe7, 0x17 }));
        public static readonly Encoder ChrominanceTable = new Encoder(new Guid(unchecked((int)0xf2e455dc), unchecked((short)0x09b3), unchecked((short)0x4316), new byte[] { 0x82, 0x60, 0x67, 0x6a, 0xda, 0x32, 0x48, 0x1c }));
        public static readonly Encoder SaveFlag = new Encoder(new Guid(unchecked((int)0x292266fc), unchecked((short)0xac40), unchecked((short)0x47bf), new byte[] { 0x8c, 0xfc, 0xa8, 0x5b, 0x89, 0xa6, 0x55, 0xde }));
+        public static readonly Encoder ColorSpace = new Encoder(new Guid(unchecked((int)0xae7a62a0), unchecked((short)0xee2c), unchecked((short)0x49d8), new byte[] { 0x9d, 0x07, 0x1b, 0xa8, 0xa9, 0x27, 0x59, 0x6e }));
+        public static readonly Encoder ImageItems = new Encoder(new Guid(unchecked((int)0x63875e13), unchecked((short)0x1f1d), unchecked((short)0x45ab), new byte[] { 0x91, 0x95, 0xa2, 0x9b, 0x60, 0x66, 0xa6, 0x50 }));
+        public static readonly Encoder SaveAsCmyk = new Encoder(new Guid(unchecked((int)0xa219bbc9), unchecked((short)0x0a9d), unchecked((short)0x4005), new byte[] { 0xa3, 0xee, 0x3a, 0x42, 0x1b, 0x8b, 0xb0, 0x6c }));


@safern, @carlossanlop, I assume we want to add XML comments to these in order to populate the docs? For consistency maybe we should do so for all the fields here.

@stephentoub, good catch. @qmfrederik could you please also include XML docs for these new APIs?

@qmfrederik the only information that the existing APIs need on top of them as triple slash is their summary, which you can get from their MS Docs documentation.

qmfrederik · 2020-03-13T17:38:16Z

OK, I think I addressed the feedback.

The XML comments keep the same format used in the previous versions of the framework. It doesn't look like GDI+ has docs with an elaborate description of these encoder parameters, so I left it at that.

safern · 2020-03-13T17:55:40Z

@carlossanlop could you sign off on the XML comments?

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs

carlossanlop

Looks good for documentation, @qmfrederik. But just for consistency with the existing documentation in MS Docs, please consider applying the suggestions I offered.

safern · 2020-03-16T18:49:24Z

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs

+        public static readonly Encoder ImageItems = new Encoder(new Guid(unchecked((int)0x63875e13), unchecked((short)0x1f1d), unchecked((short)0x45ab), new byte[] { 0x91, 0x95, 0xa2, 0x9b, 0x60, 0x66, 0xa6, 0x50 }));
+
+        /// <summary>
+        /// An <see cref="Encoder" /> object that is initialized with the globally unique identifier for the save as CMYK category.


I think we try to use the full name even if it is not needed, but not sure. @carlossanlop ?

Do you mean using <see cref="T:System.Drawing.Imaging.Encoder" />? No, it's not needed in triple slash comments. If the API was not recognized within this context, VS would give you an error. But in this case, we recognize the Encoder type.
When the project gets built and the triple slash comments get converted to their respective xml files, all API names will be automatically converted to their full name.

Using the T: prefix results in build errors: error CA1200: Avoid using cref tags with a prefix

qmfrederik · 2020-03-18T12:46:31Z

src/libraries/System.Drawing.Common/tests/ImageTests.cs

+                        Encoder.Quality.Guid,
+                        Encoder.LuminanceTable.Guid,
+                        Encoder.ChrominanceTable.Guid,
+                        Encoder.ImageItems.Guid


So, the ImageItems property was added in this PR, but is not present in the NetFramework builds since they type forward to System.Drawing, which does not have this property.

How should we tackle this?

- Add support ValueTypePointer encoder parameters - Support ColorSpace, ImageItems and SaveAsCmyk encoders

qmfrederik · 2020-03-18T22:41:56Z

/azp help

azure-pipelines · 2020-03-18T22:42:00Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

qmfrederik · 2020-03-18T22:42:28Z

/azp run "runtime (Installer Build and Test Linux_x64 Debug)"

azure-pipelines · 2020-03-18T22:42:33Z

Commenter does not have sufficient privileges for PR 32873 in repo dotnet/runtime

qmfrederik · 2020-03-18T22:42:59Z

The build failures seem unrelated but I don't have sufficient permissions to trigger a rerun.

safern · 2020-03-18T22:44:48Z

I re-ran them. You can always go to the checks tab and click on re-run no?

qmfrederik · 2020-03-18T22:58:28Z

Nope, I don't have those permissions. #718 has some more information.

Looking forward to a green CI and getting this merged!

Dotnet-GitSync-Bot added area-System.Drawing new-api-needs-documentation labels Feb 26, 2020

danmoseley requested a review from safern February 27, 2020 23:58

safern reviewed Mar 10, 2020

View reviewed changes

safern approved these changes Mar 10, 2020

View reviewed changes

stephentoub reviewed Mar 13, 2020

View reviewed changes

stephentoub approved these changes Mar 13, 2020

View reviewed changes

carlossanlop reviewed Mar 13, 2020

View reviewed changes

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs Outdated Show resolved Hide resolved

carlossanlop reviewed Mar 13, 2020

View reviewed changes

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs Outdated Show resolved Hide resolved

carlossanlop reviewed Mar 13, 2020

View reviewed changes

src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs Outdated Show resolved Hide resolved

carlossanlop approved these changes Mar 13, 2020

View reviewed changes

safern reviewed Mar 16, 2020

View reviewed changes

safern approved these changes Mar 17, 2020

View reviewed changes

qmfrederik commented Mar 18, 2020

View reviewed changes

qmfrederik added 3 commits March 18, 2020 21:48

Update System.Drawing to reflect GDI+ changes

c90c59f

- Add support ValueTypePointer encoder parameters - Support ColorSpace, ImageItems and SaveAsCmyk encoders

Skip test on NetFx

24bf7df

Add XML documentation

b9f76ee

safern merged commit e3fd0e0 into dotnet:master Mar 19, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update System.Drawing to reflect GDI+ changes #32873

Update System.Drawing to reflect GDI+ changes #32873

qmfrederik commented Feb 26, 2020

Dotnet-GitSync-Bot commented Feb 26, 2020

safern Mar 10, 2020

qmfrederik Mar 17, 2020

filipnavara Mar 17, 2020 •

edited

Loading

qmfrederik Mar 17, 2020

filipnavara Mar 17, 2020

safern Mar 17, 2020

JeremyKuhne Mar 17, 2020

safern Mar 17, 2020

qmfrederik Mar 18, 2020

safern Mar 18, 2020

safern left a comment

stephentoub commented Mar 13, 2020

stephentoub Mar 13, 2020

safern Mar 13, 2020

carlossanlop Mar 13, 2020 •

edited

Loading

qmfrederik commented Mar 13, 2020

safern commented Mar 13, 2020

carlossanlop left a comment

safern Mar 16, 2020

carlossanlop Mar 16, 2020 •

edited

Loading

qmfrederik Mar 16, 2020

qmfrederik Mar 18, 2020

qmfrederik commented Mar 18, 2020

azure-pipelines bot commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

azure-pipelines bot commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

safern commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

Update System.Drawing to reflect GDI+ changes #32873

Update System.Drawing to reflect GDI+ changes #32873

Conversation

qmfrederik commented Feb 26, 2020

Dotnet-GitSync-Bot commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipnavara Mar 17, 2020 • edited Loading

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

safern left a comment

Choose a reason for hiding this comment

stephentoub commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

qmfrederik commented Mar 13, 2020

safern commented Mar 13, 2020

carlossanlop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmfrederik commented Mar 18, 2020

azure-pipelines bot commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

azure-pipelines bot commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

safern commented Mar 18, 2020

qmfrederik commented Mar 18, 2020

filipnavara Mar 17, 2020 •

edited

Loading

carlossanlop Mar 13, 2020 •

edited

Loading

carlossanlop Mar 16, 2020 •

edited

Loading