Add support for image orientation - #110
Conversation
… from Exif data then fallback to normal orientation.
📝 WalkthroughWalkthroughAdds EXIF orientation parsing for JPEG, PNG, and TIFF images. Propagates orientation through picture APIs. Applies DrawingML rotation, flip, and axis-swap transforms. Adds coverage for metadata parsing, API forwarding, and shape generation. ChangesEXIF orientation support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Document
participant Run
participant StoryPart
participant CT_Inline
participant CT_Picture
Document->>Run: add_picture(..., orientation)
Run->>StoryPart: new_pic_inline(..., orientation)
StoryPart->>StoryPart: resolve AUTO from Image.orientation
StoryPart->>CT_Inline: create inline picture
CT_Inline->>CT_Picture: create picture with orientation
CT_Picture->>CT_Picture: apply rotation and flip transforms
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/image/test_jpeg.py (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse non-default values in orientation propagation tests.
The new propagation paths default to
1. TheExif.from_stream()test does not assertexif.orientation, and the_App1Marker.from_stream()test passes1through both fixture and expectation. Both tests pass if the new field is dropped and the default is used. Use non-default values and assert that they reach the header and constructor.Proposed test strengthening
- jfif_markers_.app1.orientation = 1 + jfif_markers_.app1.orientation = 6 assert exif.vert_dpi == vert_dpi + assert exif.orientation == 6 - ANY, marker_code, offset, length, horz_dpi, vert_dpi, 1 + ANY, marker_code, offset, length, horz_dpi, vert_dpi, 8 - return instance_mock(request, Tiff, horz_dpi=42, vert_dpi=24, orientation=1) + return instance_mock(request, Tiff, horz_dpi=42, vert_dpi=24, orientation=8)Also applies to: 328-328, 396-396
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/image/test_jpeg.py` at line 92, Strengthen the orientation propagation tests by replacing default orientation value 1 with non-default values in the Exif.from_stream(), _App1Marker.from_stream(), and related fixture cases, then assert those values are propagated into the header and constructor results. Ensure the fixture and expected value differ from the default so the tests fail if orientation propagation is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/docx/image/jpeg.py`:
- Around line 43-45: Update both image constructors to select the EXIF-specific
APP1 marker instead of using the generic markers.app1 result, ensuring a
preceding non-EXIF APP1 marker cannot override EXIF orientation. Adjust the
marker lookup around _App1Marker.from_stream and add a regression test covering
non-EXIF APP1 before EXIF APP1, preserving the later marker’s orientation.
In `@src/docx/oxml/shape.py`:
- Around line 112-114: Update StoryPart.new_pic_inline() to account for
orientation.swaps_axes before invoking image.scaled_dimensions(): swap the
requested width and height first, then create the picture and apply the existing
inline extent handling without causing a second effective swap. Add regression
coverage for width-only, height-only, and both-dimensions requests with
axis-swapping orientations.
In `@src/docx/parts/story.py`:
- Around line 75-78: Update the picture sizing flow around
CT_Inline.new_pic_inline and the InlineShape width/height setters: resolve AUTO
orientation before scaling, swap the requested width and height when
orientation.swaps_axes is true before calling image.scaled_dimensions(), and
apply the same crossed-axis mapping in both setters so explicit dimensions
remain correct after wp:extent axis swapping.
---
Nitpick comments:
In `@tests/image/test_jpeg.py`:
- Line 92: Strengthen the orientation propagation tests by replacing default
orientation value 1 with non-default values in the Exif.from_stream(),
_App1Marker.from_stream(), and related fixture cases, then assert those values
are propagated into the header and constructor results. Ensure the fixture and
expected value differ from the default so the tests fail if orientation
propagation is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8ac872e-29ea-4822-9578-8dadf6df5ef8
📒 Files selected for processing (19)
src/docx/document.pysrc/docx/enum/shape.pysrc/docx/image/constants.pysrc/docx/image/image.pysrc/docx/image/jpeg.pysrc/docx/image/png.pysrc/docx/image/tiff.pysrc/docx/oxml/shape.pysrc/docx/oxml/simpletypes.pysrc/docx/parts/story.pysrc/docx/text/run.pytests/image/test_image.pytests/image/test_jpeg.pytests/image/test_png.pytests/image/test_tiff.pytests/oxml/test_shape.pytests/parts/test_story.pytests/test_document.pytests/text/test_run.py
| orientation = markers.app1.orientation | ||
|
|
||
| return cls(px_width, px_height, horz_dpi, vert_dpi) | ||
| return cls(px_width, px_height, horz_dpi, vert_dpi, orientation) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select an EXIF APP1 marker.
When a non-EXIF APP1 marker precedes an EXIF APP1 marker, markers.app1 returns the former. _App1Marker.from_stream() assigns that marker orientation 1, so the later EXIF orientation is ignored. Add an EXIF-specific marker lookup and use it in both constructors. Add a marker-order regression test.
Also applies to: 61-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docx/image/jpeg.py` around lines 43 - 45, Update both image constructors
to select the EXIF-specific APP1 marker instead of using the generic
markers.app1 result, ensuring a preceding non-EXIF APP1 marker cannot override
EXIF orientation. Adjust the marker lookup around _App1Marker.from_stream and
add a regression test covering non-EXIF APP1 before EXIF APP1, preserving the
later marker’s orientation.
| pic = CT_Picture.new(pic_id, filename, rId, cx, cy, orientation) | ||
| if orientation.swaps_axes: | ||
| cx, cy = cy, cx |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve requested display dimensions for rotated images.
image.scaled_dimensions() runs before Line 113 swaps the inline extent axes. For ROTATE_90 or another axis-swapping orientation, a width-only request becomes the displayed height. A caller that passes width=W can receive an image with displayed width based on the unrotated height instead.
Resolve orientation before scaling. In StoryPart.new_pic_inline(), swap the requested width and height before calling image.scaled_dimensions() when orientation.swaps_axes is true. Add width-only, height-only, and both-dimensions regression tests.
Proposed fix
- rId, image = self.get_or_add_image(image_descriptor)
- cx, cy = image.scaled_dimensions(width, height)
+ rId, image = self.get_or_add_image(image_descriptor)
if orientation is EXIF_ORIENTATION.AUTO:
orientation = image.orientation
+ if orientation.swaps_axes:
+ width, height = height, width
+ cx, cy = image.scaled_dimensions(width, height)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docx/oxml/shape.py` around lines 112 - 114, Update
StoryPart.new_pic_inline() to account for orientation.swaps_axes before invoking
image.scaled_dimensions(): swap the requested width and height first, then
create the picture and apply the existing inline extent handling without causing
a second effective swap. Add regression coverage for width-only, height-only,
and both-dimensions requests with axis-swapping orientations.
| if orientation is EXIF_ORIENTATION.AUTO: | ||
| orientation = image.orientation | ||
| shape_id, filename = self.next_id, image.filename | ||
| return CT_Inline.new_pic_inline(shape_id, rId, filename, cx, cy) | ||
| return CT_Inline.new_pic_inline(shape_id, rId, filename, cx, cy, orientation) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scale axis-swapping orientations in display coordinates.
When orientation.swaps_axes is true, image.scaled_dimensions(width, height) consumes display dimensions before CT_Inline.new_pic_inline() swaps wp:extent. The returned InlineShape then has the opposite explicit width and height. Resolve AUTO before scaling. Swap the requested dimensions before calling scaled_dimensions(). Preserve the same crossed-axis rule in the InlineShape.width and InlineShape.height setters.
Proposed fix
rId, image = self.get_or_add_image(image_descriptor)
- cx, cy = image.scaled_dimensions(width, height)
if orientation is EXIF_ORIENTATION.AUTO:
orientation = image.orientation
+ if orientation.swaps_axes:
+ width, height = height, width
+ cx, cy = image.scaled_dimensions(width, height)
shape_id, filename = self.next_id, image.filename🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docx/parts/story.py` around lines 75 - 78, Update the picture sizing flow
around CT_Inline.new_pic_inline and the InlineShape width/height setters:
resolve AUTO orientation before scaling, swap the requested width and height
when orientation.swaps_axes is true before calling image.scaled_dimensions(),
and apply the same crossed-axis mapping in both setters so explicit dimensions
remain correct after wp:extent axis swapping.
1 participant
Since this fork is more maintained than the upstream, I will repost here python-openxml#1575
Fixes python-openxml#540
Modern image formats (PNG, JPEG, Tiff, etc) embed the orientation in Exif metadata.
When rotating or displaying an image, software like Word, PowerPoint, macOS Preview, Android Google Photo, etc do not rotate the actual pixel data but instead change the orientation metadata integer (https://exifstrip.com/guides/orientation-tag-explained)
This is governed in OpenXML with the
a:xfrm(transform) tag. This tag can contain therot,flipV, andflipHattributes. By mapping the 1-8 orientation values to the respective rot and flip attributes it's possible to display images that were rotated by merely changing the Exif metadata with the correct orientation.This PR add support for image orientation.
By default it gets orientation from Exif metadata. If orientation metadata is not available it falls back to "Normal" orientation (like now, defined by raw pixel data).
It is also possible to force a given orientation programmatically by:
Summary by CodeRabbit
New Features
Tests