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.

Sign up for GitHub

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

Jump to bottom

Add experimental gain map support #1121

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add experimental gain map support #1121

wants to merge 7 commits into from

Conversation

DichenZhang1
Copy link
Contributor 

This feature is hidden behind the WITH_EXPERIMENTAL_GAIN_MAP compile flag.

See https://helpx.adobe.com/camera-raw/using/gain-map.html for an introduction
to gain maps. Note that in this change, gain maps are not stored as auxiliary
items as proposed by Adobe, but as a hidden input to a new 'tmap' (tone mapped)
derived item.

An 'altr' (alternatives group) box is also added to tell the decoder that it
may show the tone mapped image (preferrably) or the base image as a fall back.
The primary item id still points to the base image for backward compatibility.

DichenZhang1 and others added 3 commits February 2, 2024 00:22
@DichenZhang1
Add experimental gain map support
afa4eb1 
    This feature is hidden behind the WITH_EXPERIMENTAL_GAIN_MAP compile flag.

    See https://helpx.adobe.com/camera-raw/using/gain-map.html for an introduction
    to gain maps. Note that in this change, gain maps are not stored as auxiliary
    items as proposed by Adobe, but as a hidden input to a new 'tmap' (tone mapped)
    derived item.

    An 'altr' (alternatives group) box is also added to tell the decoder that it
    may show the tone mapped image (preferrably) or the base image as a fall back.
    The primary item id still points to the base image for backward compatibility.
@DichenZhang1
Add experimental gain map support
cf37ae7 
    This feature is hidden behind the WITH_EXPERIMENTAL_GAIN_MAP compile flag.

    See https://helpx.adobe.com/camera-raw/using/gain-map.html for an introduction
    to gain maps. Note that in this change, gain maps are not stored as auxiliary
    items as proposed by Adobe, but as a hidden input to a new 'tmap' (tone mapped)
    derived item.

    An 'altr' (alternatives group) box is also added to tell the decoder that it
    may show the tone mapped image (preferrably) or the base image as a fall back.
    The primary item id still points to the base image for backward compatibility.
@DichenZhang1
Update heif.cc
3fe697a
@DichenZhang1
Copy link
Contributor Author

Hi there!

Can someone help to review this pull request? This is adding gain map support for HEIC and AVIF images.

Thank you!

@DichenZhang1
Copy link
Contributor Author

Hi there!

This change is adding support for encoding and decoding HEIC and AVIF with gain map images. Could anyone help to take a look and provide feedback?

Thank you!

@DichenZhang1
Copy link
Contributor Author

Friendly bumping up again. Thank you!

@farindk
Copy link
Contributor

farindk commented Apr 16, 2024 •
edited
Loading

Thank you for the contribution. It generally looks good.

The GainMapMetadata is currently a private data class, but also exposed in the API.
I guess that this data should also be public.
In that case, we should rename this to follow the usual naming scheme (like heif_gain_map_metadata), make it a POD struct and move it to a public header. We can add a new header file (e.g. heif_hdr.h) where we collect HDR related things so that heif.h is not getting too long.

@farindk
Copy link
Contributor

farindk commented Apr 16, 2024

Is this standardized anywhere? Could you point me to the specification?

The Adobe document says:

Gain Maps are currently undergoing standardization in ISO/TC 42 Photography. Furthermore, various
technical committees have ongoing efforts to standardize Gain Map storage in different file formats.
Consequently, the original Gain Map storage proposals have been removed from this document to avoid
confusion and conflicting information.

Does that mean that there is no final spec yet? We can include it as "experimental" anyway, but have to make it very clear that the spec is not finalized yet.

Do you have any kind of test program? Maybe a patched heif_convert that I could use to test this?

@DichenZhang1
Copy link
Contributor Author

Hi Dirk,

Thank you for looking into this!

I've changed the GainMapMetadata name to heif_gain_map_metadata and made it public. The spec is not finalized yet, and I'll keep monitoring the updates. For verification, I've added some code in heif_enc and heif_info and they can be a closed loop for encoding and decoding. But we only have a pseudo gain map metadata (all values are zeros) and ideally this should be an input from user.

Thank you!

farindk
farindk reviewed Jun 22, 2024
View reviewed changes
libheif/heif.h
// Get id of the gain map image of the HEIF file. If no gain map image is available, this
// method will return heif_suberror_No_item_data.
LIBHEIF_API
struct heif_error heif_context_get_gain_map_image_ID(struct heif_context* ctx, heif_item_id* id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume there can be several gain maps in the file. We should add the color image ID as another parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the two functions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is intended that we only support one gain map at most, and that gain map is for the primary image. Assume if we have multiple color images and multiple gain maps, and it is likely that users accidentally links one gain map to another gain map, and we can't detect that case.

But I agree that the standard is still in draft stage and this is not clearly defined. I think we can hold this for a little while and still mark this gain map support as "experimental".

farindk
farindk reviewed Jun 22, 2024
View reviewed changes
libheif/context.cc
@@ -517,6 +517,18 @@ static bool item_type_is_image(const std::string& item_type, const std::string&
item_type == "mski");
}

#if WITH_EXPERIMENTAL_GAIN_MAP
static bool item_type_is_gain_map_image(const std::string& item_type, const std::string& item_name) {
return (item_name == "GMap" &&
Copy link
Contributor

@farindk farindk Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that images are not using a GMap item_name.

In issue #1190, there is an image with a gain map, but the items do not have the GMap name. Instead, the gain map is linked with an auxl reference with type urn:com:apple:photo:2020:aux:hdrgainmap in the auxC property. Thus, we might have to check that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is old legacy Apple GainMap HDR from iPhone 12+ heif images, and is already present pre-iOS 18 (it is present in my heic photos that I believe are from iOS 15, iPhone 13 mini).

While it would obviously be nice if libheif supported it as well, I do not think it has anything to do with the upcoming ISO standard.

@kmilos kmilos mentioned this pull request Aug 30, 2024
Open
@lns103 lns103 mentioned this pull request Nov 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PetrVys PetrVys PetrVys left review comments

@farindk farindk farindk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DichenZhang1 @farindk @PetrVys