Skip to content

PR: Add aliases for ITU-T H.273 video color metadata #987

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
Merged
merged 4 commits into from
Aug 13, 2022
Merged

PR: Add aliases for ITU-T H.273 video color metadata #987

merged 4 commits into from
Aug 13, 2022

Conversation

fxthomas
Copy link
Contributor

@fxthomas fxthomas commented May 18, 2022 •
edited
Loading

Summary

This implements what's discussed in #982.

I tried to stick as close to the original ISO standard as possible, but there are a few caveats:

  • Some sets of primaries/whitepionts as well as transfer functions were not already present in colour ; I implemented them here based on the ISO standard in this module and added the necessary references.
  • The ISO standard is not very clear on the values of alpha, beta and gamma for ITU-R BT.1361, so I chose instead to implement the OETF based on that standard.
  • I'm not very certain of how I should implement EOTFs/OETFs when the range is not [0-1] (e.g. BT.1361, which has an extended range). Is there any consensus on that? How is it usually documented?
  • I linked the RGB <-> XYZ matrices from the original module in colour even though they are not defined explicitly in the ISO standard, just because it makes things more consistent with how the original modules in colour.models.rgb.datasets work. Let me know if this is not needed.

Preflight

Personal todo

  • Verify that ISO 23001-8:2013 aligns with ISO 23091-2
  • Verify that ISO 23001-8:2013 aligns with ITU-T H.273
  • Verify that ISO 14496-10:2020 aligns with the rest
  • Finish updating the doctests/examples
  • Finish adding typing hints

Code Style and Quality

  • Unit tests have been implemented and passed.
  • Mypy static checking has been run and passed.
  • Pre-commit hooks have been run and passed.
  • New transformations have been added to the Automatic Colour Conversion Graph.
  • New transformations have been exported to the relevant namespaces, e.g. colour, colour.models.

Documentation

  • New features are documented along with examples if relevant.
  • The documentation is Sphinx and numpydoc compliant.

@KelSolaar KelSolaar added this to the v0.4.2 milestone May 20, 2022
@KelSolaar KelSolaar changed the title Add aliases for ISO/IEC 23001-8 video color metadata PR: Add aliases for ISO/IEC 23001-8 video color metadata May 20, 2022
@KelSolaar
Copy link
Member

Some sets of primaries/whitepionts as well as transfer functions were not already present in colour ; I implemented them here based on the ISO standard in this module and added the necessary references.

I think we will probably want every single one in its own module for consistency, will make surfacing them into the API easier too.

I'm not very certain of how I should implement EOTFs/OETFs when the range is not [0-1] (e.g. BT.1361, which has an extended range). Is there any consensus on that? How is it usually documented?

For some of them we have in_normalised_code_value / out_normalised_code_value kwargs that perform full to legal / legal to full conversion.

@fxthomas
Copy link
Contributor Author

I think we will probably want every single one in its own module for consistency, will make surfacing them into the API easier too.

Okay, will do that.

For some of them we have in_normalised_code_value / out_normalised_code_value kwargs that perform full to legal / legal to full conversion.

I think i will need to look into it a bit more to be sure of what I'm doing, because technically these are beyond-full-range if I understand what their extended range means. This is similar to transfer characteristics that accept a floats in a [-1, 1] range, technically they're not "full-range", are they?

I'll work on that PR as soon as I get some time, whose availability was pretty scarce the last couple of weeks ;)

@fxthomas fxthomas force-pushed the 982-ffmpeg-iso-23001-8-video-metadata-code-points branch from 7035b06 to 2d0aa2e Compare June 19, 2022 19:37
@fxthomas fxthomas changed the title PR: Add aliases for ISO/IEC 23001-8 video color metadata PR: Add aliases for ITU-T H.273 video color metadata Jun 19, 2022
@fxthomas
Copy link
Contributor Author

fxthomas commented Jun 19, 2022 •
edited
Loading

Okay, should be good for review, I think.

  • As discussed within the linked issue, the main definitions reference ITU-T H.273, since it's the most accessible standard for these metadata.

  • The color.models.rgb.video module contains only mappings and all the other TFs and primaries are defined in new modules in color.models.rgb.datasets/transfer_functions when needed.

  • I've tried to be as precise as possible with the references ; for those I don't have direct access to (IEC 61966 and ST 428) I added a note in the docstring specifying that I used the ITU-T H.273 definition, hopefully somebody will later come along and check that what I did is indeed the same as what's defined in the referenced standard.

@fxthomas fxthomas force-pushed the 982-ffmpeg-iso-23001-8-video-metadata-code-points branch 3 times, most recently from 0bb8a43 to ffe668c Compare June 20, 2022 18:45
@fxthomas
Aliases for ITU-T H.273 video metadata (colour-science#982)
e53af2e 
This commit adds additional transfer function and primaries defined or
referenced to by ITU-T H.273, as well as aliases to their corresponding
video metadata tag values (ColourPrimaries, MatrixCoefficients and
TransferCharacteristics).
@fxthomas fxthomas force-pushed the 982-ffmpeg-iso-23001-8-video-metadata-code-points branch from ffe668c to e53af2e Compare June 20, 2022 19:52
@KelSolaar
Copy link
Member

Thanks @fxthomas! I will try to look at your PR this week. I'm trying to boil the ocean!

@KelSolaar
Copy link
Member

Hi @fxthomas,

Thanks for the great work! I think I will probably merge it in a staging branch as it requires a polishing pass! Something that would be really helpful would be if you could implement the inverse of the new CCTF you added please?

Cheers,

Thomas

@fxthomas
Copy link
Contributor Author

fxthomas commented Aug 7, 2022

@KelSolaar Sure! I added the inverses for all the new OETFs. Also when checking ITU-R BT.1361 I found a typo in the earlier commits (I had swapped the two lines in the np.where), which I also corrected.

Let me know if you need anything else.

@KelSolaar
Copy link
Member

This is awesome, thanks a lot @fxthomas, I will review that over the weekend!

@KelSolaar KelSolaar changed the base branch from develop to feature/itu_t_h273 August 13, 2022 06:09
@KelSolaar
Copy link
Member

@fxthomas : I will merge in a staging branch and varnish things a bit, thanks a lot!

@KelSolaar KelSolaar merged commit 92f0c64 into colour-science:feature/itu_t_h273 Aug 13, 2022
@KelSolaar KelSolaar mentioned this pull request Aug 21, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KelSolaar KelSolaar

@zachlewis zachlewis

@nick-shaw nick-shaw

Labels
None yet
Projects
None yet
Milestone
v0.4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@fxthomas @KelSolaar @zachlewis @nick-shaw