architecture-samples icon indicating copy to clipboard operation
architecture-samples copied to clipboard

## Code Review

Open Mdrieaz opened this issue 5 months ago • 1 comments

Code Review

This pull request effectively introduces support for HEIC UltraHDR image capture. The approach of extending the existing Camera2UltraHDRCapture class is sound, and the necessary API changes (like using CaptureRequest.JPEG_ORIENTATION instead of EXIF manipulation for HEIC) are correctly implemented. The SDK version updates and new sample demo registration are also appropriate.

Overall, the changes are well-done and enhance the camera sample capabilities. I have one suggestion for improving the robustness of the file creation logic for future extensions.

Summary of Findings

  • File Extension Handling in createFile: The createFile method determines file extensions based on the image format. The current else case defaults to .heic, which is fine for this PR but could be problematic if other non-HEIC UltraHDR formats are supported in the future via subclassing. A more explicit handling of known formats and a clearer strategy for unknown ones would improve robustness. (Commented with medium severity)
  • Copyright Year: The new file samples/camera/camera2/src/main/java/com/example/platform/camera/imagecapture/Camera2HeicUltraHDRCapture.kt has a copyright year of 2025. Typically, this is the year of creation (e.g., 2024) or matches the project's existing files (e.g., Camera2UltraHDRCapture.kt uses 2023). Please verify if 2025 is intentional or a typo. (Not commented due to review settings: low severity)
  • KDoc Documentation: Consider adding KDoc comments to the new class Camera2HeicUltraHDRCapture and its overridden ULTRAHDR_FORMAT property. Also, the newly open base class Camera2UltraHDRCapture and its ULTRAHDR_FORMAT property could benefit from KDocs explaining their roles and extensibility. This would improve code clarity and maintainability. (Not commented due to review settings: low severity)

Merge Readiness

The pull request is in good shape and introduces a valuable feature. I recommend addressing the medium-severity comment regarding file extension handling in createFile to enhance future maintainability. Once that's considered, the PR should be ready for merging. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members.

Originally posted by @gemini-code-assist[bot] in https://github.com/android/platform-samples/pull/310#pullrequestreview-2901815087

Mdrieaz avatar Jul 14 '25 11:07 Mdrieaz

Hi @Mdrieaz 👋

Thanks for the thorough code review and feedback!

Acknowledged:

The suggestion to make createFile() more robust for future non-HEIC formats makes sense. I’ll refactor it to use explicit format-to-extension mapping.

I’ll double-check and correct the copyright year.

Will also consider adding KDoc comments to the new and modified classes for better clarity and extensibility.

Appreciate the thoughtful review — I’ll push an updated commit shortly.

— Assets

VaradGupta23 avatar Jul 15 '25 12:07 VaradGupta23