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

wip: uncompressed codec filter-array support #1311

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Sep 15, 2024

For discussion.

This decodes the Filter Array as mono, and it looks roughly right, but clearly its meant to be coloured.
out

gpac interpretation:

gpac_screenshot

Any recommendations for de-Bayering libraries that will work off an explicit matrix and (ideally) relative component gain values?

Should we have a "filter array" data type?

@bradh bradh marked this pull request as draft September 15, 2024 02:25
@@ -268,6 +268,14 @@ uint32_t BitstreamRange::read32()
(buf[3]));
}

float BitstreamRange::readFloat32()
{
int i = read32();
Copy link
Contributor

@farindk farindk Sep 15, 2024

Choose a reason for hiding this comment

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

There is a BitstreamRange::read32_float() function in the timestamp-properties branch. It didn't get merged (yet) and the TAI timestamps changed their use of float to int. It seems that the two implementations are equivalent.

Could we use a reinterpret_cast instead of the memcpy? And uint32_t instead of int.

I checked how endianness works for floating point. It seems that all modern architectures use the same endianness for floats as for integers. The only exception seems to be some ARM processors that use a mixed-endian format for 64bit floats.

@farindk
Copy link
Contributor

farindk commented Sep 15, 2024

Should we have a "filter array" data type?

We may have to rethink how we map the 23001-17 components to libheif data types. I am not convinced anymore about our current approach to use heif_channel for this. The 23001-17 specs say that an image may contain several components of the same type (e.g. several R/G/B and maybe more common: several monochrome). We cannot map this to heif_channel because our image implementation currently can only hold one plane for each channel value.

Thus, we might have to change heif_image to hold an arbitrary set of components and assign the heif_channel like the 23001-17 component type to it. If the image only contains does not contains duplicate heif_channels, the API can continue to work like before, but if there are components with similar type, we will have to address them by component index. The challenge here is how to do this without too much API duplication for using heif_channel vs. component_idx.

How about this:

  • we use heif_channel like 23001-17 component types, just remapped to different numbers.
  • we use heif_channel >= 0x10000 as component indexes (i.e. 0x10003 would be component idx 3).

If we do not want that hack, instead of the second point, we can also introduce new API functions with component_idx instead of heif_channel. In that case, I would change the function signature of all heif_image_get_channel_*() to

float* heif_image_get_component_float32(struct heif_image*,
                                      uint32_t component_index,         // <<< instead of heif_channel
                                      uint32_t* out_stride);

Coming back to your question: yes, "filter array" would then be a heif_channel type.

farindk added a commit that referenced this pull request Sep 15, 2024
@farindk
Copy link
Contributor

farindk commented Sep 15, 2024

Should we have a "filter array" data type?

Can you cherry-pick 7b8545e into this PR-branch?

What should we map the 23001-17 "monochrome" component to? heif_channel_Y is probably a bad idea since that is a separate component type. Is "monochrome" a good name anyway or should it actually be "non_visual"?

@farindk
Copy link
Contributor

farindk commented Oct 4, 2024

Since I'm again shuffling code around, should I merge this now?

This PR is still in the draft status, but I think it is better to merge it now and we can decide later how to handle the channel IDs for "non-visual" channels.

@bradh
Copy link
Contributor Author

bradh commented Oct 4, 2024

I'm OK with it going in. I have no idea how to do the debayering though.

@farindk
Copy link
Contributor

farindk commented Oct 4, 2024

Did you see this? I have not tried it yet. It seems to only support a couple of standard patterns:
https://github.com/saronic-technologies/libdebayer

Probably we could do debayering in the color conversion pipeline like a regular color transformation.
However, to keep the footprint low for people that do not need it, I imagine extending the plugin system to also color-transformations. Then more advanced algorithms could be added on-demand.

@farindk
Copy link
Contributor

farindk commented Oct 4, 2024

Could you send me your example image?

@farindk farindk merged commit c9e8350 into strukturag:master Oct 4, 2024
34 of 35 checks passed
@bradh bradh deleted the filter_array_2024-09-15 branch October 5, 2024 02:44
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.

2 participants