google / libultrahdr

Ultra HDR is a true HDR image format, and is backcompatible. libultrahdr is the reference codec for the Ultra HDR format. The codecs that support the format can render the HDR intent of the image on HDR displays; other codecs can still decode and display the SDR intent of the image.
https://developer.android.com/guide/topics/media/platform/hdr-image-format
Apache License 2.0
160 stars 28 forks source link

Update library to use definitions of ultrahdr_api everywhere #176

Closed ram-mohan closed 4 months ago

ram-mohan commented 4 months ago

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,

Bug fix,

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

DichenZhang1 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

OK Thanks, I'll take a further look

DichenZhang1 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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!