Skip to content

Perf: Convert only the required source region in DrawImage#407

Open
christianrondeau wants to merge 1 commit into
SixLabors:mainfrom
mediaclip:perf/drawimage-region-conversion
Open

Perf: Convert only the required source region in DrawImage#407
christianrondeau wants to merge 1 commit into
SixLabors:mainfrom
mediaclip:perf/drawimage-region-conversion

Conversation

@christianrondeau

Copy link
Copy Markdown

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

While testing for performance improvement opportunities, I noticed that using the Draw API with an Image onto an Image, ALL pixels of the source image were converted before drawing (https://github.com/SixLabors/ImageSharp.Drawing/blob/main/src/ImageSharp.Drawing/Processing/DrawingCanvas%7BTPixel%7D.cs#L665).

This PR crops the source first and converts the cropped region after.

Impact

Measured on a 22.9 MP image (5700×4014), drawing a 1/8-area region using a custom MemoryAllocator for measuring:

Before: 103MB, 24.4ms (4.3x less memory)
After: 24MB, 14.4 ms (1.7x faster)

NOTE: This also fixes a small leak, this clone was never disposed: https://github.com/SixLabors/ImageSharp.Drawing/blob/main/src/ImageSharp.Drawing/Processing/DrawingCanvas%7BTPixel%7D.cs#L665

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes DrawingCanvas<TPixel>.DrawImage(Image, ...) when drawing from a source Image whose pixel format differs from the canvas pixel format by converting only the actually-sampled (clipped) source region instead of converting the entire source image up-front. As part of that refactor, it also ensures temporary converted images are disposed when the draw operation short-circuits early (fixing the reported leak).

Changes:

  • Pre-compute the effective clipped source/destination rectangles and, for foreign pixel formats, crop in the source format before converting to TPixel.
  • Centralize source/destination clipping logic into TryGetDrawImageClip(...) and reuse it from both the public and internal draw paths.
  • Add tests asserting that “convert clipped region then draw” matches “convert whole image then draw” across multiple clipping/transform scenarios (including a projective transform).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/ImageSharp.Drawing.Tests/Processing/DrawingCanvasTests.DrawImage.cs Adds regression tests ensuring foreign-format, clipped-region conversion produces identical output to full-image conversion across several scenarios.
src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs Crops before conversion for foreign pixel formats, factors clipping into TryGetDrawImageClip, and disposes owned temporary images on early-exit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86%. Comparing base (f23980d) to head (6e136c3).

Files with missing lines Patch % Lines
...eSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs 58% 5 Missing and 5 partials ⚠️
Additional details and impacted files
@@         Coverage Diff         @@
##           main   #407   +/-   ##
===================================
- Coverage    86%    86%   -1%     
===================================
  Files       109    109           
  Lines      9059   9071   +12     
  Branches   1087   1091    +4     
===================================
+ Hits       7826   7833    +7     
- Misses      982    985    +3     
- Partials    251    253    +2     
Flag Coverage Δ
unittests 86% <58%> (-1%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants