Simplify ImageCropper crop: drop the leaking MemoryStream round-trip

CHANGELOG.md

@@ -45,10 +45,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [SIL.Windows.Forms.Keyboarding] Removed Timer-based deferred IME conversion status restore from WindowsKeyboardSwitchingAdapter, which disrupted active Chinese Pinyin IME compositions (LT-22442). Added diagnostic tracing for keyboard switching and IME state.
 - [SIL.DictionaryServices] Fix memory leak in LiftWriter
 - [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs: `Application.Idle` handler was never unsubscribed, causing it to fire on a disposed object
-- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` returning a JPEG-format bitmap backed by a prematurely disposed `MemoryStream`
+- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` re-encoding the crop through a `MemoryStream` that had to outlive the returned bitmap; the stream could not be disposed and leaked. `GetCroppedImage` now returns the cropped bitmap directly and the caller chooses the save format (via the file extension), as `PalasoImage.Save` already does
 - [SIL.Windows.Forms] Fixed ImageCropper `Image` setter leaking the previous temp file and cropping image on re-set; fields are now nulled after disposal so a mid-setter failure does not leave disposed-but-non-null references
 - [SIL.Windows.Forms] Fixed ImageCropper not downscaling tall images before cropping (height condition was checking width)
-- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` throwing `NullReferenceException` when `Image` property is set directly rather than via `SetImage`
 - [SIL.WritingSystems] Fix IetfLanguageTag.GetGeneralCode to handle cases when zh-CN or zh-TW is a prefix and not the whole string.
 - [SIL.WritingSystems] More fixes to consistently use 繁体中文 and 简体中文 for Traditional and Simplified Chinese native language names, and Chinese (Traditional) and Chinese (Simplified) for their English names.
 - [SIL.Windows.Forms] Prevent BetterLabel from responding to OnTextChanged when it has been disposed.

SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs

@@ -48,7 +48,7 @@ public void GetCroppedImage_PngImage_ReturnsUsableBitmap()
 		}
 
 		[Test]
-		public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn()
+		public void GetCroppedImage_JpegImage_ReturnsUsableBitmap()
 		{
 			using (var tempFile = TempFile.WithExtension(".jpg"))
 			{
@@ -61,30 +61,25 @@ public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn()
 					cropper.Size = new Size(400, 300);
 					cropper.SetImage(palasoImage);
 
-					Image result;
-					// GetCroppedImage returns a Bitmap backed by a MemoryStream for JPEG.
-					// The stream must still be alive after the method returns.
-					result = cropper.GetCroppedImage();
-					try
+					using (var result = cropper.GetCroppedImage())
 					{
 						Assert.IsNotNull(result);
-						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
-						// Re-encode to force GDI+ to re-read the pixel data from the backing stream.
+						// The crop is a stand-alone in-memory bitmap, not backed by a file or stream, so
+						// it reports MemoryBmp format even for a JPEG source; the caller chooses the actual
+						// save format via the file extension.
+						Assert.AreEqual(ImageFormat.MemoryBmp.Guid, result.RawFormat.Guid);
+						// Re-encode to force GDI+ to re-read the pixel data from the backing store.
 						using (var ms = new MemoryStream())
 							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
 					}
-					finally
-					{
-						result?.Dispose();
-					}
 				}
 			}
 		}
+
 		[Test]
-		public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap()
+		public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsUsableBitmap()
 		{
-			// Setting Image directly (not via SetImage) must still initialize _originalFormat so
-			// GetCroppedImage does not throw NullReferenceException on JPEG re-encoding.
+			// Setting Image directly (not via SetImage) must still yield a usable crop.
 			using (var tempFile = TempFile.WithExtension(".jpg"))
 			{
 				using (var bmp = new Bitmap(100, 80))
@@ -99,15 +94,16 @@ public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap()
 					using (var result = cropper.GetCroppedImage())
 					{
 						Assert.IsNotNull(result);
-						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
 						Assert.Greater(result.Width, 0);
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
 					}
 				}
 			}
 		}
 
 		[Test]
-		public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow()
+		public void SetImage_Reassign_DoesNotThrow()
 		{
 			using (var tempFile1 = TempFile.WithExtension(".png"))
 			using (var tempFile2 = TempFile.WithExtension(".jpg"))
@@ -129,7 +125,7 @@ public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow()
 					using (var result = cropper.GetCroppedImage())
 					{
 						Assert.IsNotNull(result);
-						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+						Assert.Greater(result.Width, 0);
 					}
 				}
 			}
@@ -138,9 +134,10 @@ public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow()
 		[Test]
 		public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow()
 		{
-			// GetImage() returns a JPEG Bitmap backed by a MemoryStream; feeding that result
-			// back into a new cropper re-saves it in the Image setter (value.Image.Save), which
-			// crashed once the backing stream had been disposed.
+			// Regression test for issue #1275: cropping a JPEG, then feeding the result back into a
+			// new cropper (which re-saves it in the Image setter via value.Image.Save) crashed when
+			// the cropped bitmap was backed by a disposed stream. The crop is now a stand-alone
+			// bitmap, so the round-trip must not throw.
 			using (var tempFile = TempFile.WithExtension(".jpg"))
 			{
 				using (var bmp = new Bitmap(1200, 900))

SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs

@@ -4,7 +4,6 @@
 using System.Drawing.Imaging;
 using System.IO;
 using System.Windows.Forms;
-using SIL.Code;
 using SIL.IO;
 using SIL.Reporting;
 
@@ -30,7 +29,6 @@ public partial class ImageCropper : UserControl, IImageToolboxControl
 		private Point _startOfDrag = default(Point);
 
 		//we will be cropping the image, so we need to keep the original lest we be cropping the crop, so to speak
-		private ImageFormat _originalFormat;
 		private TempFile _savedOriginalImage;
 		private Image _croppingImage;
 
@@ -149,7 +147,6 @@ public PalasoImage Image
 				_savedOriginalImage = null;
 				_croppingImage?.Dispose();
 				_croppingImage = null;
-				_originalFormat = value.Image.RawFormat;
 
 				//other code changes the image of this palaso image, at which time the PI disposes of its copy,
 				//so we better keep our own.
@@ -424,31 +421,18 @@ public Image GetCroppedImage()
 						selectionHeight = originalImage.Height - top;
 					var selection = new Rectangle(left, top, selectionWidth, selectionHeight);
 
-					var cropped = originalImage.Clone(selection, originalImage.PixelFormat); //do the actual cropping
-
-					if (_originalFormat.Guid == ImageFormat.Jpeg.Guid)
+					using (var cropped = originalImage.Clone(selection, originalImage.PixelFormat)) //do the actual cropping
 					{
-						//We've sadly lost our jpeg formatting, so now we encode a new image in jpeg
-						var stream = new MemoryStream();
-						try
-						{
-							cropped.Save(stream, ImageFormat.Jpeg);
-							stream.Position = 0;
-							// Do not dispose stream on success: GDI+ bitmaps reference the stream for lazy
-							// decoding. The stream is collected when the returned Bitmap is disposed.
-							var oldCropped = cropped;
-							cropped = (Bitmap)System.Drawing.Image.FromStream(stream);
-							oldCropped.Dispose();
-						}
-						catch
-						{
-							stream.Dispose();
-							cropped.Dispose();
-							throw;
-						}
-						Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting");
+						// Return a stand-alone copy. We used to round-trip the crop through a MemoryStream
+						// to restore the JPEG RawFormat, but that stream had to outlive the returned bitmap
+						// (GDI+ decodes lazily), so it could not be disposed and leaked. Cloning from the
+						// file-backed originalImage likewise keeps a lazy reference to its source, which can
+						// leave the caller's temp file locked when the crop is later saved (as ImageCropper's
+						// own Image setter does). Copying into a fresh Bitmap severs both ties. Nothing reads
+						// the returned bitmap's RawFormat -- PalasoImage.Save picks the format from the file
+						// extension -- so the resulting MemoryBmp format is fine.
+						return new Bitmap(cropped);
 					}
-					return cropped;
 				}
 			}
 			catch (Exception e)