From 1dbde6db90f0fa53b4b6097c1a1a21a484b5cd8b Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 12:30:06 -0500 Subject: [PATCH 1/8] Show a preview for images too large for the browser to display (BL-16597) Chromium (and therefore WebView2) refuses to decode an image whose pixel count times 4 bytes overflows a signed 32-bit int, so the image chooser showed an empty preview pane for a 30000x23756 scan. Decoding at a reduced size does not help: the limit is tested against the image's natural size before any scaling. So when a picked file is over 40 megapixels, the localFilePreview endpoint now serves a downscaled JPEG instead of the original. WIC's DecodePixelWidth does the scaling as part of the decode, so a 713 megapixel JPEG costs about 2.5 seconds and under 100MB rather than the ~2.6GB a full decode would need. Any EXIF rotation is baked into the stand-in, since it is re-encoded without an EXIF block. The stand-in is generated once per picked file and reused, because the gallery asks for the same URL as both thumbnail and large preview; it is deleted when another file is picked or when the project's lifetime scope is disposed. pickLocalImageFile now also reports the original file's dimensions and byte count, so the gallery describes the file the user actually chose rather than the downscaled stand-in it is being shown. Tests cover reading dimensions (including the not-an-image fallback), the below-threshold "serve the original" case, and the downscaling itself. Co-Authored-By: Claude Opus 5 (1M context) --- .../image-gallery/ImageGalleryDialog.tsx | 12 +- .../web/controllers/ImageGalleryApi.cs | 300 +++++++++++++++++- .../web/controllers/ImageGalleryApiTests.cs | 126 ++++++++ 3 files changed, 433 insertions(+), 5 deletions(-) diff --git a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx index f6e2ad562702..24afaf390704 100644 --- a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx +++ b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx @@ -101,16 +101,24 @@ const ImageGalleryDialog: React.FunctionComponent<{ {}, ); if (!response) return undefined; - const { filePath, previewUrl } = response.data as { + const { filePath, previewUrl, width, height, size } = response.data as { filePath: string; previewUrl: string; + width: number; + height: number; + size: number; }; if (!filePath) return undefined; return { thumbnailUrl: previewUrl, reasonableSizeUrl: previewUrl, localPath: filePath, - size: 0, + // C# reports the original file's dimensions and byte count. These matter because + // previewUrl may serve a downscaled stand-in for an image too large for the + // browser to display, and we want to report what the user actually chose. + width: width || undefined, + height: height || undefined, + size: size ?? 0, type: "image", }; }; diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index 1b5482cd8fe9..c35a295da98c 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -7,6 +7,7 @@ using System.Text.RegularExpressions; using System.Threading.Tasks; using System.Windows.Forms; +using System.Windows.Media.Imaging; using Bloom.Api; using Bloom.Book; using Bloom.Edit; @@ -15,6 +16,7 @@ using Bloom.Utils; using SIL.Core.ClearShare; using SIL.IO; +using SIL.Reporting; using SIL.Windows.Forms.ClearShare; using SIL.Windows.Forms.ImageToolbox; @@ -24,7 +26,7 @@ namespace Bloom.web.controllers /// Api for the image gallery (image chooser dialog) — local collections, file picker, /// remote search results, and saving chosen images into the current book. /// - public class ImageGalleryApi + public class ImageGalleryApi : IDisposable { public EditingView View { get; set; } @@ -37,6 +39,42 @@ public class ImageGalleryApi /// private string _lastPickedLocalImagePath; + /// + /// A temporary downscaled JPEG standing in for _lastPickedLocalImagePath when the + /// original is too big for the browser to display (see MakeBrowserSafePreview). + /// Null when the original is served as-is. Deleted when another file is picked. + /// + private string _lastPickedLocalImagePreviewPath; + + /// + /// Guards _lastPickedLocalImagePreviewPath. The gallery uses the preview URL as both the + /// thumbnail and the large preview, so two requests for it typically arrive at once; + /// without this they would each spend seconds building a preview and one would leak. + /// + private readonly object _previewLock = new object(); + + /// + /// Chromium — and therefore WebView2 — flatly refuses to decode an image whose pixel + /// count times 4 bytes/pixel overflows a signed 32-bit int, i.e. anything larger than + /// about 536.9 megapixels. The <img> fires "error" a fraction of a second after + /// the bytes arrive and nothing paints, which is why the image chooser showed an empty + /// preview for a 30000x23756 scan (BL-16597). Decoding at a reduced size does not help: + /// createImageBitmap() with resizeWidth and the WebCodecs ImageDecoder with desiredWidth + /// both fail identically, because the limit is tested against the image's natural size + /// before any scaling is applied. + /// + /// Well below that hard ceiling, handing the renderer a hundred-megapixel image still + /// costs it seconds of decode time and gigabytes of RAM, all to fill a preview pane a + /// few hundred pixels tall. So past this threshold we substitute a downscaled JPEG. + /// + internal const long kMaxPreviewPixels = 40L * 1000 * 1000; + + /// + /// Longest side, in pixels, of a generated preview. The gallery's preview pane caps the + /// image at 420px tall, so this leaves plenty of room for high-DPI screens. + /// + internal const int kPreviewMaxDimension = 1600; + /// /// The root folder where SIL image collections (including Art of Reading) are installed. /// @@ -367,12 +405,254 @@ private void HandlePickLocalImageFile(ApiRequest request) ) ); + // Drop the previous pick's downscaled stand-in *before* authorizing the new path, + // so there is no instant in which a preview request for the new file would find, + // and serve, the previous file's stand-in. + DeleteLastPickedImagePreview(); _lastPickedLocalImagePath = selectedPath; + var previewUrl = string.IsNullOrEmpty(selectedPath) ? "" : "/bloom/api/imageGallery/localFilePreview?path=" + Uri.EscapeDataString(selectedPath); - request.ReplyWithJson(new { filePath = selectedPath, previewUrl }); + + // Report the *original* file's dimensions and byte count. The gallery displays + // these, and without them it would fall back to measuring whatever the preview + // turns out to be — which for a huge image is the downscaled stand-in, not the + // file the user actually chose. + var (width, height) = GetImageDimensions(selectedPath); + long size = 0; + if (!string.IsNullOrEmpty(selectedPath) && RobustFile.Exists(selectedPath)) + { + try + { + size = new FileInfo(selectedPath).Length; + } + catch (Exception e) + { + Logger.WriteMinorEvent( + $"ImageGalleryApi could not read the size of {selectedPath}: {e.Message}" + ); + } + } + + request.ReplyWithJson( + new + { + filePath = selectedPath, + previewUrl, + width, + height, + size, + } + ); + } + + /// + /// Reads an image's pixel dimensions without decoding its pixels. Returns (0,0) if the + /// dimensions can't be determined (e.g. an SVG, or a format WIC doesn't know). + /// + internal static (int width, int height) GetImageDimensions(string path) + { + if (string.IsNullOrEmpty(path) || !RobustFile.Exists(path)) + return (0, 0); + try + { + var header = ReadImageHeader(path); + // Report the dimensions as they will actually be seen, i.e. after the viewer + // applies any EXIF rotation. + return IsQuarterTurned(header.exifOrientation) + ? (header.height, header.width) + : (header.width, header.height); + } + catch (Exception e) + { + Logger.WriteMinorEvent( + $"ImageGalleryApi could not read the dimensions of {path}: {e.Message}" + ); + return (0, 0); + } + } + + /// + /// Reads an image's dimensions and EXIF orientation without decoding its pixels. WIC + /// parses the header up front but reads everything else lazily, so all the values we + /// want must be pulled out while the stream is still open — hence one method rather + /// than handing a BitmapFrame back to the caller. + /// + private static (int width, int height, int exifOrientation) ReadImageHeader(string path) + { + using var stream = RobustFile.OpenRead(path); + var decoder = BitmapDecoder.Create( + stream, + BitmapCreateOptions.IgnoreColorProfile | BitmapCreateOptions.DelayCreation, + BitmapCacheOption.None + ); + var frame = decoder.Frames[0]; + var exifOrientation = 1; + try + { + // 274 (0x112) is the EXIF "Orientation" tag. + if ( + frame.Metadata is BitmapMetadata metadata + && metadata.GetQuery("/app1/ifd/{ushort=274}") is ushort orientation + && orientation >= 1 + && orientation <= 8 + ) + exifOrientation = orientation; + } + catch (Exception) + { + // Plenty of images carry no EXIF block at all; the default is what we want. + } + return (frame.PixelWidth, frame.PixelHeight, exifOrientation); + } + + /// Whether the given EXIF orientation swaps width and height. + private static bool IsQuarterTurned(int exifOrientation) => exifOrientation >= 5; + + /// + /// The rotation/flip an EXIF orientation calls for, or null when none is needed. + /// + private static System.Windows.Media.Transform GetOrientationTransform(int exifOrientation) + { + switch (exifOrientation) + { + case 2: + return new System.Windows.Media.ScaleTransform(-1, 1); + case 3: + return new System.Windows.Media.RotateTransform(180); + case 4: + return new System.Windows.Media.ScaleTransform(1, -1); + case 5: + var transpose = new System.Windows.Media.TransformGroup(); + transpose.Children.Add(new System.Windows.Media.RotateTransform(90)); + transpose.Children.Add(new System.Windows.Media.ScaleTransform(-1, 1)); + return transpose; + case 6: + return new System.Windows.Media.RotateTransform(90); + case 7: + var transverse = new System.Windows.Media.TransformGroup(); + transverse.Children.Add(new System.Windows.Media.RotateTransform(270)); + transverse.Children.Add(new System.Windows.Media.ScaleTransform(-1, 1)); + return transverse; + case 8: + return new System.Windows.Media.RotateTransform(270); + default: + return null; + } + } + + /// + /// If is too large for the browser to display (see + /// kMaxPreviewPixels), writes a downscaled JPEG to a temp file and returns its path; + /// otherwise returns null, meaning "serve the original". + /// + /// WIC's DecodePixelWidth/Height does the scaling as part of the decode, so a 713 + /// megapixel JPEG costs about 2.5 seconds and under 100MB rather than the ~2.6GB a + /// full decode would need. + /// + internal static string MakeBrowserSafePreview(string originalPath) + { + try + { + var header = ReadImageHeader(originalPath); + long pixels = (long)header.width * header.height; + if (pixels <= kMaxPreviewPixels) + return null; + + // Constrain the longer side; WIC preserves the aspect ratio when only one of + // DecodePixelWidth/DecodePixelHeight is set, and does the scaling as part of + // the decode rather than decoding full size and shrinking afterwards. + // A stream rather than a UriSource, because Uri would treat a "#" in a + // perfectly legal filename as the start of a fragment. CacheOption.OnLoad + // makes EndInit() do the decoding, so the stream can be closed right after. + var scaled = new BitmapImage(); + using (var stream = RobustFile.OpenRead(originalPath)) + { + scaled.BeginInit(); + scaled.StreamSource = stream; + scaled.CacheOption = BitmapCacheOption.OnLoad; + scaled.CreateOptions = BitmapCreateOptions.IgnoreColorProfile; + if (header.width >= header.height) + scaled.DecodePixelWidth = kPreviewMaxDimension; + else + scaled.DecodePixelHeight = kPreviewMaxDimension; + scaled.EndInit(); + } + scaled.Freeze(); + + // Bake in any EXIF rotation. We re-encode without an EXIF block, so a viewer + // has no orientation tag to apply and the pixels must already be upright. + BitmapSource upright = scaled; + var transform = GetOrientationTransform(header.exifOrientation); + if (transform != null) + { + var rotated = new TransformedBitmap(scaled, transform); + rotated.Freeze(); + upright = rotated; + } + + // A plain temp path rather than SIL's TempFile, whose Dispose would delete the + // file we are about to start serving. DeleteLastPickedImagePreview cleans up. + var previewPath = Path.Combine( + Path.GetTempPath(), + "BloomImagePreview-" + Guid.NewGuid() + ".jpg" + ); + var encoder = new JpegBitmapEncoder { QualityLevel = 85 }; + encoder.Frames.Add(BitmapFrame.Create(upright)); + using (var output = RobustFile.Create(previewPath)) + encoder.Save(output); + + Logger.WriteMinorEvent( + $"ImageGalleryApi made a {upright.PixelWidth}x{upright.PixelHeight} preview" + + $" for {Path.GetFileName(originalPath)}" + + $" ({header.width}x{header.height}, too large for the browser)" + ); + return previewPath; + } + catch (Exception e) + { + // Includes SVG and anything else WIC can't open. Falling back to the original + // is right: those are the formats the browser handles fine anyway. + Logger.WriteMinorEvent( + $"ImageGalleryApi could not make a downscaled preview for {originalPath}: {e.Message}" + ); + return null; + } + } + + /// Deletes the cached downscaled preview, if there is one. + private void DeleteLastPickedImagePreview() + { + lock (_previewLock) + { + if (string.IsNullOrEmpty(_lastPickedLocalImagePreviewPath)) + return; + try + { + RobustFile.Delete(_lastPickedLocalImagePreviewPath); + } + catch (Exception e) + { + // Nothing to do about it; it's a temp file, and it is named + // recognisably enough to be swept up later. + Logger.WriteMinorEvent( + $"ImageGalleryApi could not delete {_lastPickedLocalImagePreviewPath}: {e.Message}" + ); + } + _lastPickedLocalImagePreviewPath = null; + } + } + + /// + /// Picking a new file removes the previous file's stand-in, so at most one is ever + /// left over — the one for the last file picked. This is where that one goes; Autofac + /// disposes us with the project's lifetime scope. + /// + public void Dispose() + { + DeleteLastPickedImagePreview(); } /// @@ -397,7 +677,21 @@ private void HandleLocalFilePreview(ApiRequest request) return; } - request.ReplyWithImage(fullPath); + // Images past a certain size don't render in the browser at all, so substitute a + // downscaled copy (BL-16597). Generated once and reused, since the gallery asks + // for this URL as both the thumbnail and the large preview. + string pathToServe; + lock (_previewLock) + { + if ( + _lastPickedLocalImagePreviewPath == null + || !RobustFile.Exists(_lastPickedLocalImagePreviewPath) + ) + _lastPickedLocalImagePreviewPath = MakeBrowserSafePreview(fullPath); + pathToServe = _lastPickedLocalImagePreviewPath ?? fullPath; + } + + request.ReplyWithImage(pathToServe); } /// diff --git a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs index 0298bb259021..b856fba85957 100644 --- a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs +++ b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs @@ -1,3 +1,5 @@ +using System.Drawing; +using System.Drawing.Imaging; using System.IO; using Bloom.web.controllers; using NUnit.Framework; @@ -88,5 +90,129 @@ public void HandlesMultiWordGrantor() Assert.That(credits, Is.EqualTo("Acme Publishing House")); } } + + /// + /// Covers the machinery behind BL-16597: reporting the chosen file's real dimensions, + /// and substituting a downscaled JPEG for an image the browser cannot display. + /// + [TestFixture] + public class LargeImagePreviewTests + { + private string _tempFolder; + + [SetUp] + public void Setup() + { + _tempFolder = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Directory.CreateDirectory(_tempFolder); + } + + [TearDown] + public void TearDown() + { + Directory.Delete(_tempFolder, recursive: true); + } + + /// + /// Writes a blank PNG of the given pixel size. 1-bit-per-pixel keeps even a + /// deliberately enormous test image down to a few megabytes of memory and a + /// trivially small file, so the over-the-threshold cases are cheap to run. + /// + private string WritePng(string name, int width, int height) + { + var path = Path.Combine(_tempFolder, name); + using (var bitmap = new Bitmap(width, height, PixelFormat.Format1bppIndexed)) + bitmap.Save(path, ImageFormat.Png); + return path; + } + + [Test] + public void GetImageDimensions_ReturnsThePixelDimensions() + { + var path = WritePng("small.png", 640, 480); + + var (width, height) = ImageGalleryApi.GetImageDimensions(path); + + Assert.That(width, Is.EqualTo(640)); + Assert.That(height, Is.EqualTo(480)); + } + + [Test] + public void GetImageDimensions_ReturnsZeros_ForMissingFile() + { + var path = Path.Combine(_tempFolder, "nothing-here.png"); + Assert.That(File.Exists(path), Is.False, "Test setup: the file must not exist"); + + Assert.That(ImageGalleryApi.GetImageDimensions(path), Is.EqualTo((0, 0))); + } + + [Test] + public void GetImageDimensions_ReturnsZeros_ForSomethingThatIsNotAnImage() + { + // An SVG is the realistic case: the chooser allows it, but WIC cannot read it. + var path = Path.Combine(_tempFolder, "drawing.svg"); + File.WriteAllText(path, ""); + + Assert.That(ImageGalleryApi.GetImageDimensions(path), Is.EqualTo((0, 0))); + } + + [Test] + public void MakeBrowserSafePreview_ReturnsNull_WhenTheBrowserCanCopeWithTheOriginal() + { + var path = WritePng("modest.png", 2000, 1500); + Assert.That( + 2000L * 1500, + Is.LessThan(ImageGalleryApi.kMaxPreviewPixels), + "Test setup: this image is supposed to be under the threshold" + ); + + // Null means "serve the original". + Assert.That(ImageGalleryApi.MakeBrowserSafePreview(path), Is.Null); + } + + [Test] + public void MakeBrowserSafePreview_DownscalesAnImageTooBigForTheBrowser() + { + const int originalWidth = 8000; + const int originalHeight = 6000; + Assert.That( + (long)originalWidth * originalHeight, + Is.GreaterThan(ImageGalleryApi.kMaxPreviewPixels), + "Test setup: this image is supposed to be over the threshold" + ); + var path = WritePng("enormous.png", originalWidth, originalHeight); + + var previewPath = ImageGalleryApi.MakeBrowserSafePreview(path); + + Assert.That(previewPath, Is.Not.Null, "Should have made a downscaled stand-in"); + try + { + Assert.That(File.Exists(previewPath), Is.True); + using var preview = Image.FromFile(previewPath); + // The longer side is constrained; the aspect ratio is preserved. + Assert.That(preview.Width, Is.EqualTo(ImageGalleryApi.kPreviewMaxDimension)); + Assert.That( + preview.Height, + Is.EqualTo( + ImageGalleryApi.kPreviewMaxDimension * originalHeight / originalWidth + ) + ); + } + finally + { + File.Delete(previewPath); + } + } + + [Test] + public void MakeBrowserSafePreview_ReturnsNull_ForAFormatItCannotRead() + { + var path = Path.Combine(_tempFolder, "drawing.svg"); + File.WriteAllText(path, ""); + + // Falling back to the original is right: the browser handles SVG fine. + Assert.That(ImageGalleryApi.MakeBrowserSafePreview(path), Is.Null); + } + } } } From 627f9a9c572106d1720697e2bd85ee122e3848f2 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 12:41:05 -0500 Subject: [PATCH 2/8] Read EXIF orientation from TIFFs too, and correct a comment (BL-16597) The image chooser accepts .tif/.tiff, but the orientation tag was only looked for at the JPEG APP1 query path, so a rotated TIFF was treated as upright and previewed sideways. Try the TIFF top-level IFD path as well. Also: the comment in HandleLocalFilePreview claimed the stand-in is "generated once and reused". That is true of the stand-in, but for an image small enough to serve as-is there is nothing to cache, so the (cheap) header read does repeat per request. Say what actually happens. Both noticed by Devin (informational flags on PR #8111). Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/ImageGalleryApi.cs | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index c35a295da98c..ad1173684112 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -489,23 +489,42 @@ private static (int width, int height, int exifOrientation) ReadImageHeader(stri BitmapCacheOption.None ); var frame = decoder.Frames[0]; - var exifOrientation = 1; - try - { - // 274 (0x112) is the EXIF "Orientation" tag. - if ( - frame.Metadata is BitmapMetadata metadata - && metadata.GetQuery("/app1/ifd/{ushort=274}") is ushort orientation - && orientation >= 1 - && orientation <= 8 - ) - exifOrientation = orientation; - } - catch (Exception) + return ( + frame.PixelWidth, + frame.PixelHeight, + ReadExifOrientation(frame.Metadata as BitmapMetadata) + ); + } + + /// + /// The EXIF orientation recorded in the given metadata, or 1 ("upright") if there is + /// none. The chooser accepts TIFFs as well as JPEGs, and the tag lives at a different + /// query path in each, so both are tried. + /// + private static int ReadExifOrientation(BitmapMetadata metadata) + { + if (metadata == null) + return 1; + // 274 (0x112) is the EXIF "Orientation" tag. In a JPEG it hangs off the APP1 + // marker segment; in a TIFF it is in the top-level IFD. + foreach (var query in new[] { "/app1/ifd/{ushort=274}", "/ifd/{ushort=274}" }) { - // Plenty of images carry no EXIF block at all; the default is what we want. + try + { + if ( + metadata.GetQuery(query) is ushort orientation + && orientation >= 1 + && orientation <= 8 + ) + return orientation; + } + catch (Exception) + { + // Plenty of images carry no EXIF block at all, and asking a decoder for a + // query path its format doesn't have is how we find that out. + } } - return (frame.PixelWidth, frame.PixelHeight, exifOrientation); + return 1; } /// Whether the given EXIF orientation swaps width and height. @@ -678,8 +697,9 @@ private void HandleLocalFilePreview(ApiRequest request) } // Images past a certain size don't render in the browser at all, so substitute a - // downscaled copy (BL-16597). Generated once and reused, since the gallery asks - // for this URL as both the thumbnail and the large preview. + // downscaled copy (BL-16597). Once made, a stand-in is reused, since the gallery + // asks for this URL as both the thumbnail and the large preview. For an image + // that doesn't need one, all this repeats is the header read, which is cheap. string pathToServe; lock (_previewLock) { From ad578434afce86cd0325a162da6c00fd4a49d849 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 12:49:34 -0500 Subject: [PATCH 3/8] Key the preview cache to the file it was made from (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cached downscaled stand-in was keyed only on "one exists", so nothing in HandleLocalFilePreview itself established that it belonged to the file being requested; that was an invariant maintained at a distance, by the order in which HandlePickLocalImageFile cleared the cache and reassigned the authorized path. Unreachable in practice — picking a file means working a modal dialog, and the front-end only asks for the preview URL after the pick replies — but it is cheaper to make the cache say what it is about than to keep the reasoning. Remembering the source file also lets us remember "this one needs no stand-in", so an image small enough to serve as-is is no longer re-examined on every request. Devin raised this on PR #8111; the suggested hardening is what is implemented here. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/ImageGalleryApi.cs | 70 ++++++++++++------- 1 file changed, 46 insertions(+), 24 deletions(-) diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index ad1173684112..65f115da9ce8 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -47,7 +47,17 @@ public class ImageGalleryApi : IDisposable private string _lastPickedLocalImagePreviewPath; /// - /// Guards _lastPickedLocalImagePreviewPath. The gallery uses the preview URL as both the + /// The file _lastPickedLocalImagePreviewPath was made from, or null if we have not yet + /// looked at any file. This is what the cache is keyed on, so that a stand-in can only + /// ever be served for the file it was made from — rather than that being an invariant + /// maintained by the order in which HandlePickLocalImageFile does things. + /// It is also how we remember that a file needs no stand-in, which is why it is set + /// even in that case: without it we would re-examine such a file on every request. + /// + private string _lastPickedLocalImagePreviewSourcePath; + + /// + /// Guards the two fields above. The gallery uses the preview URL as both the /// thumbnail and the large preview, so two requests for it typically arrive at once; /// without this they would each spend seconds building a preview and one would leak. /// @@ -405,9 +415,9 @@ private void HandlePickLocalImageFile(ApiRequest request) ) ); - // Drop the previous pick's downscaled stand-in *before* authorizing the new path, - // so there is no instant in which a preview request for the new file would find, - // and serve, the previous file's stand-in. + // The previous pick's downscaled stand-in (if any) is now unreachable. Serving the + // wrong image is prevented by the cache's key, not by this; the point here is just + // to not leave the temp file lying around until the next pick. DeleteLastPickedImagePreview(); _lastPickedLocalImagePath = selectedPath; @@ -641,26 +651,30 @@ internal static string MakeBrowserSafePreview(string originalPath) } } - /// Deletes the cached downscaled preview, if there is one. + /// + /// Empties the preview cache, deleting the downscaled stand-in if there is one. + /// private void DeleteLastPickedImagePreview() { lock (_previewLock) { - if (string.IsNullOrEmpty(_lastPickedLocalImagePreviewPath)) - return; - try - { - RobustFile.Delete(_lastPickedLocalImagePreviewPath); - } - catch (Exception e) + if (!string.IsNullOrEmpty(_lastPickedLocalImagePreviewPath)) { - // Nothing to do about it; it's a temp file, and it is named - // recognisably enough to be swept up later. - Logger.WriteMinorEvent( - $"ImageGalleryApi could not delete {_lastPickedLocalImagePreviewPath}: {e.Message}" - ); + try + { + RobustFile.Delete(_lastPickedLocalImagePreviewPath); + } + catch (Exception e) + { + // Nothing to do about it; it's a temp file, and it is named + // recognisably enough to be swept up later. + Logger.WriteMinorEvent( + $"ImageGalleryApi could not delete {_lastPickedLocalImagePreviewPath}: {e.Message}" + ); + } } _lastPickedLocalImagePreviewPath = null; + _lastPickedLocalImagePreviewSourcePath = null; } } @@ -697,17 +711,25 @@ private void HandleLocalFilePreview(ApiRequest request) } // Images past a certain size don't render in the browser at all, so substitute a - // downscaled copy (BL-16597). Once made, a stand-in is reused, since the gallery - // asks for this URL as both the thumbnail and the large preview. For an image - // that doesn't need one, all this repeats is the header read, which is cheap. + // downscaled copy (BL-16597). What we worked out about a file is remembered and + // reused, since the gallery asks for this URL as both the thumbnail and the large + // preview — including the "this one is fine as it is" answer, which costs a header + // read to reach. string pathToServe; lock (_previewLock) { - if ( - _lastPickedLocalImagePreviewPath == null - || !RobustFile.Exists(_lastPickedLocalImagePreviewPath) - ) + var answerIsAboutThisFile = + _lastPickedLocalImagePreviewSourcePath == fullPath + && ( + _lastPickedLocalImagePreviewPath == null + || RobustFile.Exists(_lastPickedLocalImagePreviewPath) + ); + if (!answerIsAboutThisFile) + { + DeleteLastPickedImagePreview(); _lastPickedLocalImagePreviewPath = MakeBrowserSafePreview(fullPath); + _lastPickedLocalImagePreviewSourcePath = fullPath; + } pathToServe = _lastPickedLocalImagePreviewPath ?? fullPath; } From 4b05e72ff565bff9521e17e9fbe2af10566cefba Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 12:57:59 -0500 Subject: [PATCH 4/8] Pin down that preview generation works off the UI thread (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the only place in Bloom that uses WPF's WIC-backed imaging, and the API server calls it from a thread-pool thread. That is fine as long as the bitmaps are frozen, which MakeBrowserSafePreview does — but NUnit runs this project's tests on an STA thread, so nothing here actually exercised the apartment the real caller uses. Run it on an MTA thread and check the result. Raised by Devin on PR #8111, which asked for a runtime sanity check; a test seemed better than checking once by hand. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/ImageGalleryApiTests.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs index b856fba85957..425939d01341 100644 --- a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs +++ b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs @@ -1,6 +1,8 @@ +using System; using System.Drawing; using System.Drawing.Imaging; using System.IO; +using System.Threading; using Bloom.web.controllers; using NUnit.Framework; @@ -204,6 +206,54 @@ public void MakeBrowserSafePreview_DownscalesAnImageTooBigForTheBrowser() } } + [Test] + public void MakeBrowserSafePreview_WorksOnAnMtaBackgroundThread() + { + // This is the only place in Bloom that uses WPF's WIC-backed imaging, and the + // API server calls it from a thread-pool thread — not the STA thread NUnit + // runs tests on (see the Apartment attribute in BloomTests.csproj). WPF + // imaging is fine off the UI thread as long as the bitmaps are frozen, which + // MakeBrowserSafePreview does; this pins that down where it would otherwise + // only be true by inspection. + var path = WritePng("enormous-mta.png", 8000, 6000); + string previewPath = null; + Exception failure = null; + var thread = new Thread(() => + { + try + { + previewPath = ImageGalleryApi.MakeBrowserSafePreview(path); + } + catch (Exception e) + { + failure = e; + } + }); + thread.SetApartmentState(ApartmentState.MTA); + thread.Start(); + + Assert.That( + thread.Join(TimeSpan.FromMinutes(1)), + Is.True, + "Preview generation did not finish" + ); + Assert.That(failure, Is.Null, $"Preview generation threw: {failure}"); + Assert.That(previewPath, Is.Not.Null, "Should have made a downscaled stand-in"); + try + { + using var preview = Image.FromFile(previewPath); + Assert.That( + preview.Width, + Is.EqualTo(ImageGalleryApi.kPreviewMaxDimension), + "The stand-in made off the UI thread should be scaled like any other" + ); + } + finally + { + File.Delete(previewPath); + } + } + [Test] public void MakeBrowserSafePreview_ReturnsNull_ForAFormatItCannotRead() { From 97b1483ec851c0a23094fca3f4df8f29a6ae9eef Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 13:33:35 -0500 Subject: [PATCH 5/8] Preview TIFFs, and don't turn transparency black (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the reviewers turned up, both decided by John. Substituting a stand-in was keyed on size alone, but size is only one of the two reasons a browser won't display a picked file. The chooser's filter accepts .tif/.tiff and no browser will draw one, so a TIFF showed the same empty pane this card is about — and, once big TIFFs started being converted on their way past the size threshold, whether a TIFF previewed depended on how many pixels it had, which is not explicable to anyone. Bloom already applies the rule "convert what browsers can't draw so they can draw it" when importing: a TIFF lands in the book folder as a PNG. This is the same rule, applied to the preview. Matched on extension rather than on the bytes, deliberately: ReplyWithImage takes the response's content type from the extension, so the extension is what the browser goes on. An image already smaller than kPreviewMaxDimension is no longer enlarged, since a stand-in made only because of format has no reason to grow. A stand-in is a JPEG, which has no alpha channel, so a picture with see-through areas previewed with them solid black — whatever sits in the colour channels under a transparent pixel. Composite onto white first. Done unconditionally rather than only for images carrying transparency: deciding that reliably means enumerating the pixel formats that can hold alpha and checking indexed palettes for a transparent entry, against one pass over an image already capped at 1600px. It is pixel arithmetic rather than the WPF rendering stack, so it stays safe on the API server's threads. The transparency test was confirmed to fail without the fix (corner pixel came out 0,0,0), so it is testing what it claims to. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/ImageGalleryApi.cs | 122 ++++++++++++++++-- .../web/controllers/ImageGalleryApiTests.cs | 112 ++++++++++++++++ 2 files changed, 220 insertions(+), 14 deletions(-) diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index 65f115da9ce8..eebdd72dc812 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -40,8 +40,8 @@ public class ImageGalleryApi : IDisposable private string _lastPickedLocalImagePath; /// - /// A temporary downscaled JPEG standing in for _lastPickedLocalImagePath when the - /// original is too big for the browser to display (see MakeBrowserSafePreview). + /// A temporary JPEG standing in for _lastPickedLocalImagePath when the browser could + /// not display the original itself (see MakeBrowserSafePreview). /// Null when the original is served as-is. Deleted when another file is picked. /// private string _lastPickedLocalImagePreviewPath; @@ -76,6 +76,7 @@ public class ImageGalleryApi : IDisposable /// Well below that hard ceiling, handing the renderer a hundred-megapixel image still /// costs it seconds of decode time and gigabytes of RAM, all to fill a preview pane a /// few hundred pixels tall. So past this threshold we substitute a downscaled JPEG. + /// (Size is only one of the two reasons for substituting one — see NeedsStandIn.) /// internal const long kMaxPreviewPixels = 40L * 1000 * 1000; @@ -573,9 +574,98 @@ private static System.Windows.Media.Transform GetOrientationTransform(int exifOr } /// - /// If is too large for the browser to display (see - /// kMaxPreviewPixels), writes a downscaled JPEG to a temp file and returns its path; - /// otherwise returns null, meaning "serve the original". + /// Lays the image over a white background, so that anything see-through in it becomes + /// white rather than whatever happens to be in the RGB channels underneath. + /// + /// A stand-in is encoded as JPEG, which has no alpha channel: without this, a large + /// image with transparent areas — a line-art scan on transparency, say — previews with + /// those areas solid black, because that is what is stored under a fully transparent + /// pixel. White is what the chooser and the page behind it both use. + /// + /// Done unconditionally rather than only for images that carry transparency, because + /// deciding that reliably means enumerating the pixel formats that can hold an alpha + /// channel *and* checking indexed palettes for a transparent entry, whereas this is one + /// pass over an image already capped at kPreviewMaxDimension — milliseconds against the + /// seconds the decode itself takes. Pure pixel arithmetic, so unlike the WPF rendering + /// stack it is safe on whatever thread the API server hands us. + /// + private static BitmapSource FlattenOntoWhite(BitmapSource source) + { + // Bgra32 is straight (non-premultiplied) alpha, which is what the blend below + // assumes; Pbgra32 would already have the colour scaled by the alpha. + var bgra = new FormatConvertedBitmap( + source, + System.Windows.Media.PixelFormats.Bgra32, + null, + 0 + ); + int width = bgra.PixelWidth, + height = bgra.PixelHeight; + int stride = width * 4; + var pixels = new byte[(long)height * stride]; + bgra.CopyPixels(pixels, stride, 0); + for (int i = 0; i < pixels.Length; i += 4) + { + byte alpha = pixels[i + 3]; + if (alpha == 255) + continue; + // result = colour*alpha + white*(1-alpha), in 0..255 integer arithmetic. + int inverse = 255 - alpha; + pixels[i] = (byte)((pixels[i] * alpha + 255 * inverse) / 255); + pixels[i + 1] = (byte)((pixels[i + 1] * alpha + 255 * inverse) / 255); + pixels[i + 2] = (byte)((pixels[i + 2] * alpha + 255 * inverse) / 255); + pixels[i + 3] = 255; + } + var flattened = BitmapSource.Create( + width, + height, + 96, + 96, + System.Windows.Media.PixelFormats.Bgra32, + null, + pixels, + stride + ); + flattened.Freeze(); + return flattened; + } + + /// + /// File types the browser can put in an <img>. The chooser also offers .tif/.tiff, + /// which it cannot, so those get a stand-in (see NeedsStandIn). Matched on extension + /// rather than on the actual bytes deliberately: ReplyWithImage derives the response's + /// content type from the extension, so the extension is what the browser goes on. + /// + private static readonly HashSet s_browserRenderableExtensions = new HashSet( + StringComparer.OrdinalIgnoreCase + ) + { + ".jpg", + ".jpeg", + ".png", + ".gif", + ".bmp", + ".svg", + ".webp", + }; + + /// + /// Whether the browser would fail to display this file as it stands, so that we should + /// serve it a re-encoded stand-in instead. Two separate reasons: the file is a format + /// the browser cannot draw at all, or it is so large the browser refuses to decode it + /// (see kMaxPreviewPixels). + /// + private static bool NeedsStandIn(string path, int width, int height) + { + if (!s_browserRenderableExtensions.Contains(Path.GetExtension(path))) + return true; + return (long)width * height > kMaxPreviewPixels; + } + + /// + /// If the browser could not display as it stands — too + /// many pixels, or a format it cannot draw — writes a JPEG stand-in to a temp file and + /// returns its path. Otherwise returns null, meaning "serve the original". /// /// WIC's DecodePixelWidth/Height does the scaling as part of the decode, so a 713 /// megapixel JPEG costs about 2.5 seconds and under 100MB rather than the ~2.6GB a @@ -586,13 +676,14 @@ internal static string MakeBrowserSafePreview(string originalPath) try { var header = ReadImageHeader(originalPath); - long pixels = (long)header.width * header.height; - if (pixels <= kMaxPreviewPixels) + if (!NeedsStandIn(originalPath, header.width, header.height)) return null; // Constrain the longer side; WIC preserves the aspect ratio when only one of // DecodePixelWidth/DecodePixelHeight is set, and does the scaling as part of - // the decode rather than decoding full size and shrinking afterwards. + // the decode rather than decoding full size and shrinking afterwards. An image + // that is already smaller than that is left at its own size — a stand-in made + // only because of its format has no reason to be enlarged. // A stream rather than a UriSource, because Uri would treat a "#" in a // perfectly legal filename as the start of a fragment. CacheOption.OnLoad // makes EndInit() do the decoding, so the stream can be closed right after. @@ -603,10 +694,13 @@ internal static string MakeBrowserSafePreview(string originalPath) scaled.StreamSource = stream; scaled.CacheOption = BitmapCacheOption.OnLoad; scaled.CreateOptions = BitmapCreateOptions.IgnoreColorProfile; - if (header.width >= header.height) - scaled.DecodePixelWidth = kPreviewMaxDimension; - else - scaled.DecodePixelHeight = kPreviewMaxDimension; + if (Math.Max(header.width, header.height) > kPreviewMaxDimension) + { + if (header.width >= header.height) + scaled.DecodePixelWidth = kPreviewMaxDimension; + else + scaled.DecodePixelHeight = kPreviewMaxDimension; + } scaled.EndInit(); } scaled.Freeze(); @@ -629,14 +723,14 @@ internal static string MakeBrowserSafePreview(string originalPath) "BloomImagePreview-" + Guid.NewGuid() + ".jpg" ); var encoder = new JpegBitmapEncoder { QualityLevel = 85 }; - encoder.Frames.Add(BitmapFrame.Create(upright)); + encoder.Frames.Add(BitmapFrame.Create(FlattenOntoWhite(upright))); using (var output = RobustFile.Create(previewPath)) encoder.Save(output); Logger.WriteMinorEvent( $"ImageGalleryApi made a {upright.PixelWidth}x{upright.PixelHeight} preview" + $" for {Path.GetFileName(originalPath)}" - + $" ({header.width}x{header.height}, too large for the browser)" + + $" ({header.width}x{header.height}, which the browser could not display)" ); return previewPath; } diff --git a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs index 425939d01341..a186595e9c58 100644 --- a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs +++ b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs @@ -128,6 +128,22 @@ private string WritePng(string name, int width, int height) return path; } + /// + /// Writes a TIFF, optionally with see-through areas. TIFF is the format the browser + /// cannot draw, so these are the fixtures for the stand-in-because-of-format path. + /// + private string WriteTiff(string name, int width, int height, bool transparent) + { + var path = Path.Combine(_tempFolder, name); + using (var bitmap = new Bitmap(width, height, PixelFormat.Format32bppArgb)) + { + using (var g = Graphics.FromImage(bitmap)) + g.Clear(transparent ? Color.Transparent : Color.CornflowerBlue); + bitmap.Save(path, ImageFormat.Tiff); + } + return path; + } + [Test] public void GetImageDimensions_ReturnsThePixelDimensions() { @@ -206,6 +222,102 @@ public void MakeBrowserSafePreview_DownscalesAnImageTooBigForTheBrowser() } } + [Test] + public void MakeBrowserSafePreview_MakesAStandInForATiff_EvenASmallOne() + { + // No browser will draw a TIFF, so size is not the only reason to substitute one. + const int width = 300; + const int height = 200; + Assert.That( + (long)width * height, + Is.LessThan(ImageGalleryApi.kMaxPreviewPixels), + "Test setup: this one is meant to be under the size threshold, so that only " + + "its format can be the reason for a stand-in" + ); + var path = WriteTiff("scan.tif", width, height, transparent: false); + + var previewPath = ImageGalleryApi.MakeBrowserSafePreview(path); + + Assert.That(previewPath, Is.Not.Null, "A TIFF always needs a stand-in"); + try + { + using var preview = Image.FromFile(previewPath); + Assert.That( + preview.RawFormat.Guid, + Is.EqualTo(ImageFormat.Jpeg.Guid), + "The stand-in has to be something the browser can draw" + ); + // Small enough already; there is no reason to enlarge it. + Assert.That(preview.Width, Is.EqualTo(width)); + Assert.That(preview.Height, Is.EqualTo(height)); + } + finally + { + File.Delete(previewPath); + } + } + + [Test] + public void MakeBrowserSafePreview_ShowsSeeThroughAreasAsWhite_NotBlack() + { + // JPEG has no alpha channel, so without compositing first, a fully transparent + // pixel encodes as whatever is in its colour channels — normally black. + var path = WriteTiff("transparent.tif", 120, 90, transparent: true); + + var previewPath = ImageGalleryApi.MakeBrowserSafePreview(path); + + Assert.That(previewPath, Is.Not.Null, "Test setup: expected a stand-in for a TIFF"); + try + { + using var preview = new Bitmap(previewPath); + var corner = preview.GetPixel(0, 0); + var middle = preview.GetPixel(preview.Width / 2, preview.Height / 2); + // JPEG is lossy, so allow a little drift rather than demanding pure 255s. + Assert.That( + corner.R + corner.G + corner.B, + Is.GreaterThan(720), + $"Transparent areas should come out white; the corner was {corner}" + ); + Assert.That( + middle.R + middle.G + middle.B, + Is.GreaterThan(720), + $"Transparent areas should come out white; the middle was {middle}" + ); + } + finally + { + File.Delete(previewPath); + } + } + + [Test] + public void MakeBrowserSafePreview_KeepsTheColoursOfAnOpaqueImage() + { + // Sanity check on the compositing above: it must not wash out a solid image. + var path = WriteTiff("solid.tif", 120, 90, transparent: false); + + var previewPath = ImageGalleryApi.MakeBrowserSafePreview(path); + + Assert.That(previewPath, Is.Not.Null, "Test setup: expected a stand-in for a TIFF"); + try + { + using var preview = new Bitmap(previewPath); + var middle = preview.GetPixel(preview.Width / 2, preview.Height / 2); + var expected = Color.CornflowerBlue; + Assert.That( + Math.Abs(middle.R - expected.R) + + Math.Abs(middle.G - expected.G) + + Math.Abs(middle.B - expected.B), + Is.LessThan(30), + $"Expected roughly {expected} but the stand-in had {middle}" + ); + } + finally + { + File.Delete(previewPath); + } + } + [Test] public void MakeBrowserSafePreview_WorksOnAnMtaBackgroundThread() { From 9fd3bd6d615452a2f033c43374c01ea95e281ad8 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 13:41:44 -0500 Subject: [PATCH 6/8] Test the EXIF orientation path, which nothing covered (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin pointed out that every image fixture so far was orientation 1, so none of the rotated paths were exercised and a regression in them would go unnoticed. It also observed that baking in the rotation by hand is only correct if WPF's decoder doesn't already do it — true as far as anyone knew, but nothing established it. Both are now settled by the same test. A JPEG fixture carrying a chosen EXIF orientation is built by splicing a hand-written APP1 segment in after the start-of-image marker (System.Drawing cannot construct a PropertyItem to attach one, and pulling WPF into the test project just to write a fixture costs more than twenty bytes of well-specified header). All eight orientations are then checked: 5-8 must report the stored dimensions swapped, 1-4 unswapped. Those first four would pass even if the tag were being ignored entirely, but 5-8 cannot — and they would also fail if WPF were auto-rotating, since the swap would then be applied twice. Also covers an orientation value outside 1-8 being ignored rather than reaching the rotation table. Riding along: a "seen again" note on the existing papercut about the nine tests build/agent-dotnet.sh cannot run, which cost time again on this branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/controllers/ImageGalleryApiTests.cs | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs index a186595e9c58..91dd0f7302b8 100644 --- a/src/BloomTests/web/controllers/ImageGalleryApiTests.cs +++ b/src/BloomTests/web/controllers/ImageGalleryApiTests.cs @@ -128,6 +128,87 @@ private string WritePng(string name, int width, int height) return path; } + /// + /// Writes a JPEG carrying the given EXIF orientation, by splicing a hand-built APP1 + /// segment in directly after the start-of-image marker. Done by hand because + /// System.Drawing cannot construct a PropertyItem to attach, and pulling WPF into + /// this test project just to write a fixture would be a heavier price than 20 bytes + /// of well-specified header. + /// + private string WriteJpegWithExifOrientation( + string name, + int width, + int height, + ushort orientation + ) + { + var path = Path.Combine(_tempFolder, name); + byte[] jpeg; + using (var bitmap = new Bitmap(width, height, PixelFormat.Format24bppRgb)) + using (var memory = new MemoryStream()) + { + using (var g = Graphics.FromImage(bitmap)) + g.Clear(Color.CornflowerBlue); + bitmap.Save(memory, ImageFormat.Jpeg); + jpeg = memory.ToArray(); + } + Assert.That( + jpeg[0] == 0xFF && jpeg[1] == 0xD8, + Is.True, + "Test setup: expected the file to start with a JPEG start-of-image marker" + ); + + // APP1 holding a minimal little-endian TIFF header whose only IFD entry is + // tag 274 (0x0112, Orientation), type 3 (SHORT), one value. + var app1 = new byte[] + { + 0xFF, + 0xE1, // APP1 marker + 0x00, + 0x22, // segment length (34), including these two bytes + 0x45, + 0x78, + 0x69, + 0x66, + 0x00, + 0x00, // "Exif\0\0" + 0x49, + 0x49, + 0x2A, + 0x00, // little-endian TIFF header + 0x08, + 0x00, + 0x00, + 0x00, // offset of IFD0 from the TIFF header + 0x01, + 0x00, // one directory entry + 0x12, + 0x01, // tag 0x0112 = Orientation + 0x03, + 0x00, // type 3 = SHORT + 0x01, + 0x00, + 0x00, + 0x00, // one value + (byte)(orientation & 0xFF), + (byte)(orientation >> 8), + 0x00, + 0x00, // the value, in the entry itself + 0x00, + 0x00, + 0x00, + 0x00, // no next IFD + }; + + using (var file = File.Create(path)) + { + file.Write(jpeg, 0, 2); // the start-of-image marker + file.Write(app1, 0, app1.Length); + file.Write(jpeg, 2, jpeg.Length - 2); // the rest of the original + } + return path; + } + /// /// Writes a TIFF, optionally with see-through areas. TIFF is the format the browser /// cannot draw, so these are the fixtures for the stand-in-because-of-format path. @@ -222,6 +303,46 @@ public void MakeBrowserSafePreview_DownscalesAnImageTooBigForTheBrowser() } } + // The eight EXIF orientations. 5-8 are the ones that stand the image on its side, + // so for those the dimensions the viewer ends up seeing are the stored ones swapped. + [TestCase((ushort)1, 400, 200, TestName = "Orientation1_Upright")] + [TestCase((ushort)2, 400, 200, TestName = "Orientation2_MirroredHorizontally")] + [TestCase((ushort)3, 400, 200, TestName = "Orientation3_UpsideDown")] + [TestCase((ushort)4, 400, 200, TestName = "Orientation4_MirroredVertically")] + [TestCase((ushort)5, 200, 400, TestName = "Orientation5_Transposed")] + [TestCase((ushort)6, 200, 400, TestName = "Orientation6_QuarterTurnedClockwise")] + [TestCase((ushort)7, 200, 400, TestName = "Orientation7_Transverse")] + [TestCase((ushort)8, 200, 400, TestName = "Orientation8_QuarterTurnedAnticlockwise")] + public void GetImageDimensions_AllowsForExifRotation( + ushort orientation, + int expectedWidth, + int expectedHeight + ) + { + // Stored 400x200 every time; only the orientation tag differs. + var path = WriteJpegWithExifOrientation( + $"rotated{orientation}.jpg", + 400, + 200, + orientation + ); + + var (width, height) = ImageGalleryApi.GetImageDimensions(path); + + Assert.That(width, Is.EqualTo(expectedWidth)); + Assert.That(height, Is.EqualTo(expectedHeight)); + } + + [Test] + public void GetImageDimensions_TreatsAnOutOfRangeOrientationAsUpright() + { + // 9 is not a real EXIF orientation; the reader should ignore it rather than + // feed it to the rotation table. + var path = WriteJpegWithExifOrientation("nonsense.jpg", 400, 200, 9); + + Assert.That(ImageGalleryApi.GetImageDimensions(path), Is.EqualTo((400, 200))); + } + [Test] public void MakeBrowserSafePreview_MakesAStandInForATiff_EvenASmallOne() { From 202bc79b73686696b7a667b78ada60e9eae4da6a Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 27 Jul 2026 14:05:57 -0500 Subject: [PATCH 7/8] Log a papercut: three C# tests fail intermittently (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noticed while preflighting this branch. One full-suite run showed 12 failures rather than the usual 9 environmental ones; a second run at the same commit showed 9, and the three extras passed in isolation. None of them are near this branch's subject. Recording it because the cost is that a run of 12 can no longer be read at a glance — you have to run the suite twice to find out which failures are noise. Docs only; no product code in this commit. Co-Authored-By: Claude Opus 5 (1M context) --- PAPERCUTS.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/PAPERCUTS.md b/PAPERCUTS.md index c96dceaf2749..fec238200a79 100644 --- a/PAPERCUTS.md +++ b/PAPERCUTS.md @@ -116,6 +116,22 @@ House rules: the exit code" hazard, different cause. +## 2026-07-27 — Three C# tests fail intermittently, on top of the nine environmental ones + +- **Cut:** One full-suite run came back with 12 failures instead of the usual 9. The extras were + `CheckAudioForAllText_SpansAudioMissing`, `BringBookUpToDate_MovesMetaDataToJson` and + `InsertPageAfter_FromAnotherBook_CopiesWidget(True)`. A second run at the *identical* commit gave + 9 again, and all three passed when run on their own, so they are flaky rather than broken — none + of them were anywhere near the branch's subject (image handling). The cost is readability: on top + of the nine already-known environmental failures, an agent seeing 12 now has to run the suite a + second time to work out which are noise before it can call a run clean. +- **Idea:** Find the shared state (all three build books/collections in temp folders, so likely a + fixture or folder-name collision when the suite runs under load) — or, cheaply, quarantine them + with `[Retry]` so the noise stops masking real failures. +- **Context:** BloomDesktop, seen during `/preflight` of PR #8111 (BL-16597), on runs 5 and 6 of six + full-suite runs that day; runs 1-4 and 6 all showed exactly the known 9. + + ## 2026-07-24 — agent-dotnet.sh test exits 0 even when tests fail - **Cut:** `build/agent-dotnet.sh test src/BloomTests/BloomTests.csproj` returned exit code 0 on a From 336da95eefbba7c42635bfafbe1475296bdd9853 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 4 Aug 2026 13:57:21 -0500 Subject: [PATCH 8/8] Don't hold the global api lock while building a large image preview (BL-16597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin caught this on the rebased branch. RegisterEndpointHandler's requiresSync parameter defaults to true, and imageGallery/localFilePreview was registered with only three arguments, so it ran inside BloomApiHandler's main SyncObj. That lock is what serializes most api requests against each other, and this handler holds it for as long as it takes to decode and re-encode the picked image — about two and a half seconds for the 713 megapixel scan in the tests. Every other synchronized request waits that out, so choosing a very large image in the chooser could briefly freeze unrelated parts of the UI. Nothing here needs that lock. The only state shared between requests is the preview cache, and _previewLock already guards every read and write of it (and still coalesces the gallery's near-simultaneous thumbnail and large-preview requests for the same file into a single build). The endpoint is also not on the short list that BloomApiHandler routes to ThumbnailsAndPreviewsSyncObj, so opting out is the same move the i18n endpoints already make, for the same reason. One consequence worth recording: serving the file (ReplyWithImage) already happened outside _previewLock, and pickLocalImageFile still takes the global lock, so the two can now overlap where previously they could not. If a new file were picked in the window between choosing the path and streaming it, the temp stand-in could be deleted mid-serve and the preview would fail to load. That needs a modal file dialog to be opened and confirmed inside a file-serve window, and the worst outcome is a broken preview image, so it is left alone rather than paid for with a longer lock hold. Riding along, per the papercut convention: the entry about three intermittently failing C# tests no longer opens by counting them "on top of the nine environmental ones". Master's PR #8107 fixed those nine and deleted their papercut, and a full run this session came back 0 of 3027 — so the entry now states its own case, and records that a green baseline makes those three the only remaining noise. Co-Authored-By: Claude Opus 5 (1M context) --- PAPERCUTS.md | 29 ++++++++++++------- .../web/controllers/ImageGalleryApi.cs | 10 ++++++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/PAPERCUTS.md b/PAPERCUTS.md index fec238200a79..1ca8ee317169 100644 --- a/PAPERCUTS.md +++ b/PAPERCUTS.md @@ -116,20 +116,27 @@ House rules: the exit code" hazard, different cause. -## 2026-07-27 — Three C# tests fail intermittently, on top of the nine environmental ones - -- **Cut:** One full-suite run came back with 12 failures instead of the usual 9. The extras were - `CheckAudioForAllText_SpansAudioMissing`, `BringBookUpToDate_MovesMetaDataToJson` and - `InsertPageAfter_FromAnotherBook_CopiesWidget(True)`. A second run at the *identical* commit gave - 9 again, and all three passed when run on their own, so they are flaky rather than broken — none - of them were anywhere near the branch's subject (image handling). The cost is readability: on top - of the nine already-known environmental failures, an agent seeing 12 now has to run the suite a - second time to work out which are noise before it can call a run clean. +## 2026-07-27 — Three C# tests fail intermittently under load + +- **Cut:** `CheckAudioForAllText_SpansAudioMissing`, `BringBookUpToDate_MovesMetaDataToJson` and + `InsertPageAfter_FromAnotherBook_CopiesWidget(True)` fail sometimes and pass sometimes. They + failed together on one full-suite run, passed on a second run at the *identical* commit, and + passed when run on their own — so they are flaky rather than broken, and none of them is anywhere + near what that branch was changing (image handling). The cost is that a full run can no longer be + trusted on one reading: an agent has to run the suite twice to tell noise from a real regression. + That matters more now than when this was first written, because the full suite otherwise comes + back completely green (see the 2026-08-04 note below) — these three are the only remaining noise, + so any other failure is signal. - **Idea:** Find the shared state (all three build books/collections in temp folders, so likely a fixture or folder-name collision when the suite runs under load) — or, cheaply, quarantine them with `[Retry]` so the noise stops masking real failures. -- **Context:** BloomDesktop, seen during `/preflight` of PR #8111 (BL-16597), on runs 5 and 6 of six - full-suite runs that day; runs 1-4 and 6 all showed exactly the known 9. +- **Context:** BloomDesktop, seen during `/preflight` of PR #8111 (BL-16597), on two of six + full-suite runs that day. +- **2026-08-04:** All three passed on a full run of 3027 tests with **0 failures** — the first + entirely green full suite under `build/agent-dotnet.sh`, now that PR #8107 has fixed the nine + environmental failures that used to accompany them. Not evidence against this cut: intermittent + is intermittent. Recorded because it removes the nine-failure baseline the original wording + leaned on. ## 2026-07-24 — agent-dotnet.sh test exits 0 even when tests fail diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index eebdd72dc812..f309b60189d5 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -108,10 +108,18 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) HandlePickLocalImageFile, false ); + // requiresSync: false deliberately. Building a stand-in for a very large image + // decodes and re-encodes it, which takes on the order of seconds; if this ran under + // the api handler's global lock it would stall every other synchronized request for + // that whole time. Nothing here needs that lock: the only state shared between + // requests is the preview cache, and _previewLock already guards it (and still + // coalesces the gallery's near-simultaneous thumbnail and large-preview requests + // into a single build). apiHandler.RegisterEndpointHandler( "imageGallery/localFilePreview", HandleLocalFilePreview, - false + false, + requiresSync: false ); apiHandler.RegisterEndpointHandler( "imageGallery/local-collections/collections",