Skip to content

Simplify CriticalCopyPixels in BitmapSource by removing duplicate type checks #9395

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.

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

Merged
merged 1 commit into from
Feb 28, 2025

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Jul 13, 2024

Description

Removes redundant type checks second-time around when copying pixels in CriticalCopyPixels. There's a small performance improvement, however, the main goal here is decreasing code size, and also code simplification that has been made possible with the introduction of GetArrayDataReference few releases ago.

Since Unsafe.AddByteOffset won't do an index-out-of-bounds check for us, we do it ourselves to keep the same behaviour as previously, throwing IndexOutOfRangeException on when offset exceeds the array length.

Customer Impact

Smaller code-size of PresentationCore.

Regression

No.

Testing

Local build.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 13, 2024 13:01
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Jul 13, 2024
@h3xds1nz h3xds1nz changed the title [BitmapSource.cs] Simplify CriticalCopyPixels by removing duplicate type checks Simplify CriticalCopyPixels in BitmapSource by removing duplicate type checks Sep 1, 2024
@h3xds1nz h3xds1nz force-pushed the bitmap-src-simplify-pinning branch from 01ead7b to ff111c1 Compare October 7, 2024 15:57
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 06

siagupta0202
siagupta0202 previously approved these changes Jan 27, 2025
@siagupta0202
Copy link
Contributor

@h3xds1nz this PR is good to be merged. Can you please resolve the merge conflicts?

@h3xds1nz
Copy link
Member Author

@siagupta0202 Done, sorry for the delay.

@ThomasGoulet73
Copy link
Contributor

@h3xds1nz: Just a ping to let you know that the test failures in this PR were fixed in main, pulling main should fix the failures.

@h3xds1nz h3xds1nz force-pushed the bitmap-src-simplify-pinning branch from adf31ec to 2bd0281 Compare February 28, 2025 09:03
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 10.95136%. Comparing base (d94671d) to head (2bd0281).
Report is 2 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #9395         +/-   ##
===================================================
- Coverage   10.99508%   10.95136%   -0.04373%     
===================================================
  Files           3215        3215                 
  Lines         648472      648440         -32     
  Branches       71534       71528          -6     
===================================================
- Hits           71300       71013        -287     
- Misses        576170      576436        +266     
+ Partials        1002         991         -11     
Flag Coverage Δ
Debug 10.95136% <0.00000%> (-0.04373%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@siagupta0202 siagupta0202 merged commit 83241ee into dotnet:main Feb 28, 2025
8 checks passed
@siagupta0202
Copy link
Contributor

@h3xds1nz this PR is good to be merged. Can you please resolve the merge conflicts?

No problem, and thank you so much for your contributions!

@h3xds1nz
Copy link
Member Author

@siagupta0202 Thank you, happy to contribute :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants