Add pptx_optimize_images tool (#85)#93
Conversation
- Add Magick.NET-Q8-AnyCPU dependency for cross-platform image processing - Implement OptimizeImages service method in PresentationService.ImageOptimization.cs - Add pptx_optimize_images MCP tool with targetDpi, jpegQuality, convertFormats parameters - Create ImageOptimizationResult and OptimizedImageInfo models - Downscale images based on display dimensions vs. pixel dimensions at target DPI - Convert BMP/TIFF to PNG/JPEG when convertFormats=true - Recompress JPEG images at specified quality level - Only replace images when optimization results in smaller size - Validate package with OpenXmlValidator before and after modification - All 542 tests passing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongalloway
added
type:feature
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new MCP tool to optimize embedded images in PPTX files, extending PresentationService with an ImageMagick-based workflow and returning structured per-image savings/metadata.
Changes:
- Added
pptx_optimize_imagestool wiring and structured error result handling. - Implemented
PresentationService.OptimizeImages()to traverse presentation image parts and attempt downscale/convert/recompress with OpenXmlValidator checks. - Introduced new result models and added Magick.NET as a dependency.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/PptxMcp/Tools/PptxTools.Optimization.cs |
Adds the new MCP tool entry point and structured result fallback. |
src/PptxMcp/Services/PresentationService.ImageOptimization.cs |
Core image optimization implementation using Magick.NET and OpenXML part traversal. |
src/PptxMcp/Models/ImageOptimizationResult.cs |
Defines result records for per-image and overall optimization reporting. |
src/PptxMcp/PptxMcp.csproj |
Adds Magick.NET-Q8-AnyCPU dependency. |
.squad/agents/nate/history.md |
Adds research notes/history entry related to Magick.NET feasibility. |
Comments suppressed due to low confidence (1)
src/PptxMcp/Services/PresentationService.ImageOptimization.cs:201
targetWidth/targetHeightcan become 0 when the picture extents are 0 EMUs (or extremely small), which can causeimage.Resize((uint)targetWidth, ...)to throw. Clamp the computed target dimensions to at least 1 pixel (and consider an upper bound as well) before callingResize().
targetWidth = (int)Math.Ceiling(displayWidthPixels);
targetHeight = (int)Math.Ceiling(displayHeightPixels);
// Preserve aspect ratio.
if (targetWidth / (double)targetHeight > aspectRatio)
targetWidth = (int)Math.Ceiling(targetHeight * aspectRatio);
else
targetHeight = (int)Math.Ceiling(targetWidth / aspectRatio);
}
}
// Determine if format conversion is needed.
bool needsConversion = convertFormats &&
(info.Format == MagickFormat.Bmp ||
info.Format == MagickFormat.Tiff ||
info.Format == MagickFormat.Tiff64);
// Determine target format.
MagickFormat targetFormat = info.Format;
if (needsConversion)
{
// Convert BMP/TIFF to PNG for lossless, or JPEG for photos.
// Use PNG as default for safety.
targetFormat = MagickFormat.Png;
}
// Perform optimization if needed.
if (!needsDownscaling && !needsConversion && info.Format != MagickFormat.Jpeg)
{
// No optimization possible.
return new OptimizedImageInfo(
ImagePath: imagePart.Uri.ToString(),
OriginalFormat: originalFormat,
OptimizedFormat: originalFormat,
OriginalWidth: originalWidth,
OriginalHeight: originalHeight,
OptimizedWidth: originalWidth,
OptimizedHeight: originalHeight,
OriginalSizeBytes: originalSize,
OptimizedSizeBytes: originalSize,
BytesSaved: 0,
Action: "skipped");
}
using var image = new MagickImage(originalCopy);
bool modified = false;
var actions = new List<string>();
// Downscale if needed.
if (needsDownscaling)
{
image.Resize((uint)targetWidth, (uint)targetHeight);
actions.Add("downscaled");
| // Convert format if needed. | ||
| if (needsConversion) | ||
| { | ||
| image.Format = targetFormat; | ||
| actions.Add($"converted to {targetFormat}"); | ||
| modified = true; | ||
| } | ||
|
|
||
| // Recompress JPEG. | ||
| if (image.Format == MagickFormat.Jpeg) | ||
| { | ||
| image.Quality = (uint)jpegQuality; | ||
| actions.Add("recompressed"); | ||
| modified = true; | ||
| } | ||
|
|
||
| if (!modified) | ||
| { | ||
| return new OptimizedImageInfo( | ||
| ImagePath: imagePart.Uri.ToString(), | ||
| OriginalFormat: originalFormat, | ||
| OptimizedFormat: originalFormat, | ||
| OriginalWidth: originalWidth, | ||
| OriginalHeight: originalHeight, | ||
| OptimizedWidth: originalWidth, | ||
| OptimizedHeight: originalHeight, | ||
| OriginalSizeBytes: originalSize, | ||
| OptimizedSizeBytes: originalSize, | ||
| BytesSaved: 0, | ||
| Action: "skipped"); | ||
| } | ||
|
|
||
| // Write optimized image to memory. | ||
| var optimizedStream = new MemoryStream(); | ||
| image.Write(optimizedStream); | ||
| optimizedStream.Position = 0; | ||
|
|
||
| long optimizedSize = optimizedStream.Length; | ||
|
|
||
| // Only replace if the new image is smaller. | ||
| if (optimizedSize < originalSize) | ||
| { | ||
| imagePart.FeedData(optimizedStream); | ||
|
|
There was a problem hiding this comment.
needsConversion changes image.Format and writes the converted bytes back into the existing ImagePart via FeedData(). That keeps the original part URI/content-type (e.g., .bmp / image/bmp) while storing PNG bytes, which can produce a corrupted/invalid PPTX in PowerPoint. For format conversion, create a new ImagePart with the correct content type/extension, update all Blip.Embed relationship IDs that referenced the old part (similar to the deduplication redirect logic), then delete the old part relationships once nothing references it.
| int targetDpi = 150, | ||
| int jpegQuality = 85, | ||
| bool convertFormats = true) | ||
| { |
There was a problem hiding this comment.
targetDpi and jpegQuality are documented as constrained (DPI > 0, JPEG quality 1–100) but aren’t validated. A negative targetDpi can make displayWidthPixels negative and lead to a negative targetWidth/targetHeight, which then gets cast to uint in Resize() (potentially triggering huge allocations / OOM). Add explicit argument validation/clamping for these parameters before doing any calculations.
This issue also appears on line 148 of the same file.
| { | |
| { | |
| if (targetDpi <= 0) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(targetDpi), "targetDpi must be greater than 0."); | |
| } | |
| if (jpegQuality < 1 || jpegQuality > 100) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(jpegQuality), "jpegQuality must be between 1 and 100."); | |
| } |
| foreach (var picture in shapeTree.Elements<P.Picture>()) | ||
| { | ||
| var blip = picture.GetFirstChild<P.BlipFill>()?.GetFirstChild<A.Blip>(); | ||
| if (blip?.Embed?.Value == relId) | ||
| { | ||
| // Found the picture shape; extract display dimensions. | ||
| var extents = picture.ShapeProperties?.Transform2D?.Extents; | ||
| if (extents?.Cx?.HasValue == true && extents.Cy?.HasValue == true) | ||
| { | ||
| return (extents.Cx.Value, extents.Cy.Value); | ||
| } | ||
| } |
There was a problem hiding this comment.
GetImageDisplaySize() only checks shapeTree.Elements<P.Picture>() (direct children), so it will miss pictures nested inside group shapes and other containers. Using a descendant traversal (as is done for blip updates elsewhere) would make the display-size detection more complete and reduce unexpected “no downscaling” cases.
| [McpServerTool(Title = "Optimize Images")] | ||
| public partial Task<string> pptx_optimize_images( | ||
| string filePath, | ||
| int targetDpi = 150, | ||
| int jpegQuality = 85, | ||
| bool convertFormats = true) => | ||
| ExecuteToolStructured(filePath, | ||
| () => _service.OptimizeImages(filePath, targetDpi, jpegQuality, convertFormats), | ||
| error => new ImageOptimizationResult( | ||
| Success: false, | ||
| FilePath: filePath, | ||
| ImagesProcessed: 0, | ||
| ImagesSkipped: 0, | ||
| TotalBytesBefore: 0, | ||
| TotalBytesAfter: 0, | ||
| TotalBytesSaved: 0, | ||
| OptimizedImages: [], | ||
| Validation: new ValidationStatus(0, 0, false), | ||
| Message: error)); |
There was a problem hiding this comment.
No tests were added for the new pptx_optimize_images tool / OptimizeImages() behavior. The repo has established integration tool tests (e.g., tests/PptxMcp.Tests/Tools/PptxToolsTests.cs), so this should include coverage for at least: (1) no-op when already within target DPI, (2) downscaling reduces pixel dimensions and file size, and (3) format conversion path (BMP/TIFF) if supported.
| // Track which ImageParts we've already processed (same part may be shared). | ||
| var processedImageUris = new HashSet<string>(); | ||
|
|
||
| foreach (var ownerPart in allOwnerParts) | ||
| { | ||
| foreach (var idPartPair in ownerPart.Parts) | ||
| { | ||
| if (idPartPair.OpenXmlPart is not ImagePart imagePart) | ||
| continue; | ||
|
|
||
| var uri = imagePart.Uri.ToString(); | ||
| if (!processedImageUris.Add(uri)) | ||
| continue; // Already processed this image | ||
|
|
||
| var imageInfo = OptimizeImagePart( | ||
| ownerPart, | ||
| imagePart, | ||
| targetDpi, | ||
| jpegQuality, | ||
| convertFormats); |
There was a problem hiding this comment.
Downscaling chooses a single display size for each ImagePart (first owner part encountered + first matching Picture on that part). If the same underlying image part is reused on multiple slides (or multiple times on one slide) at different sizes, this can downscale to the smallest encountered size and degrade quality where the image is displayed larger. Consider aggregating the maximum required display dimensions across all references before deciding a target size, or skipping downscaling when you can’t reliably compute the largest on-slide usage.
| // Read image metadata with lightweight MagickImageInfo. | ||
| MagickImageInfo info; | ||
| try | ||
| { | ||
| info = new MagickImageInfo(originalCopy); | ||
| originalCopy.Position = 0; | ||
| } | ||
| catch | ||
| { | ||
| // Corrupted or unsupported image format. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
OptimizeImagePart() returns null on corrupted/unsupported images, which means those images are silently omitted from OptimizedImages and from the processed/skipped counts and byte totals. Consider returning an OptimizedImageInfo entry with an explicit “error/unsupported” action (and sizes if available) so the result accounting reflects everything scanned.
| /// Optimize images in a PowerPoint presentation by downscaling, converting formats, and recompressing. | ||
| /// Scans all images across slides, layouts, and masters. Downscales images that are larger than their | ||
| /// display dimensions warrant based on target DPI. Converts BMP/TIFF to PNG/JPEG. Recompresses JPEG images | ||
| /// at the specified quality level. Only replaces images when optimization results in smaller file size. |
There was a problem hiding this comment.
The tool XML doc says it “Scans all images across slides, layouts, and masters” and downscales based on display dimensions, but the implementation only computes display size for SlidePart pictures (layouts/masters return null, so they won’t be downscaled based on on-slide extents). Either extend display-size calculation to layouts/masters or adjust the tool description to avoid overstating the behavior.
| /// Optimize images in a PowerPoint presentation by downscaling, converting formats, and recompressing. | |
| /// Scans all images across slides, layouts, and masters. Downscales images that are larger than their | |
| /// display dimensions warrant based on target DPI. Converts BMP/TIFF to PNG/JPEG. Recompresses JPEG images | |
| /// at the specified quality level. Only replaces images when optimization results in smaller file size. | |
| /// Optimize images in a PowerPoint presentation by downscaling where display size is known, converting formats, and recompressing. | |
| /// Scans images across slides, layouts, and masters. For images with known on-slide display dimensions (typically on slides), | |
| /// downscales when the source image exceeds what the target DPI warrants. Converts BMP/TIFF to PNG/JPEG and recompresses JPEG | |
| /// images at the specified quality level. Only replaces images when optimization results in smaller file size. |
- Created ImageOptimizationTests.cs with 10 comprehensive test cases - Tests cover: no images, small images, JPEG recompression, format conversion, downscaling, custom parameters, multiple images, and error handling - 3 tests pass (NoImages, SmallImage, FileNotFound) - 7 tests fail due to implementation bug: 'Entries cannot be opened multiple times in Update mode' when calling FeedData after GetStream - Bug is in PresentationService.ImageOptimization.cs:247 - Tests correctly expose the implementation issue and validate expected behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Close original GetStream() before calling FeedData() to avoid 'Entries cannot be opened multiple times in Update mode' error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #85
Implementation
Key Features
Parameters
Testing
Dependencies
Uses Magick.NET (ImageMagick wrapper) per Nate's research and approval. Cross-platform, Apache 2.0 licensed.