Fix HEIC and JXL image rotation by properly reading orientation metadata #1055
Conversation
WalkthroughThe PR refines thumbnail generation for HEIC and JPEG XL formats by extracting actual image orientation from metadata instead of using hardcoded defaults. Orientation-aware metadata decoding is now applied to both formats before thumbnail creation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes follow a consistent pattern applied to two image formats with straightforward metadata extraction and error handling improvements. Single-file scope with homogeneous modifications across HEIC and JXL code paths. Possibly related PRs
Suggested labels
Security recommendations
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: tankerkiller125 <[email protected]>
Co-authored-by: tankerkiller125 <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/internal/data/repo/repo_item_attachments.go (2)
445-447: Bug: lost error on Tx creationReturning nil on Tx failure reports success to caller.
Apply:
- if err != nil { - return nil - } + if err != nil { + return err + }
261-266: Add deferredtopic.Shutdown(ctx)after eachpubsub.OpenTopicinrepo_item_attachments.go
- In
backend/internal/data/repo/repo_item_attachments.goat line 261, insertdefer topic.Shutdown(ctx)immediately after theOpenTopiccall.- At line 727, do the same for the second
OpenTopic.No other
OpenTopicwithout shutdown was found. Closing topics promptly prevents resource leaks and potential Denial-of-Service scenarios.
🧹 Nitpick comments (6)
backend/internal/data/repo/repo_item_attachments.go (6)
629-651: HEIC: fall back on missing/bad metadata and clamp orientationDon’t fail thumbnailing if HEIC metadata is unreadable. Default to 1 and clamp to [1..8]; log at debug instead of error. This aligns with your JXL path and avoids unnecessary rollbacks.
Apply:
- log.Debug().Msg("reading original file orientation") - imageMeta, err := imagemeta.DecodeHeif(bytes.NewReader(contentBytes)) - if err != nil { - log.Err(err).Msg("failed to decode heic file metadata") - err := tx.Rollback() - if err != nil { - return err - } - return err - } - orientation := uint16(imageMeta.Orientation) + log.Debug().Msg("reading original file orientation") + orientation := uint16(1) // default + imageMeta, err := imagemeta.DecodeHeif(bytes.NewReader(contentBytes)) + if err != nil { + log.Debug().Err(err).Msg("unable to decode heic metadata; defaulting orientation to 1") + } else { + if o := uint16(imageMeta.Orientation); o >= 1 && o <= 8 { + orientation = o + } else { + log.Debug().Uint16("orientation", uint16(imageMeta.Orientation)).Msg("invalid heic orientation; defaulting to 1") + } + }
660-679: JXL: good fallback; also clamp orientation to [1..8]Prevents invalid values from corrupting rotation.
Apply:
- orientation := uint16(1) // Default orientation + orientation := uint16(1) // Default orientation imageMeta, err := imagemeta.Decode(bytes.NewReader(contentBytes)) if err != nil { log.Debug().Msg("unable to decode jxl metadata, using default orientation") } else { - orientation = uint16(imageMeta.Orientation) + if o := uint16(imageMeta.Orientation); o >= 1 && o <= 8 { + orientation = o + } else { + log.Debug().Uint16("orientation", uint16(imageMeta.Orientation)).Msg("invalid jxl orientation; defaulting to 1") + } }
533-542: Make extension-based content-type overrides case-insensitiveUppercase extensions (e.g., .HEIC, .JXL) currently won’t match.
Apply:
-if contentType == "application/octet-stream" { - switch { - case strings.HasSuffix(title, ".heic") || strings.HasSuffix(title, ".heif"): - contentType = "image/heic" - case strings.HasSuffix(title, ".avif"): - contentType = "image/avif" - case strings.HasSuffix(title, ".jxl"): - contentType = "image/jxl" - } -} +if contentType == "application/octet-stream" { + switch strings.ToLower(filepath.Ext(title)) { + case ".heic", ".heif": + contentType = "image/heic" + case ".avif": + contentType = "image/avif" + case ".jxl": + contentType = "image/jxl" + } +}
461-463: Use typed enum for attachment type “thumbnail”Avoid string drift; be consistent with other usages of
attachment.Type*.Apply:
- att := tx.Attachment.Create(). + att := tx.Attachment.Create(). SetID(uuid.New()). SetTitle(fmt.Sprintf("%s-thumb", title)). - SetType("thumbnail") + SetType(attachment.TypeThumbnail)And:
- attachment.TypeNEQ("thumbnail"), + attachment.TypeNEQ(attachment.TypeThumbnail),Also applies to: 716-717
688-688: Typo in error string“thumnails” → “thumbnails”.
Apply:
- return fmt.Errorf("file type %s is not supported for thumbnail creation or document thumnails disabled", title) + return fmt.Errorf("file type %s is not supported for thumbnail creation or document thumbnails disabled", title)
441-710: Security hardening: cap decoded pixels and unify metadata-fallback behavior
- Add a maximum pixel count (e.g., 100MP) to mitigate decode-bombs; validate
bounds.Dx()*bounds.Dy()before scaling.- Prefer graceful default-orientation fallback for all formats (JPEG/PNG/GIF/WebP) when EXIF read fails, mirroring JXL.
- Consider context deadlines for long decodes; respect
ctx.Done()to abort.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.scaffold/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
backend/internal/data/repo/repo_item_attachments.go(2 hunks)
🔇 Additional comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
632-632: LGTM: corrected error messagesNice cleanup: error messages now correctly reference HEIC and JPEG XL.
Also applies to: 663-663
Problem
Images uploaded in HEIC and JXL formats were displaying rotated 90 degrees clockwise from their original orientation. This occurred because the orientation metadata embedded by phones and cameras was not being properly read during thumbnail creation, causing images to display in their raw sensor orientation rather than the intended viewing orientation.
Root Cause
In the thumbnail creation process (
CreateThumbnailfunction):imagemeta.Decode()function which doesn't properly handle HEIF/HEIC format metadata, failing to extract the correct orientation value1(no rotation)Solution
This PR makes minimal, surgical changes to properly extract and apply orientation metadata for both formats:
HEIC Images
imagemeta.Decode()toimagemeta.DecodeHeif()- the format-specific decoder designed for HEIF/HEIC metadata extractionJXL Images
imagemeta.Decode()with graceful fallback1Error Message Improvements
Technical Details
The extracted orientation values are passed to the existing
ApplyOrientation()function inutils/image.go, which correctly handles all 8 EXIF orientation transformations during thumbnail generation. This ensures images display in their intended orientation regardless of how the camera sensor captured them.Testing
Fixes #1067
Co-authored-by: tankerkiller125 [email protected]
Original prompt
Fixes #983
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit