Consolidate the Image class, fix bug#40181
Consolidate the Image class, fix bug#40181safern merged 18 commits intodotnet:masterfrom CoreCompat:fixes/graphics-consolidate
Conversation
|
I filed #40182 in case the new values to Encoder, EncoderParameterValueType require an API proposal. I can exclude these changes from this PR if that speeds up things. |
|
/azp run corefx-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Linux x64_Debug failed with
|
| case EncoderParameterValueType.ValueTypeRationalRange: | ||
| size = 2 * 2 * 4; | ||
| break; | ||
| case EncoderParameterValueType.ValueTypePointer: |
There was a problem hiding this comment.
This branch uses ValueTypePointer, which was added to EncoderParameterValueType (public API) as part of this PR. That value is required to implement the fix. I can hard-coded it to 9, if you want to keep the API & the fix separate, though.
There was a problem hiding this comment.
Hmm I think we should keep the fix separate from this PR until the API is approved.
There was a problem hiding this comment.
OK, I reverted that change for now. Any idea how long it would take for that API to get reviewed?
There was a problem hiding this comment.
No idea. I just marked it as read-for-review, and it is marked as ready-for-review, so it is just a matter of waiting for the API Review team to get to it.
qmfrederik
changed the title
|
@safern Anything else you need for this PR? |
|
|
||
| ValidateImage(image); | ||
|
|
||
| Image img = CreateImageObject(image); |
There was a problem hiding this comment.
I think in Unix we should be able to cleanup and remove CreateFromHandle and just move the code to CreateImageObject, no?
There was a problem hiding this comment.
Those things are rabbit holes 😄. CreateImageObject only differed in the exception that is thrown (good, so we streamlined that, too), and the way a Metafile object is created from a handle. So I cleaned that up, too. Looks like there are some opportunities left in Metafile, let me see if I can cook up a separate PR for that.
There was a problem hiding this comment.
Cool. Thanks. Taking another look.
Share the following methods across Windows & Unix: - FromFile - GetEncoderParameterList - GetPixelFormatSize - IsAlphaPixelFormat
Metafile: Consolidate initialization from native handle
| } | ||
| } | ||
|
|
||
| [ActiveIssue(20884, TestPlatforms.AnyUnix)] |
There was a problem hiding this comment.
Love this. Hopefully CI is happy about it 😄
|
Thanks @qmfrederik nice cleanup. The |
* Add new values for Encoder, EncoderParameterValueType * Support encoder parameters of type ValueTypePointer * Fix an overflow error when reading EncoderParameters * Add unit tests * Consolidate the Image class Share the following methods across Windows & Unix: - FromFile - GetEncoderParameterList - GetPixelFormatSize - IsAlphaPixelFormat * Fix P/Invoke declaration * Fix compliation on NetFX * Skip test on NetFX * Fix typo * Always skip GetEncoderParameterList_ReturnsExpected on netFX * Remove new API * Revert changes which require an API change * Suppress failing test * Image: Consolidate CreateImageObject Metafile: Consolidate initialization from native handle * Throw SkipTestException instead of silently skipping the test * Enable some on Unix which rely on consolidated code * Add missing using statement * Get SkipTestException right Commit migrated from dotnet/corefx@31a04af
4 participants
This PR digs a bit deeper into the
System.Drawing.Imageclass:GetEncoderParameterListwas broken on .NET Core (worked on .NET Framework) because of a bug inEncoderParameters.ConvertFromMemory. Fixes OverflowException from GetEncoderParameterList docs#5607, dotnet/corefx#33188GetEncoderParameterListwas broken for the JPEG encoder, which now returns (an empty list of) parameters of typeEncoderParameterValueTypePointer. This was broken in .NET Framework, too (see e.g. this StackOverflow report)IsAlphaPixelFormatandGetPixelFormatSizewere different; I kept the Windows implementation and added unit tests based on the Unix implementation.As a result of 2, I also updated
Encoderdoc andEncoderParameterValueTypedoc with values which have been added in newer versions of GDI+.