Support DJI distortion params #86
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for DJI distortion parameters and distinguishes between calibrated focal lengths for Sentera and DJI sensors. Key changes include adding a new function calibrated_focal_length(), updating focal_length_pixels() to accommodate the new calibrated focal length, and parsing DJI distortion parameters from XMP data.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_imgparse.py | Removed tests invoking the old use_calibrated flag usage. |
| imgparse/xmp_tags.py | Added DJI distortion tag. |
| imgparse/parser.py | Introduced calibrated_focal_length and updated focal length and distortion methods. |
njwitthoeft
left a comment
njwitthoeft
left a comment
There was a problem hiding this comment.
All looks good.
One small request on the parsing approach for consistency.
Also of note - Sentera returns distortion parameters as k1,k2,k3,p1,p2. I'm glad you're making DJI return consistent with Sentera. I sometimes think we'd be better off changing how we return Sentera results.
Either way, a NamedTuple would help clarify our return order, this order is not consistent with OpenCV and has caused major headaches for me in the past.
| if len(parts) != 2: | ||
| raise ValueError("Invalid dewarp data format: missing semicolon") | ||
|
|
||
| values = [float(v) for v in parts[1].split(",")] |
There was a problem hiding this comment.
This is inconsistent with the list(map(float, str().split()) used below. I think your approach works better.
Could you update the Sentera approach in 298 to be consistent with the list comprehension used here?
There was a problem hiding this comment.
Updated. Also updated the return type to a NamedTuple in OpenCV order
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for parsing DJI distortion parameters and introduces a new function, calibrated_focal_length(), to return the calibrated focal length along with a flag that indicates if conversion from pixels to meters is needed.
- Removed the use_calibrated argument from focal_length_meters() and added a dedicated calibrated_focal_length() method.
- Updated tests to verify new behavior for DJI distortion parameters and focal length calculations.
- Modified XMP tag definitions and added a DistortionParams type to support structured distortion parameter data.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_imgparse.py | Updated tests for focal length and distortion parameters for DJI/Sentera. |
| pyproject.toml | Bumped version from 2.0.4 to 2.0.5. |
| imgparse/xmp_tags.py | Added new DJI distortion tag. |
| imgparse/types.py | Introduced a new NamedTuple, DistortionParams, for structured parameters. |
| imgparse/parser.py | Added calibrated_focal_length(), modified focal_length_pixels(), and updated distortion_parameters() to handle DJI and Sentera formats. |
Comments suppressed due to low confidence (1)
tests/test_imgparse.py:143
- Consider adding tests that directly invoke the new calibrated_focal_length() method for Sentera sensors to ensure correct conversion from mm to meters.
focal2 = sentera_parser.focal_length_meters(use_calibrated=True)
| return list( | ||
| map(float, str(self.xmp_data[self.xmp_tags.DISTORTION]).split(",")) | ||
| ) | ||
| if self.make() == "DJI": |
There was a problem hiding this comment.
Consider adding more detailed documentation on the expected format of DJI distortion data (e.g., semicolon-separated header and comma-separated numeric values) to aid future maintainers.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Support DJI distortion params
What?
Support parsing DJI distortion parameters. As part of this, add a new
calibrated_focal_length()function that returns the calibrated focal length from the xmp if available. The problem is Sentera and DJI store the calibrated focal length differently. Sentera stores it in mm and DJI stores it in pixels. So we also need to return a flag indicating if we need to convert this focal length to pixels or not forfocal_length_pixels(), the preferred function to get the focal length for a given image.Why?
Needed to dewarp DJI images. Mavic 3's don't dewarp out of the box, so customers are flying with the setting off, and we need to support it to avoid reruns. New function to get the calibrated focal length fixes an issue where we were incorrectly scaling the DJI calibrated focal length by the pixel pitch
PR Checklist
Breaking Changes
calibrated_focal_lengthremoved as an argument fromfocal_length_meters()and instead is a new function. Checked and nothing is usingfocal_length_meters()directly, so shouldn't be a breaking change for anyone.