-
Notifications
You must be signed in to change notification settings - Fork 584
EncodedDepthImage archetype and integration with viewer and re_mcap #11877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for opening this pull request.
Because this is your first time contributing to this repository, make sure you've read our Contributor Guide and Code of Conduct.
|
Hi @oxkitsune , I believe I am comfortable with the prototype, the proof concept works well with lint and my local mcap bag with RVL-encoded depth image. I am ready to submit a separate PR for EncodedDepthImage archetype. However, I am not very confident about my changes in If you are happy with the proof-of-concept demo and the design of the EncodedDepthImage archetype, I can prepare the first PR for EncodedDepthImage archetype. |
|
This demo looks great, thanks for putting it together! Go ahead and open the PR for the |
|
According to our discussion, we will not split this PR into small PRs. There are a few decisions we need to make:
|
|
After discussing with @oxkitsune
|
| pub use ros_rvl::{ | ||
| RvlDecodeError, RvlMetadata, decode_ros_rvl_f32, decode_ros_rvl_u16, parse_ros_rvl_metadata, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the module's already pub, I think it's neater to avoid re-exporting. This means that in the future, when adding more formats, we can easily access it using re_depth_compression::<format>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| PayloadLengthMismatch { width: u32, height: u32 }, | ||
| } | ||
|
|
||
| fn parse_ros_rvl_metadata(data: &[u8]) -> Result<RvlMetadata, RvlDecodeError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this is the same metadata parsing from crates/viewer/re_viewer_context/src/utils/depth.rs right? Is there any way we can avoid duplicating this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, refactored into re_depth_compression
| format.datatype() | ||
| ))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be a good place to validate that the decoded_format is actually single channel data. This now support RGB pngs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we have enforced single-channel PNG
| // Read width and height from bytes 12-15 and 16-19 respectively | ||
| let width = u32::from_le_bytes([buf[12], buf[13], buf[14], buf[15]]); | ||
| let height = u32::from_le_bytes([buf[16], buf[17], buf[18], buf[19]]); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the structure given above, I think that this, in its current state, will lead to a lot of false positives.
I think we should also check that depth_quant_a and depth_quant_b are both valid floats with sane ranges (e.g. not NaN or inf and not absurdly large)
| let depth_quant_a = f32::from_le_bytes([buf[4], buf[5], buf[6], buf[7]]); | |
| let depth_quant_b = f32::from_le_bytes([buf[8], buf[9], buf[10], buf[11]]); | |
| // Reject NaNs/infs | |
| if !depth_quant_a.is_finite() || !depth_quant_b.is_finite() { | |
| return false; | |
| } | |
| // Reject insane magnitudes | |
| if depth_quant_a <= 0.0 || depth_quant_a > 1e4 || depth_quant_b.abs() > 1e4 { | |
| return false; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| #[test] | ||
| fn test_guess_from_data() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of this test is for MediaType::rvl(), which I think is fine. No need to test the GLB/stl here in my opinion. Test can also be renamed to test_guess_from_data_rvl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| /// Image format (width, height, datatype). | ||
| /// | ||
| /// Unlike generic [archetypes.EncodedImage], most depth codecs do not carry full headers, | ||
| /// so width/height/datatype must be logged explicitly. | ||
| format: rerun.components.ImageFormat ("attr.rerun.component_required", order: 1100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Image format (width, height, datatype). | |
| /// | |
| /// Unlike generic [archetypes.EncodedImage], most depth codecs do not carry full headers, | |
| /// so width/height/datatype must be logged explicitly. | |
| format: rerun.components.ImageFormat ("attr.rerun.component_required", order: 1100); | |
| /// Image format (width, height, datatype). | |
| /// | |
| /// Standard image formats like PNG or JPEG include this metadata in their headers, | |
| /// allowing [archetypes.EncodedImage] to be self-describing. Depth images, however, | |
| /// typically use headerless codecs, so this information must be provided explicitly. | |
| format: rerun.components.ImageFormat ("attr.rerun.component_required", order: 1100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
crates/store/re_types/src/archetypes/encoded_depth_image_ext.rs
Outdated
Show resolved
Hide resolved
| bytes | ||
| } | ||
|
|
||
| fn encode_rvl(values: &[u16]) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you essentially implement a full encoder here as well, maybe it makes sense to just bundle the decoder + encoder up in a separate rust crate, so that it can be used by others as well?
Just a suggestion as I was surprised to see an encoder here as well 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored the parsing/encoding/decoding logic back to re_depth_compression crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can see, this is 99% the same as the depth_images visualizer. The main difference is the component identifiers (e.g. DepthImage::meter() vs EncodedDepthImage::meter()).
Can we move the implementation to a unified function that takes the component identifiers?
improve RVL media-type detection via the shared parser plus quantization sanity checks, trimmed the guess-from-data test to RVL coverage, and fixed the RVL doc link. improve depth decoding: enforce single-channel PNG depth, error on RVL/ImageFormat resolution mismatches, and validate decoded byte counts.
…mat and 1000.0 for integer format
…ion helper and remove duplicated rendering logic
Related
EncodedDepthImagearchetype #9046What
This is a draft PR to provide prototype for EncodedDepthImage. This PR needs to be split into sub-PRs once reviewed by @oxkitsune