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

Additional color transfers #81

Merged
merged 6 commits into from
Oct 20, 2017

Conversation

kodawah
Copy link
Contributor

@kodawah kodawah commented Sep 18, 2017

Hi,
I've added a few color transfer characteristics used in some old files.
Please let me know if you would like me to change anything. In particular I'd like to know your opinion on how I implemented the log transfers.
Thanks

Copy link
Owner

@sekrit-twc sekrit-twc left a comment

Choose a reason for hiding this comment

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

xvYCC requires more thought than merely adding some equations. The obvious issue is that it produces linear RGB values outside of [0.0,1.0], which is not handled correctly by the LUT implementations. There is also the semantic issue of what it means to convert (RGB, xvYCC, 709) to (RGB, 709, 709), given that xvYCC uses the same non-linearity as Rec.709.

In addition, there is the issue of scene vs. display-referred grading at play. The question being whether xvYCC should be handled by inverting a symmetrical BT.1886 (pure-power) function, or if the reflected Rec.709 OETF should be used in all cases.

In summary, I can not merge this commit without test data and industry best-practice guidelines. Since xvYCC is a defunct SONY initiative, such information may be difficult to obtain.

x = SMPTE_240M_ALPHA * zimg_x_powf(x, 0.45f) - (SMPTE_240M_ALPHA - 1.0f);

return x;
}
Copy link
Owner

@sekrit-twc sekrit-twc Sep 19, 2017

Choose a reason for hiding this comment

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

SMPTE 240M, like all other legacy gamma functions, is an OETF. The EOTF should be BT.1886.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good point, should I just reflect that in the function name or is there something else I am missing?

Copy link
Owner

Choose a reason for hiding this comment

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

The function name should be updated, and in select_transfer_function, the OETF should only be returned if scene_referred is true. See the other gamma functions.

Copy link
Owner

Choose a reason for hiding this comment

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

Attached.

s240m.pdf

@sekrit-twc
Copy link
Owner

I can not review the log or gamma 2.2/2.8 commits without real test data or best-practice guidelines.

@@ -140,6 +140,26 @@ float smpte_240m_inverse_eotf(float x) noexcept
return x;
}

float log10_eotf(float x) noexcept
{
if (x < 0.01f)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is the threshold 0.01f? Similarly, why does 0 map to 0.01f in the inverse? That causes non-monotonic behaviour at the next greatest value.

Copy link
Contributor Author

@kodawah kodawah Sep 19, 2017

Choose a reason for hiding this comment

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

It is defined as such in the specifications. This is for log10 but log316 is similar

V=1.0+Log10(Lc)÷2 for 1 >= Lc >= 0.01
V=0.0 for 0.01>Lc >= 0

I mapped 0 -> 0.01 in the inverse because I thought it was the right thing to do, but I was unsure as well.

@kodawah kodawah force-pushed the additional_color_transfers branch from 94bf39f to e44523d Compare September 19, 2017 09:58
@kodawah
Copy link
Contributor Author

kodawah commented Sep 19, 2017

thanks for the review, I should have some samples I can share for most of the transfers.
I wasn't aware of the added complexity of xvYCC, however I know it's unfortunately still used by some screen recording software (which record the screen transfer afaict).

I fixed the build while I prepare the samples in the meantime

@kodawah kodawah force-pushed the additional_color_transfers branch from e44523d to 782bfe5 Compare September 19, 2017 14:40
@sekrit-twc
Copy link
Owner

Can you provide a reference for "screen recording software" that uses xvYCC? I know FRAPS and such record a full-range YUV signal, but that is not the same as xvYCC. xvYCC specifically refers to the SONY method of increasing the gamut by using the 0-15 and 236-255 bands to encode negative and super-positive colors.

@sekrit-twc
Copy link
Owner

@kodabb Were you able to find the test content?

@kodawah
Copy link
Contributor Author

kodawah commented Sep 29, 2017

hi sorry, got busy while sorting them out, I'll be able to provide them within next week

@kodawah kodawah force-pushed the additional_color_transfers branch from 782bfe5 to 7cc84f2 Compare October 3, 2017 22:01
@kodawah
Copy link
Contributor Author

kodawah commented Oct 3, 2017

updated code to fix function naming and oetf selection

@@ -250,6 +260,10 @@ TransferFunction select_transfer_function(TransferCharacteristics transfer, doub
func.to_gamma_scale = 1.0f;

switch (transfer) {
case TransferCharacteristics::LOG_100:
func.to_linear = scene_referred ? log100_inverse_oetf : rec_1886_eotf;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think Rec.1886 is correct for LOG-encoded images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm you're right, the colours do not appear correctly
do you think the log transfers could be describing EOTFs instead?

Copy link
Owner

Choose a reason for hiding this comment

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

Could be that there is no OETF/EOTF distinction for log.

@@ -134,6 +137,26 @@ float srgb_inverse_eotf(float x) noexcept
return x;
}

float xvycc_eotf(float x) noexcept
{
Copy link
Owner

Choose a reason for hiding this comment

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

xvYCC should be implemented as a mirrored Rec.709 OETF. In other words, copysign(rec709_oetf(fabs(x)), x).

Copy link
Owner

Choose a reason for hiding this comment

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

It is also still unclear if the standard gamut portion of xvYCC should be handled with Rec.1886, which is what would occur if the signal were to reach a non-xv TV.

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 tried using rec 709 but the colors when not display correctly with quicktime. I had to use the pure power bt1886 instead.

Regarding the last point we could maybe introduce a new range type? This is what the specs say on that regard

Otherwise (TransferCharacteristics is equal to 11 (IEC 61966-2-4) or 12 (Rec. ITU-R BT.1361 extended colour gamut system)), E′R, E′G and E′B are analogue with a larger range not specified in this part of ISO/IEC 23001.

or maybe let the applications deal with it?

Copy link
Owner

@sekrit-twc sekrit-twc Oct 5, 2017

Choose a reason for hiding this comment

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

Can you elaborate on how you used QuickTime to test this? The problem with using BT.1886 instead of Rec.709 outside of the [0-1] range, i.e. the extended gamut, is that it affects the gamut width. For example, 1.2^2.2 (BT.709) is less saturated than 1.2^2.4 (BT.1886). The behavior needs to be compared to a reference xvYCC implementation.

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 converted a video containing xvycc with zimg and made sure colors matches the ones of the original video, can post pics if needed.

@@ -120,6 +120,16 @@ float rec_1886_inverse_eotf(float x) noexcept
return x < 0.0f ? 0.0f : zimg_x_powf(x, 1.0f / 2.4f);
}

float log100_oetf(float x) noexcept
{
return x < 0.01f ? 0 : 1.0f + zimg_x_logf(x) / 2.0f;
Copy link
Owner

@sekrit-twc sekrit-twc Oct 4, 2017

Choose a reason for hiding this comment

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

Isn't logf the base-e logarithm? Based on the threshold of 0.01, it seems that the base-10 log10f needs to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm logf should be natural log, you're right log10 should be used, thanks for spotting

@kodawah
Copy link
Contributor Author

kodawah commented Oct 4, 2017

i'm getting the feeling that gamma22 and gamma28 are eotf only, i have wrong colors with the scene_referred thing

@kodawah kodawah force-pushed the additional_color_transfers branch from 7cc84f2 to ff8e35b Compare October 4, 2017 23:36
@sekrit-twc
Copy link
Owner

How do you determine if something has the right or wrong colors?

@kodawah
Copy link
Contributor Author

kodawah commented Oct 5, 2017

How do you determine if something has the right or wrong colors?

@sekrit-twc I tried to replicate conversion with different tools, ie vf_colorspace in ffmpeg or mpv, and visually check that the colors were the same.

@kodawah
Copy link
Contributor Author

kodawah commented Oct 5, 2017

btw any suggestions for this error?

src/zimg/colorspace/gamma.cpp: In function ‘float zimg::colorspace::log100_oetf(float)’:
src/zimg/colorspace/gamma.cpp:122:33: error: ‘log10f’ is not a member of ‘std’

it seems the solution is to include the <cmath> header, but it's already there...

@sekrit-twc
Copy link
Owner

But how do you know the conversion in ffmpeg and mpv are correct? Also, log10f is in the global namespace. In std:: there is only "log10", which is overloaded.

@kodawah
Copy link
Contributor Author

kodawah commented Oct 8, 2017

wrt mpv I trust the guy who wrote the rendering engine, wrt ffmpeg after I added the conversion parameters I stopped getting complaints from users about incorrect color support

@kodawah kodawah force-pushed the additional_color_transfers branch from ff8e35b to ef1f49f Compare October 9, 2017 15:32
@kodawah
Copy link
Contributor Author

kodawah commented Oct 9, 2017

build fixed

@kodawah kodawah force-pushed the additional_color_transfers branch from ef1f49f to bfad217 Compare October 16, 2017 18:35
@kodawah
Copy link
Contributor Author

kodawah commented Oct 16, 2017

@sekrit-twc hi, do you have any advice on how I can move this set of PRs forward? Thank you

@sekrit-twc
Copy link
Owner

@kodabb,

I need to find the time to review the transfer characteristics changes. I can merge the other PRs without further action.

PLEASE NOTE: You must agree to either assign copyright of code to the project owner or license contributions under WTFPL (see COPYING).

@kodawah
Copy link
Contributor Author

kodawah commented Oct 19, 2017

I agree to license my contributions under the WTFPL terms.

thanks for all the help and time spent on these PRs

@sekrit-twc sekrit-twc merged commit bfad217 into sekrit-twc:master Oct 20, 2017
sekrit-twc added a commit that referenced this pull request Oct 20, 2017
@kodawah
Copy link
Contributor Author

kodawah commented Oct 21, 2017

🎉

sekrit-twc pushed a commit that referenced this pull request May 9, 2021
sekrit-twc pushed a commit that referenced this pull request Jul 6, 2021
See #81

(cherry picked from commit a1a4fb5)
@sekrit-twc
Copy link
Owner

The commit was reversed following a customer escalation. I have created #155 for further discussion regarding this topic.

Ichunjo pushed a commit to Varde-s-Forks/zimg that referenced this pull request Jan 4, 2022
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