+semver:major Fix unusable ImageCropper crop; RawFormat now MemoryBmp by imnasnainaec · Pull Request #1530 · sillsdev/libpalaso

imnasnainaec · 2026-07-13T15:22:51Z

Warning

Breaking change. GetCroppedImage no longer returns a JPEG-encoded bitmap for JPEG sources. A real crop now has RawFormat MemoryBmp; a whole-image selection has Png, since the crop is read back from the cropper's PNG temp file. Callers that read RawFormat, or that call Image.Save(path) on the result and rely on GDI+ picking the JPEG encoder implicitly, must now pass an explicit ImageFormat — a .jpg path would otherwise receive PNG bytes. Going through PalasoImage.Save(path) is unaffected, since it picks the encoder from the file extension. ImageToolbox's own use goes through PalasoImage, so nothing in this repo changes behaviour.

GetImage is unaffected when nothing was cropped: it now returns the image untouched, so an uncropped JPEG stays a JPEG.

(.RawFormat is used in mono: https://github.com/search?q=org%3Asillsdev+.rawformat&type=code)

GetCroppedImage re-encodes a JPEG crop through a MemoryStream and returns what Image.FromStream produces. That stream sits in a using, so it is disposed before the bitmap is returned, and GDI+ decodes lazily — the crop refers to a stream that is already gone. The next thing to touch its pixels fails with "A generic error occurred in GDI+".

That is #1275: choose an image from a file, Crop, Choose, Crop. Leaving the Crop tab calls GetImage, which stores the stream-backed crop on the PalasoImage; returning re-enters the Image setter, which calls value.Image.Save(...) on that crop and throws. Only JPEG sources take the re-encode path, matching the report's "happens for most but not all".

The change

Return the cropped bitmap directly instead of the re-encoded one, so nothing holds a lazy reference to a stream. Preserving the JPEG RawFormat is what required the re-encode in the first place, so giving it up is what makes the fix possible — hence the breaking change above.

GetImage now skips the crop entirely when no grip has moved, returning the PalasoImage untouched. That is the common path — open the Crop tab, leave it — and it used to replace the image with a copy of itself round-tripped through the PNG temp file, costing the original's format and bit depth and leaving that temp file locked until the copy was disposed.

With the re-encode gone _originalFormat has no remaining use, so it goes too, along with the Require.That self-check, the SIL.Code import, and the comments describing the old workaround.

Removing _originalFormat fixes a second bug on the way. It was assigned only in SetImage, so assigning the Image property directly left it null, and GetCroppedImage threw a NullReferenceException reading _originalFormat.Guid — swallowed by the catch and surfaced as "Sorry, there was a problem getting the image".

ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged is deleted: it is [Explicit("By hand only")] so it never ran in CI, and it asserted exactly the JPEG RawFormat this change gives up.

Tests

Five of the added cases fail against master:

GetCroppedImage_JpegImage_ReturnsUsableBitmap(true) and (false) — the crop is backed by a disposed stream.
GetCroppedImage_ImageSetViaPropertyDirectly_ReturnsUsableBitmap — the null _originalFormat NRE.
GetImage_NothingCropped_ReturnsImageUntouched — the image comes back re-encoded.
GetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow — the Crash in ImageToolbox switching between Crop and Choose #1275 round trip.

GetImage_ResultOutlivesCropper_SavedOriginalTempFileIsDeleted pins the temp file against being held open by the result, and GetCroppedImage_PngImage_ReturnsUsableBitmap and GetCroppedImage_OneBitPng_PreservesPixelFormat cover what the change must not disturb.

--filter ImageToolbox is green on net8.0-windows and net48: 56 passed, 1 pre-existing skip.

Fixes #1275

Devin review: https://app.devin.ai/review/sillsdev/libpalaso/pull/1530

This change is

Return the cropped bitmap as a stand-alone copy instead of round-tripping through a MemoryStream that had to outlive it; unsubscribe the disposed Application.Idle handler; dispose and null out prior image state before re-assignment; and fix a height-vs-width typo that skipped downscaling of tall images. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-07-13T15:33:45Z

Palaso Tests

4 files ± 0 4 suites ±0 10m 9s ⏱️ -33s
5 132 tests + 8 4 900 ✅ + 9 232 💤 - 1 0 ❌ ±0
16 711 runs +24 15 994 ✅ +28 717 💤 - 4 0 ❌ ±0

Results for commit d5c58a1. ± Comparison against base commit dafc9bd.

This pull request removes 1 and adds 9 tests. Note that renamed tests count towards both.

SIL.Windows.Forms.Tests.ImageToolbox.ImageToolboxTests ‑ ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged

SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_ImageSetViaPropertyDirectly_ReturnsUsableBitmap
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_JpegImage_ReturnsUsableBitmap(False)
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_JpegImage_ReturnsUsableBitmap(True)
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_OneBitPng_PreservesPixelFormat
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_PngImage_ReturnsUsableBitmap
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetImage_NothingCropped_ReturnsImageUntouched
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetImage_ResultOutlivesCropper_SavedOriginalTempFileIsDeleted(False)
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetImage_ResultOutlivesCropper_SavedOriginalTempFileIsDeleted(True)

♻️ This comment has been updated with latest results.

ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged asserted that cropping preserves the original RawFormat, which no longer holds now that GetCroppedImage intentionally returns a stand-alone bitmap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ea fix (BL-1275) Strengthen the double-dispose and reassignment tests to actually exercise and verify cleanup (temp file and cropping image disposal) instead of only asserting no exception is thrown, and add a regression test for the height/width downscaling typo. Add the missing CHANGELOG entry for the null-guard in CalculateSourceImageArea, and trim stale comments in GetCroppedImage describing the removed JPEG re-encoding approach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The Image setter's dispose-and-null block and the CalculateSourceImageArea null guard it required are removed here, leaving this branch to the GetCroppedImage change alone.

imnasnainaec · 2026-08-27T13:27:09Z

@andrew-polk What's your take on the breaking change of this pr? (Don't worry about doing code review... not worthwhile unless and until we decide it's a worthwhile change.)

Master's #1536 hardened the ImageCropper.Image setter and Dispose against leaking/prematurely disposing image state; this branch removes _originalFormat and the JPEG re-encode from GetCroppedImage. Both land, with these conceptual resolutions where they overlap: - SetImage: master hoisted `var originalFormat = image.Image.RawFormat` above `Image = image` so a throwing setter couldn't leave _originalFormat describing the new image. The field no longer exists, so the guard is moot; SetImage is back to a plain `Image = image`. - ImageCropper.cs no longer needs `using System.IO`: this branch dropped the MemoryStream, master dropped the `catch (IOException)`. Neither alone orphaned it. - ImageCropperTests: kept all six tests. Dropped the comment in Image_NewImageFailsToLoad_LeavesPreviousImageStateIntact explaining that it assigns the property because SetImage would throw reading RawFormat first -- SetImage no longer reads RawFormat. The test still targets the setter, which is where the exception-safety contract lives. - CHANGELOG: both entries kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

andrew-polk · 2026-09-14T16:28:19Z

@imnasnainaec
Sorry; I never saw this until now. I've gotten into the habit of deleting notifications because I've been inundated with them...

Anyway, for Bloom, I don't think this affects us because 6.4 is the last version to use the Image Toolbox from libpalaso (we are replacing it with a React one in 6.5. And hopefully 6.4 will not be getting any more updates from libpalaso.

This bug is certainly real and we even track it, but decided not to fix it.
https://issues.bloomlibrary.org/youtrack/issue/BL-15019

But you should probably make the judgement about the breaking change based on Flex, not Bloom.

Happy to discuss further if helpful.

GetCroppedImage copied the crop into a new Bitmap to detach it from the source. That copy was unnecessary: Bitmap.Clone(rect, pixelFormat) already copies the pixels out, and the result survives both the source bitmap's disposal and deletion of the temp file behind it. The #1275 crash came from the old JPEG branch returning an Image.FromStream bitmap backed by a MemoryStream it then disposed, and that branch is already gone. The copy did have a cost. Bitmap(Image) renders into a 32bpp bitmap, so a cropped 1-bit PNG came back as Format32bppArgb, undoing the bit-depth preservation PalasoImage.SaveImageSafely goes out of its way to get (BL-2841). Returning the clone directly keeps the source pixel format. Dropping the copy also corrects the RawFormat we documented: the crop is taken from the PNG temp file, so it reports Png, not MemoryBmp. Updated the CHANGELOG and the JPEG test to match, and gave the entry the BREAKING CHANGE: prefix AGENTS.md asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

d69a46b rewrote the file with Python's utf-8-sig encoding, which strips a BOM on read but always writes one. CHANGELOG.md had none, so the commit added one and made the file's first line show up in every diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jasonleenaylor

From the FieldWorks side, we can take this. FLEx's only consumer of the toolbox is the Picture Properties dialog, which detects a crop by comparing image sizes and saves through PalasoImage.Save(path), so the encoder comes from the file extension and nothing reads RawFormat. FieldWorks is already on the 18.0.0 beta line, so the major bump costs us nothing extra. Today a FLEx user who crops a JPEG and re-enters the Crop tab gets the toolbox error dialog, and this fixes that.

One question inline about the comment and CHANGELOG wording.

This review was assisted by Claude Fable 5.1.

jasonleenaylor · 2026-09-15T23:26:21Z

+					// Clone already copies the pixels out, so the crop outlives originalImage and its
+					// temp file. Don't copy it into a new Bitmap to "detach" it: that widens a 1-bit
+					// PNG to 32bpp (BL-2841) and buys nothing.
+					return originalImage.Clone(selection, originalImage.PixelFormat);


It looks like Clone(selection, PixelFormat) only copies the pixels out when selection is smaller than the bitmap. When it is the full bounds, which is what unmoved grips produce (lines 432-435 snap to the edge), GDI+ returns a bitmap that still shares originalImage's file-backed data: RawFormat is Png, the temp file stays locked, and DisposeImageState swallows the IOException and leaks it. A real crop gives MemoryBmp, detached. I think the test GetCroppedImage_JpegImage_ReturnsUsableBitmap sees Png only because it never moves a grip.

If this is correct maybe update the comments, changelog wording and Test as follows:

This comment: "Clone copies the pixels out for a partial selection, so a real crop outlives originalImage and its temp file. A full-bounds clone still shares the file-backed source (RawFormat Png, temp file locked until it is disposed). Don't copy into a new Bitmap to detach it: that widens a 1-bit PNG to 32bpp (BL-2841)."

CHANGELOG: replace "Its RawFormat is therefore Png (the format of the temp file the cropper crops from) rather than Jpeg" with "Its RawFormat is no longer Jpeg: MemoryBmp for a crop, Png when the selection is the whole image".

Test: move a grip before asserting, or assert Is.Not.EqualTo(ImageFormat.Jpeg.Guid).

Non-crop is now a proper no-op.

Comments and tests updated.

Bitmap.Clone copies the pixels out only for a partial selection. Asked for the whole bitmap - what unmoved grips give us - GDI+ hands back one that still shares the source's file-backed data, so the crop kept _savedOriginalImage locked for as long as the caller held it. The delete in DisposeImageState then failed and the temp file was leaked; in a Debug build it also tripped the Debug.Fail in TempFile.Dispose, which is why GetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow was failing. CI missed it because it tests Release, where Debug.Fail compiles out and the IOException is swallowed. Copy the rows by hand for that case rather than going through new Bitmap(source), which would widen a 1-bit PNG to 32bpp (BL-2841). GetCroppedImage now reports MemoryBmp whatever the source format and whatever the selection, so the doc comment and CHANGELOG say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Replaces the hand-rolled CopyDetached from the previous commit. Leaving the Crop tab without having dragged a grip took the full cropping path anyway: decode the PNG temp file, clone the whole thing, assign the result back over the image we already had. That clone still shared the temp file, which is what leaked it, so the previous commit copied the pixels out row by row to get away from it. Not doing the work at all is better. GetImage now returns the PalasoImage untouched when every grip is still at its edge, so an uncropped image keeps its original format, bit depth and resolution instead of coming back round-tripped through a PNG, nothing reads _savedOriginalImage, and the leak is gone by construction. It also narrows the breaking change: only a real crop now reports MemoryBmp, and an uncropped JPEG stays Jpeg. GetCroppedImage keeps its old behaviour for direct callers, including the whole-image case that holds the temp file open; its doc comment says so, and that nothing in the toolbox asks for one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Drop the parts that described the implementation these commits replaced rather than the code that is there, and fold the two GetCroppedImage JPEG tests into one with a case per selection, so the pairing they were commenting on is expressed by the test cases instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

imnasnainaec self-assigned this Jul 13, 2026

This comment was marked as outdated.

Sign in to view

imnasnainaec marked this pull request as draft July 13, 2026 16:36

github-advanced-security Bot found potential problems Jul 13, 2026

View reviewed changes

Comment thread SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs Fixed

imnasnainaec mentioned this pull request Jul 13, 2026

Fix ImageCropper crash when switching Crop/Choose tabs #1521

Closed

1 task

This was referenced Aug 26, 2026

ImageCropper: dispose the state built for the previous image on re-set #1536

Merged

[SIL.Windows.Forms] Fix ImageCropper resource leaks and cropping correctness bugs #1528

Closed

Merge master; drop the leak fix and guard now handled elsewhere

912b4d8

The Image setter's dispose-and-null block and the CalculateSourceImageArea null guard it required are removed here, leaving this branch to the GetCroppedImage change alone.

imnasnainaec changed the title ~~Fix ImageCropper crash and resource leaks~~ Fix ImageCropper returning a crop backed by a disposed stream Aug 27, 2026

Call out the RawFormat change in the CHANGELOG as breaking

6e07840

imnasnainaec changed the title ~~Fix ImageCropper returning a crop backed by a disposed stream~~ Fix ImageCropper returning a crop backed by a disposed stream (breaking: crop is no longer JPEG) Aug 27, 2026

imnasnainaec changed the title ~~Fix ImageCropper returning a crop backed by a disposed stream (breaking: crop is no longer JPEG)~~ +semver:major Fix unusable ImageCropper crop; RawFormat now Png Sep 14, 2026

imnasnainaec and others added 3 commits September 14, 2026 16:33

Clean up the comments

1373ba2

Remove stale commented-out code

997f0e6

imnasnainaec marked this pull request as ready for review September 14, 2026 21:26

imnasnainaec requested a review from jasonleenaylor September 14, 2026 21:30

imnasnainaec added api change fix labels Sep 14, 2026

jasonleenaylor approved these changes Sep 15, 2026

View reviewed changes

imnasnainaec marked this pull request as draft September 16, 2026 17:57

imnasnainaec and others added 2 commits September 16, 2026 15:22

imnasnainaec marked this pull request as ready for review September 16, 2026 20:08

imnasnainaec requested a review from jasonleenaylor September 16, 2026 20:09

imnasnainaec changed the title ~~+semver:major Fix unusable ImageCropper crop; RawFormat now Png~~ +semver:major Fix unusable ImageCropper crop; RawFormat now MemoryBmp Sep 16, 2026

Navigation Menu

Uh oh!

Uh oh!

+semver:major Fix unusable ImageCropper crop; RawFormat now MemoryBmp - #1530

+semver:major Fix unusable ImageCropper crop; RawFormat now MemoryBmp#1530
imnasnainaec wants to merge 13 commits into
masterfrom
fix/1275-bitmap-instead-of-stream

imnasnainaec commented Jul 13, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 13, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

imnasnainaec commented Aug 27, 2026

Uh oh!

andrew-polk commented Sep 14, 2026

Uh oh!

jasonleenaylor left a comment

Uh oh!

jasonleenaylor Sep 15, 2026

Uh oh!

imnasnainaec Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

imnasnainaec commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The change

Tests

Uh oh!

github-actions Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Palaso Tests

Uh oh!

This comment was marked as outdated.

Uh oh!

imnasnainaec commented Aug 27, 2026

Uh oh!

andrew-polk commented Sep 14, 2026

Uh oh!

jasonleenaylor left a comment

Choose a reason for hiding this comment

Uh oh!

jasonleenaylor Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

imnasnainaec Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imnasnainaec commented Jul 13, 2026 •

edited

Loading

github-actions Bot commented Jul 13, 2026 •

edited

Loading