Skip to content
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

Update library to use definitions of ultrahdr_api everywhere #176

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

ram-mohan
Copy link
Contributor

@ram-mohan ram-mohan commented Jun 17, 2024

This is a major change but maintains bitexactness with previous commit. This change unifies legacy structure definitions with definitions of ultrahdr_api.h. This helps for better extensibility for new features and avoid redundancy.

Legacy structures are moved to ultrahdr.h. These are deprecated and only retained for backward compatibility.

Briefly,

  • unify ultrahdr_color_gamut with uhdr_color_gamut_t
  • unify ultrahdr_transfer_function with uhdr_color_transfer_t
  • unify ultrahdr_metadata_struct with uhdr_gainmap_metadata_t
  • unify jpegr_uncompressed_struct with uhdr_raw_image_t
  • unify jpegr_compressed_struct with uhdr_compressed_image_t
  • unify jpegr_exif_struct with uhdr_mem_block_t
  • unify status_t with uhdr_error_info_t
  • Deprecate ultrahdr_output_format
  • Added methods to Jpeg*Helper to simplify data translation between helper
    and its users

Bug fix,

  • For images with multi channel gainmap, decoded gainmap is not copied
    completely for getter functions. This is fixed.
  • Add support for tonemapping linear transfer inputs
  • Fixes oss-fuzz: 69287

Test: ./ultrahdr_unit_test
Test: ./ultrahdr_dec_fuzzer
Test: ./ultrahdr_enc_fuzzer

@ram-mohan ram-mohan force-pushed the unify branch 6 times, most recently from 76efcf6 to be54923 Compare June 17, 2024 20:52
@DichenZhang1
Copy link
Collaborator

Thank you for the change, but we may need to hold on for a little bit. jpegr.h layer is currently used by Android, and this change will also need change in Android and it's out of scope now. We will hold this change for a while and land it with v2 version.

@DichenZhang1
Copy link
Collaborator

I would recommend separate this change land the bug fixing part now, and hold the API change part. Let me know what you think. Thank you!

@ram-mohan
Copy link
Contributor Author

ram-mohan commented Jun 18, 2024

Thank you for the change, but we may need to hold on for a little bit. jpegr.h layer is currently used by Android, and this change will also need change in Android and it's out of scope now. We will hold this change for a while and land it with v2 version.

This change does not effect android builds. The android builds works normally. It seems recent sample app changes are causing build issues in android. These were corrected in this commit.

I would recommend separate this change land the bug fixing part now, and hold the API change part. Let me know what you think. Thank you!

Some fixes work with the changes made. I will try to seperate independent one's and give a pull request.

@DichenZhang1
Copy link
Collaborator

OK Thanks, I'll take a further look

@ram-mohan ram-mohan force-pushed the unify branch 8 times, most recently from 6efe7b8 to 173a942 Compare June 23, 2024 21:37
@DichenZhang1
Copy link
Collaborator

Thank you for the change, but we may need to hold on for a little bit. jpegr.h layer is currently used by Android, and this change will also need change in Android and it's out of scope now. We will hold this change for a while and land it with v2 version.

This change does not effect android builds. The android builds works normally. It seems recent sample app changes are causing build issues in android. These were corrected in this commit.

I would recommend separate this change land the bug fixing part now, and hold the API change part. Let me know what you think. Thank you!

Some fixes work with the changes made. I will try to seperate independent one's and give a pull request.

OK then we are having two interfaces of the public APIs. I'm not against with this move, but I'm feeling this is not an urgent need. And additionally this change is fairly too large and I would still recommend separating this change and we'll merge the bug fixing part first. Let me know what you think. thank you!

@ram-mohan
Copy link
Contributor Author

OK then we are having two interfaces of the public APIs.

As the interface is from ultrahdr_api.h, there is only one. But if we were to include jpegr.h directly then as you have rightly pointed out, there are aliases for each api.

I'm not against with this move, but I'm feeling this is not an urgent need. And additionally this change is fairly too large and I would still recommend separating this change and we'll merge the bug fixing part first.

Certain fixes that are independent of this change, i have moved them out and issued pull requests. #178 #179 #180
But some fixes where a little more coupled with this change (oss-fuzz 69287 and returning decoded gainmap). So i did not move them. Also, some parts of this change simplifies handling 444 and 422 support.

When I use the attached sources, I get error "encountered unknown error during decoding" (exit code 255).
I assume this is related to the JPG, though it does not say which file decoding failed (would be nice to update the error message to be more clear there).

It also addresses this.

Some portions of this change are not at all a priority and can be marked as good to have and some other parts are helpful towards addressing known issues.

@DichenZhang1
Copy link
Collaborator

OK my only concern of this change is about having aliases at jpegr.cpp layer, but indeed this change makes the code unified and clean, and has benefit of simplifying 444 and 422 color format. I've approved this change. Thank you for work!

examples/ultrahdr_app.cpp Outdated Show resolved Hide resolved
- Add error checks for input params of newly added setter functions
- update documentation of sample app for newly added options
- fix sample app build issues in aosp
- communicate error correctly if write to output has failed
- update gainmap scale factor if its too large

Test: ./ultrahdr_unit_test
This is a major change but maintains bitexactness with previous commit.
This change unifies legacy structure definitions with definitions of
ultrahdr_api.h. This helps for better extensibility for new features and
avoid redundancy.

Legacy structures are moved to ultrahdr.h. These are deprecated and only
retained for backward compatibility.

Briefly,
- unify ultrahdr_color_gamut with uhdr_color_gamut_t
- unify ultrahdr_transfer_function with uhdr_color_transfer_t
- unify ultrahdr_metadata_struct with uhdr_gainmap_metadata_t
- unify jpegr_uncompressed_struct with uhdr_raw_image_t
- unify jpegr_compressed_struct with uhdr_compressed_image_t
- unify jpegr_exif_struct with uhdr_mem_block_t
- unify status_t with uhdr_error_info_t
- Deprecate ultrahdr_output_format
- Added methods to Jpeg*Helper to simplify data translation between helper
  and its users
- Improved error propogation across library

Bug fix,
- For images with multi channel gainmap, decoded gainmap is not copied
  completely for getter functions. This is fixed.
- Add support for tonemapping linear transfer inputs
- Fixes oss-fuzz: 69287

Test: ./ultrahdr_unit_test
Test: ./ultrahdr_enc_fuzzer
Test: ./ultrahdr_dec_fuzzer
@DichenZhang1 DichenZhang1 merged commit c9c8c74 into google:main Jun 27, 2024
11 checks passed
@ram-mohan ram-mohan deleted the unify branch June 27, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants