RPP JPEG compression HOST : Resolve review comments#426
Merged
r-abishek merged 1 commit intor-abishek:ar/opt_jpeg_distortion_hostfrom Apr 25, 2025
Merged
Conversation
Collaborator
HazarathKumarM
commented
Apr 25, 2025
- Resolved the review comments
There was a problem hiding this comment.
Pull Request Overview
This PR resolves review comments related to JPEG compression for HOST and refines several DCT and quantization routines, along with minor documentation updates. Key changes include:
- Replacing hardcoded test case boundaries with dynamic values from imageAugmentationMap in runImageTests.py.
- Refactoring DCT, quantization, and inverse DCT routines in jpeg_compression_distortion.cpp, including the introduction of helper functions clamp() and get_mul_add().
- Updating function documentation in rppt_tensor_geometric_augmentations.h regarding the JPEG quality factor range.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| utilities/test_suite/HOST/runImageTests.py | Switched to dynamic case boundary based on imageAugmentationMap keys. |
| src/modules/tensor/cpu/kernel/jpeg_compression_distortion.cpp | Refactored DCT/IDCT implementations with added helper functions and enhanced inline documentation. |
| api/rppt_tensor_geometric_augmentations.h | Updated parameter documentation for JPEG quality factor. |
Comments suppressed due to low confidence (1)
src/modules/tensor/cpu/kernel/jpeg_compression_distortion.cpp:180
- Ensure that the refactored dct_8x8_1d_core logic—with the reduced temporary storage and reordered computations—preserves the mathematical properties of the original DCT. Consider adding unit tests to validate the transformation.
T temp[8];
Comment on lines
+42
to
+43
| caseMin = min(imageAugmentationMap.keys()) | ||
| caseMax = max(imageAugmentationMap.keys()) |
There was a problem hiding this comment.
Before using min() and max() on imageAugmentationMap.keys(), verify that imageAugmentationMap is non‐empty to prevent potential runtime exceptions.
Suggested change
| caseMin = min(imageAugmentationMap.keys()) | |
| caseMax = max(imageAugmentationMap.keys()) | |
| if imageAugmentationMap: | |
| caseMin = min(imageAugmentationMap.keys()) | |
| caseMax = max(imageAugmentationMap.keys()) | |
| else: | |
| raise ValueError("imageAugmentationMap is empty. Cannot determine caseMin and caseMax.") |
r-abishek
reviewed
Apr 25, 2025
| template <typename T> | ||
| inline T clamp(T val, T minVal, T maxVal) | ||
| { | ||
| return (val < minVal) ? minVal : (val > maxVal) ? maxVal : val; |
Owner
There was a problem hiding this comment.
If this is the most optimal, this is a very generic function that need not be inside jpeg_compression_distortion.cpp specifically. Could be outside so other functions could use it. Can be changed later.
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
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.