Conversation
Contributor
WalkthroughModified metadata string construction in ExifHandler to incorporate image dimensions (WIDTHxHEIGHT format) and switched from file-based metadata loading to BitmapDecoder frame-based extraction. Updated VideoTransformCommands to introduce a DefaultFrameRate constant and replace hard-coded frame rate values throughout multiple processing code paths, including conditional loop handling for non-video inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PhotoLocator/Metadata/ExifHandler.cs (1)
647-659:⚠️ Potential issue | 🟠 MajorUse orientation-adjusted dimensions here.
For EXIF orientations
6and8,frame.PixelWidth/frame.PixelHeightare the stored raster dimensions, not the displayed ones. Portrait photos will therefore show reversed dimensions in the new metadata string.💡 Proposed fix
var orientation = orientationValue switch { 3 => Rotation.Rotate180, 6 => Rotation.Rotate90, 8 => Rotation.Rotate270, _ => Rotation.Rotate0 }; var timeStamp = DecodeTimeStamp(metadata, file); - return (GetGeotag(metadata), timeStamp, GetMetadataString(metadata, frame.PixelWidth, frame.PixelHeight, timeStamp), orientation); + var (displayWidth, displayHeight) = orientation is Rotation.Rotate90 or Rotation.Rotate270 + ? (frame.PixelHeight, frame.PixelWidth) + : (frame.PixelWidth, frame.PixelHeight); + return (GetGeotag(metadata), timeStamp, GetMetadataString(metadata, displayWidth, displayHeight, timeStamp), orientation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Metadata/ExifHandler.cs` around lines 647 - 659, The code uses frame.PixelWidth/frame.PixelHeight (stored raster dimensions) when building the metadata string and in the early null-metadata return, which is wrong for orientation 6/8; compute displayedWidth and displayedHeight after computing orientation (swap when orientation is Rotation.Rotate90 or Rotation.Rotate270) and use those displayed dimensions in the early return string and in the call to GetMetadataString (replace frame.PixelWidth/frame.PixelHeight with displayedWidth/displayedHeight); keep the rest of the logic (orientationValue, DecodeTimeStamp, GetGeotag) unchanged and reference BitmapDecoder.Create, orientationValue/orientation, OrientationQuery1/OrientationQuery2, GetMetadataString, and the early "if (frame.Metadata is not BitmapMetadata metadata) return ..." branch when applying the change.
🧹 Nitpick comments (1)
PhotoLocator/Metadata/ExifHandler.cs (1)
584-585: This overload still omits the new dimensions token.
GetMetadataString(BitmapMetadata, Stream?)now hardcodes(0, 0), so callers of this public overload still get the old metadata format whileDecodeMetadataAsyncgets the newWIDTHxHEIGHTtoken. Consider deriving dimensions here as well, or collapsing call sites onto the width/height-aware path so the output stays consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Metadata/ExifHandler.cs` around lines 584 - 585, The public overload GetMetadataString(BitmapMetadata, Stream?) currently calls GetMetadataString(metadata, 0, 0, DecodeTimeStamp(metadata, imageStream)) which hardcodes width/height and omits the new WIDTHxHEIGHT token; update this overload to derive actual image dimensions (e.g., read pixel width/height from the BitmapMetadata or the provided imageStream) and pass them into GetMetadataString(metadata, width, height, DecodeTimeStamp(...)) or refactor callers to use the width/height-aware path so the output includes the new dimensions token consistently (look for the GetMetadataString overloads and DecodeTimeStamp usage to modify the correct method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@PhotoLocator/Metadata/ExifHandler.cs`:
- Around line 647-659: The code uses frame.PixelWidth/frame.PixelHeight (stored
raster dimensions) when building the metadata string and in the early
null-metadata return, which is wrong for orientation 6/8; compute displayedWidth
and displayedHeight after computing orientation (swap when orientation is
Rotation.Rotate90 or Rotation.Rotate270) and use those displayed dimensions in
the early return string and in the call to GetMetadataString (replace
frame.PixelWidth/frame.PixelHeight with displayedWidth/displayedHeight); keep
the rest of the logic (orientationValue, DecodeTimeStamp, GetGeotag) unchanged
and reference BitmapDecoder.Create, orientationValue/orientation,
OrientationQuery1/OrientationQuery2, GetMetadataString, and the early "if
(frame.Metadata is not BitmapMetadata metadata) return ..." branch when applying
the change.
---
Nitpick comments:
In `@PhotoLocator/Metadata/ExifHandler.cs`:
- Around line 584-585: The public overload GetMetadataString(BitmapMetadata,
Stream?) currently calls GetMetadataString(metadata, 0, 0,
DecodeTimeStamp(metadata, imageStream)) which hardcodes width/height and omits
the new WIDTHxHEIGHT token; update this overload to derive actual image
dimensions (e.g., read pixel width/height from the BitmapMetadata or the
provided imageStream) and pass them into GetMetadataString(metadata, width,
height, DecodeTimeStamp(...)) or refactor callers to use the width/height-aware
path so the output includes the new dimensions token consistently (look for the
GetMetadataString overloads and DecodeTimeStamp usage to modify the correct
method).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3042eb52-1afa-4c47-9e64-818cc8b369ff
📒 Files selected for processing (2)
PhotoLocator/Metadata/ExifHandler.csPhotoLocator/VideoTransformCommands.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor