Fix Behavior Differences in Unix System.Drawing.Common

[mellinoe]

This is a tracking issue to mark failing tests against. I will fill this out with more details as I understand them.

[hughbe]

What's the compat story for unix and GdIWindows? Are we looking for 1-1 compat even with bugs and edge cases?

[mellinoe]

I think it will probably be unreasonable to try to achieve "pixel perfect", 100% identical behavior for most of the library. I think we will need to be more lenient for edge case behavior and bugs in GDI+, but we can examine them case-by-case. I think once we get more tests online, behavior differences will become obvious. From what I've seen running your Icon tests on the Unix implementation, there are a lot of differences in edge-case and exception behavior, which are fixable by trying to re-use parts of the Windows implementation.

@akoeplinger @marek-safar Any other thoughts to add?

[akoeplinger]

I agree we'll have to try and see which edge-cases we run into and pixel perfect matching in all cases shouldn't be a goal (at least initially).

[hughbe]

At least from looking at Icon.cs, a bunch of edge cases will be in exception types, disposal of objects and argument validation. This can be fixed by creating a PAL layer between the two implementations by starting to consolidate common code (preferring Windows where applicable)

[mellinoe]

@hughbe Yep, that was what I thought as well. I'm hopeful that a lot of the exception behavior can be unified in that way.

Another good thing about Icon.cs is that it seems to be 90% managed code. There seem to be functional problems with some of the tests (lots of icons returning the wrong Width and Height), but that should be fixable from the C# code.

[mellinoe]

Here's some behavior differences I've discovered while trying to unify the Pen code, and other related types.

Pen

• Pen.get_CustomEndCap is emulated on Unix. There is no "get" accessor for this property in libgdiplus, so we return the last-known value that was set in managed code. This will most often be correct, but it is valid to set this prorpety through other means (PInvoke, native code, etc.), so the cached value is not guaranteed to be in sync with the native state of the object.
  NOTE: There are other places in the Windows System.Drawing code which make the same assumption, although it is wrong.
• The get accessor of Pen.Color has different behavior on Unix when the value has never been set. Instead of throwing an ArgumentException, the code returns a "zero" color.

CustomLineCap

• libgdiplus does not implement GetCustomLineCapType. Currently this is only used to determine which subclass of CustomLineCap to return in its Clone method. I believe we can work around this limitation by just overriding the clone method in AdjustableArrowCap.

[mellinoe]

StringFormat

SetDigitSubstitution does not work for some inputs on libgdiplus.

[mellinoe]

PrinterSettings

Creating a PrinterSettings object with an invalid name causes a NullReferenceException.

[ViktorHofer]

@safern I think we should prioritize this item. Do you have plan to address the mentioned issues here or should I step in and help?

[safern]

I've found plenty of tests disabled against this issue. I think we need to see which ones are still failing, try to consolidate all the exceptions between Windows and Unix and then all the libgdiplus related issues put them in one issue in corefx so that we can track them all, disable the tests against that issue and then open an issue in libgdiplus to get them fix and try to have a more stable cross library that compatibility is better.

[filipnavara]

@akoeplinger How feasible would it be to run the CoreFX System.Drawing.Common tests as part of the CI for libgdiplus?

[joshfree]

Moving to the Future milestone

[filipnavara]

What's best way to update the [ActiveIssue(20884, TestPlatforms.AnyUnix)] tests if we fix an underlying bug in libgdiplus? Should we introduce a new ConditonalFact like GdiplusIsAvailable, but with test for a specific version?

[qmfrederik]

I spent some time testing libgdiplus using the CoreFX test suite. I did that by building libgdiplus on Windows, and manually patching corefx code so that it would use libgdiplus.dll instead of gdiplus.dll.

I currently see about ~75% of all drawing tests passing on libgdiplus on Windows.

Here are a couple things I learned:

• There were a couple of bugs which would cause the process to crash. Don't attempt to implement GdipCreateFromHDC on Windows mono/libgdiplus#557, GdipCreateBitmapFromScan0: Fix a buffer overflow with pixel formats < 32 bpp mono/libgdiplus#558, Don't implement GdipCreateBitmapFromHICON on Windows mono/libgdiplus#559 and Implement deep cloning of ImageAttributes mono/libgdiplus#560 fix these crashes.

• There are a couple of corefx tests which pass invalid pointer values (e.g. (IntPtr)10) to libgdiplus, which still cause the process to crash.

• I've fixed a bunch of other compat issues - see Keep GraphicsContainer/GraphicsState non-zero mono/libgdiplus#561, Matrix: Handle boundary values mono/libgdiplus#562, Return OutOfMemory for 0-length gradient brushes mono/libgdiplus#564, CustomLineCap: Better argument validation mono/libgdiplus#565 and Various Image Attribute fixes mono/libgdiplus#567

• On macOS, the test process still aborts because of exceptions in finalizers (even after Clean up System.Drawing.Common unit tests corefx#38771):

   Reason: Unhandled Exception: System.InvalidOperationException: Object is currently in use elsewhere.
     at System.Drawing.Graphics.Flush(FlushIntention intention)
     at System.Drawing.Graphics.Dispose()
     at System.Drawing.Graphics.Finalize()
  

• There's still a bunch of low hanging fruit which can be tackled:

  • Argument validation
  • For drawing, there are rounding differences and other non-material differences. We should probably loosen the tests a bit
  • For drawing, some of the theories create test data which libgdiplus doesn't support. For example, when the data method for a theory would create a bitmap with 64-bit pixel data, you'll get an NotImplementedException. This will cause entire theory not to run, because the data method errors out. We should probably update the data methods to not return anything which isn't supported on libgdiplus when running on Unix, or use delegates, so that the individual test fails, and not the data method (causing the entire theory to be skipped).

• While doing this, I converted the failing CoreFX tests I was working on to a unit test in C (using the Google Test framework). This integrates within Visual Studio, so you get a very nice developer experience. Porting CoreFX tests for System.Drawing to a unit test in C for libgdiplus is pretty straightforward - see tests.cpp for an overview.

• libgdiplus runs the Mono System.Drawing test suite during CI. Mono has been gradually changing tests from its own test suite to corefx tests. libgdiplus does not run the corefx tests in CI, so test coverage has been decreasing.