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

Unify color space settings as 709 #3415

Merged
merged 3 commits into from Sep 7, 2020

Conversation

jpark37
Copy link
Collaborator

@jpark37 jpark37 commented Sep 5, 2020

Description

Change the default color setting in the UI from sRGB to 709, and VIDEO_CS_DEFAULT from 601 to 709.

Submodule PR: obsproject/obs-amd-encoder#405

Motivation and Context

It seems like YouTube applies nonlinear-to-linear sRGB, and
linear-to nonlinear-709 transformations to uploaded videos now. This
makes sRGB too dark on their platform for video players that alias 709
as sRGB, which is almost everyone. Make 709 the default to keep peace.

Update VIDEO_CS_DEFAULT to 709 for consistency.

How Has This Been Tested?

Debugger inspection of all changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (a change to documentation pages)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@koitsu
Copy link

koitsu commented Sep 5, 2020

Is this safe to do? Wasn't there a guy in Discord who proposed doing this 6-8 months ago and was told no, and that sRGB should be the default? What's changed between then vs. now? Edit: it was a couple years ago, actually. @jp9000 stated that there were other problems caused by switching to 709 (affecting Twitch).

@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 5, 2020

We've done a fair amount of testing since, and Twitch seems to be decently fine with 709 on various devices.

@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 6, 2020

Revert back to 601 for now. I'd like to update the meaning of VIDEO_CS_DEFAULT, and also add plumbing for VIDEOINFOHEADER2, but these changes are going to take some time.

@jpark37 jpark37 changed the title UI: Switch sRGB to 709 as default color space UI: Switch sRGB back to 601 as default color space Sep 6, 2020
@koitsu
Copy link

koitsu commented Sep 6, 2020

We've done a fair amount of testing since, and Twitch seems to be decently fine with 709 on various devices.

I'll take this to mean "yes, the person who told us to do this 2 years ago was right". Thanks.

@WizardCM
Copy link
Member

WizardCM commented Sep 6, 2020

Does that mean this issue has been fixed on both our side and Twitch app?

@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 6, 2020

The Android issue was device dependent. One device could have good 601 and bad 709, and another bad 601 and good 709: obsproject/rfcs#7 (comment)

We did help the situation somewhat by submitting a fix, so that both the video stream and container are tagged: obsproject/rfcs#7 (comment)

It seems like YouTube applies nonlinear-to-linear sRGB, and
linear-to nonlinear-709 transformations to uploaded videos now. This
makes sRGB too dark on their platform for video players that alias 709
as sRGB, which is almost everyone. Make 709 the default to keep peace.
@jpark37 jpark37 marked this pull request as draft September 6, 2020 19:42
@jpark37 jpark37 changed the title UI: Switch sRGB back to 601 as default color space Unify color space settings as 709 Sep 6, 2020
@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 6, 2020

People don't want to go backwards to 601, so I've instead modified VIDEO_CS_DEFAULT to mean 709. Plumbing VIDEOINFOHEADER2 is a bit of a headache, but that can come later (if ever).

Still needs more testing, which I can look at doing later today or tomorrow, so back to draft status.

@jp9000
Copy link
Member

jp9000 commented Sep 7, 2020

Fine with me as long as it's working on the majority of the expected outputs okay

@jp9000
Copy link
Member

jp9000 commented Sep 7, 2020

I should clarify: outputs and inputs. I would check to see where VIDEO_CS_DEFAULT is used and make sure any inputs that use it behave as expected.

@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 7, 2020

All changes have been tested. Do i need to do something extra for the documentation change?

I'll mark this ready for review after the AMD encoder submodule PR is merged.

@jpark37 jpark37 force-pushed the default-color-space branch 2 times, most recently from bc28649 to 0d94f2b Compare September 7, 2020 03:29
Consistent with modified default UI setting.
@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 7, 2020

Went back through, and tied up a few loose ends. I think I'm done.

@jpark37
Copy link
Collaborator Author

jpark37 commented Sep 7, 2020

Documentation verified. Just waiting for submodule PR merge.

@jpark37 jpark37 marked this pull request as ready for review September 7, 2020 15:03
@jp9000 jp9000 merged commit 86147d9 into obsproject:master Sep 7, 2020
@jpark37 jpark37 deleted the default-color-space branch September 7, 2020 20:36
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.

None yet

4 participants