Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove p/invoke to RtlMoveMemory, cleanup caller #42749

Merged
merged 1 commit into from
Sep 27, 2020
+11 −23
Merged

Remove p/invoke to RtlMoveMemory, cleanup caller #42749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 11 additions & 20 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,32 +664,23 @@ public void Save(Stream outputStream)
}
}

private void CopyBitmapData(BitmapData sourceData, BitmapData targetData)
private unsafe void CopyBitmapData(BitmapData sourceData, BitmapData targetData)
{
int offsetSrc = 0;
int offsetDest = 0;
byte* srcPtr = (byte*)sourceData.Scan0;
byte* destPtr = (byte*)targetData.Scan0;

Debug.Assert(sourceData.Height == targetData.Height, "Unexpected height. How did this happen?");
int height = Math.Min(sourceData.Height, targetData.Height);
long bytesToCopyEachIter = Math.Abs(targetData.Stride);

for (int i = 0; i < Math.Min(sourceData.Height, targetData.Height); i++)
for (int i = 0; i < height; i++)
{
IntPtr srcPtr, destPtr;
if (IntPtr.Size == 4)
{
srcPtr = new IntPtr(sourceData.Scan0.ToInt32() + offsetSrc);
destPtr = new IntPtr(targetData.Scan0.ToInt32() + offsetDest);
}
else
{
srcPtr = new IntPtr(sourceData.Scan0.ToInt64() + offsetSrc);
destPtr = new IntPtr(targetData.Scan0.ToInt64() + offsetDest);
}

UnsafeNativeMethods.CopyMemory(new HandleRef(this, destPtr), new HandleRef(this, srcPtr), Math.Abs(targetData.Stride));

offsetSrc += sourceData.Stride;
offsetDest += targetData.Stride;
Buffer.MemoryCopy(srcPtr, destPtr, bytesToCopyEachIter, bytesToCopyEachIter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Buffer.MemoryCopy rather than Span.CopyTo?
Also, why copy by row rather than just the entire bitmap at once (which I would imagine has higher throughput)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer.MemoryCopy because I'm already dealing with raw pointers, so may as well use the pointer-based APIs. Converting them to span would introduce artificial complexity for no benefit. It doesn't make the code "safe", for instance.

Copy by row because the source and the destination might have different stride lengths.

Copy link
Member Author

@GrabYourPitchforks GrabYourPitchforks Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, if the destination stride length is shorter than the source stride length, the copy will still "succeed". Though subsequent row copies might overwrite previous row copies, and I don't know what happens to the data beyond the last row of the dest. Hence some of my nervousness around trying to infer what an appropriate "actual dest length" parameter would be, so I'm continuing to say "eh, just assume the dest is large enough" - which matches what the original code did. I'm not trying to fix any bugs that might exist in this code, just bring the code into compliance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy by row because the source and the destination might have different stride lengths.

Ah, I didn't realize that this supported copying a subset of the original bitmap; that makes sense.

srcPtr += sourceData.Stride;
destPtr += targetData.Stride;
}

GC.KeepAlive(this); // finalizer mustn't deallocate data blobs while this method is running
}

private static bool BitmapHasAlpha(BitmapData bmpData)
Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Drawing.Common/src/System/Drawing/UnsafeNativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ namespace System.Drawing
{
internal class UnsafeNativeMethods
{
[DllImport(ExternDll.Kernel32, SetLastError = true, ExactSpelling = true, EntryPoint = "RtlMoveMemory")]
public static extern void CopyMemory(HandleRef destData, HandleRef srcData, int size);

[DllImport(ExternDll.Kernel32, SetLastError = true)]
public static extern int GetSystemDefaultLCID();

Expand Down