Correct and simplify ImageCollection_Item_Get32bppColorDepth_Success#4092
Conversation
The original implementation incorrectly referenced elements in the collection. It also double disposed elements in the collection that under stress could lead to instabilities. And it was too verbose that made it harder to maintain the test. Closes dotnet#3898
ImageCollection_Item_Get32bppColorDepth_Success
| using Bitmap resultImage24bppEmpty = Assert.IsType<Bitmap>(collection[4]); | ||
| Assert.NotSame(image24bppRgbEmpty, resultImage24bppEmpty); | ||
| Assert.Equal(new Size(16, 16), resultImage24bppEmpty.Size); | ||
| Assert.Equal(PixelFormat.Format32bppArgb, resultImage24bppEmpty.PixelFormat); | ||
| Assert.Equal(Color.FromArgb(0xFF, 0x00, 0x00, 0x00), resultImage24bppEmpty.GetPixel(0, 0)); | ||
| Assert.Equal(Color.FromArgb(0xFF, 0x00, 0x00, 0x00), resultImage24bppEmpty.GetPixel(1, 0)); | ||
|
|
||
| using Bitmap resultImage24bppRgbCustom = Assert.IsType<Bitmap>(collection[5]); | ||
| Assert.NotSame(image24bppRgbCustom, resultImage24bppRgbCustom); | ||
| Assert.Equal(new Size(16, 16), resultImage24bppRgbCustom.Size); | ||
| Assert.Equal(PixelFormat.Format32bppArgb, resultImage24bppRgbCustom.PixelFormat); | ||
| Assert.Equal(Color.FromArgb(0xFF, 0xFF, 0x00, 0x00), resultImage24bppRgbCustom.GetPixel(0, 0)); | ||
| Assert.Equal(Color.FromArgb(0xFF, 50, 75, 100), resultImage24bppRgbCustom.GetPixel(1, 0)); | ||
|
|
||
| using Bitmap resultImage32bppEmpty = Assert.IsType<Bitmap>(collection[4]); | ||
| Assert.NotSame(image32bppRgbEmpty, resultImage32bppEmpty); | ||
| Assert.Equal(new Size(16, 16), resultImage32bppEmpty.Size); | ||
| Assert.Equal(PixelFormat.Format32bppArgb, resultImage32bppEmpty.PixelFormat); | ||
| Assert.Equal(Color.FromArgb(0xFF, 0x00, 0x00, 0x00), resultImage32bppEmpty.GetPixel(0, 0)); | ||
| Assert.Equal(Color.FromArgb(0xFF, 0x00, 0x00, 0x00), resultImage32bppEmpty.GetPixel(1, 0)); | ||
|
|
||
| using Bitmap resultImage32bppRgbCustom = Assert.IsType<Bitmap>(collection[5]); |
There was a problem hiding this comment.
This is essentially the underlying issue - instead of referencing collection[2] and collection[3] we would incorrectly reference collection[4] and collection[5] - human error. But the verboseness of the implementation made it harder to spot.
There was a problem hiding this comment.
I'd like to understand the bug, do you have any theory how that can lead to flaky tests? I see the indexing error but that looks like a deterministic error, not something that can lead to flaky tests. Furthermore images 2&3 seem to use the same colors as 4&5 so I see why the test doesn't just fail by default, but I don't see why it would become flaky yet.
It also double disposed elements in the collection that under stress could lead to instabilities.
I'd like to understand that as well, to evaluate whether that instability could be improved separately from this issue. IDisposable implementations should normally trying to be tolerant for double disposal.
Given that you say
By getting a bitmap from the ImageListcollection ImageList will clone the original bitmap.
do you mind pointing out where the double disposal happened?
There was a problem hiding this comment.
This still remains a mystery, there was a lot of guessing and there are still things that I don't understand. This was a "quick" fix, because these tests have been troublesome for some time now.
So some of my thoughts were pure speculations and of "gut feel" kind, but given a sufficiently large pool of data even the most implausible scenario may turn true :)
I think I had reasonable theory yesterday, but I'm struggling to articulate it now. In fact, I see that I should be disposing the retrieved image myself, as it is a copy of an image from the image list. So we have 3 distinct images -> the original image, an ImageList copy, and a retrieved copy. The first two get disposed of, but the last one isn't (explicitly).
Looking at the error message the only (even remotely) plausible conclusion I could come up with was that the order of images somehow changed in the ImageList. So when we expected get index:3 (or 5, line:215 where it failed) we somehow got index:4.
Could this happen because of some underlying changes in the ImageList (e.g. an image for disposed, that changed the order, or another race condition in the underlying ImageList that replaces images)?
Thinking now "double dispose" was a total brain lapse on my part, I think I was looking at the wrong piece of code.
There's one more thing that bother me, and I'm yet to spend time investigating it.
using var image32bppArgbCustom = new Bitmap(16, 16, PixelFormat.Format32bppArgb);
image32bppArgbCustom.SetPixel(0, 0, Color.Red);
image32bppArgbCustom.SetPixel(1, 0, Color.FromArgb(200, 50, 75, 100)); <-- we set this
collection.Add(image32bppArgbCustom);
// ...
using Bitmap resultImage32bppArgbCustom = Assert.IsType<Bitmap>(collection[7]);
Assert.NotSame(image32bppArgbCustom, resultImage32bppArgbCustom);
// ...
Assert.Equal(Color.FromArgb(0xC8, 0x55, 0x68, 0x7B), resultImage32bppArgbCustom.GetPixel(1, 0)); <-- but assert this?There was a problem hiding this comment.
Looking at the error message the only (even remotely) plausible conclusion I could come up with was that the order of images somehow changed in the ImageList.
Thats what seems most likely, yes, but I couldn't come up with a scenario either. The ImageList shouldn't have race conditions, the code is sequential and the ImageList isn't shared with other tests.
There's one more thing that bother me, and I'm yet to spend time investigating it.
I ran some experiments and it looks like the test was written against visual styles disabled, i.e. not using v6 common controls. Copy/pasting the old test into a new project (with asserts stubbed) fails the test, when using v6 common controls I get different color values yet still not the same value written into the bitmap. In short, there is probably a bug somewhere, if images can't roundtrip with alpha transparency then the image list getter is pretty useless when alpha transparent images are used. (I assume adding the image doesn't disturb transparency, at least in v6 common controls, doing a quick google search for known issues and it seems to indicate pre-v6 common controls didn't support alpha transparency at all, and in v6 common controls you can at least load the images and render them. Not sure what goes wrong with WinForms retrieving the images back from the ImageList, definitely worth a separate investigation.)
There was a problem hiding this comment.
Thank you for lookin into this!
I can tell you one thing - even with access to the native code, it is very hard to reason about it... There are multiple layers and COM involved, and because it is optimised and inlined some breakpoints are impossible to hit.
Codecov Report
@@ Coverage Diff @@
## master #4092 +/- ##
====================================================
- Coverage 68.11782% 37.14385% -30.97397%
====================================================
Files 1478 984 -494
Lines 508789 250421 -258368
Branches 41439 36963 -4476
====================================================
- Hits 346576 93016 -253560
+ Misses 155982 151935 -4047
+ Partials 6231 5470 -761
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Wow, this gets weirder by a hour. Another build failed in Debug/x64 with: |
The original implementation incorrectly referenced elements in the collection.
It also double disposed elements in the collection that under stress could lead to instabilities.And it was too verbose that made it harder to maintain the test.
Closes #3898
Microsoft Reviewers: Open in CodeFlow