diff --git a/CHANGELOG.md b/CHANGELOG.md index e79f3ff00..a7c2bd07f 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index d9458ef8c..868b0ee6c 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/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)) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 834ebbf54..40a4360f5 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/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)