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

Resolution: Allow 3:2 pulldown and next integer and doubled upper integer #14077

Closed
wants to merge 1 commit into from

Conversation

Memphiz
Copy link
Member

@Memphiz Memphiz commented Jun 18, 2018

The new whitelisted refresh rate approach changed behaviour in a way that results in a regression for macOS users. macOS has no fractional rates. So those users depend on automated switching to 24hz for 23.xx FPS content and tend to use sync to display in addition.

This change restores that refreshrate matching behaviour.

23.xx matches to 24hz
29.xx matches to 30hz or 60hz

Motivation and Context

Regression reported in https://forum.kodi.tv/showthread.php?tid=332653

How Has This Been Tested?

User gb160 confirmed the fix

Thx @fritsch for the code.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@Memphiz Memphiz requested a review from lrusak June 18, 2018 18:05
@Memphiz Memphiz added Type: Fix non-breaking change which fixes an issue v18 Leia labels Jun 18, 2018
@lrusak
Copy link
Contributor

lrusak commented Jun 18, 2018

Seems fine. My only suggestion is to add logging in between the selections like there is above. This makes it easier to know what's going on and what's being tried/matched

@@ -168,6 +168,71 @@ void CResolutionUtils::FindResolutionFromWhitelist(float fps, int width, bool is
}
}

// Allow 3:2 pull down

This comment was marked as spam.

}
}

// Allow next upper integer refreshrate - some computers, like the MAC don't have fractional refreshrates - those refreshrate

This comment was marked as spam.

}
}

// And finally to help people to get 29.97i to 60 hz

This comment was marked as spam.

@FernetMenta
Copy link
Contributor

see my comments. exceptions must be limited to known cases. we don't want any undesired magic elsewhere.

const RESOLUTION_INFO desktop_info = CServiceBroker::GetWinSystem()->GetGfxContext().GetResInfo(CDisplaySettings::GetInstance().GetCurrentResolution());

if (info.iScreenWidth == desktop_info.iWidth &&
info.iScreen == desktop_info.iScreen &&

This comment was marked as spam.

@MartijnKaijser
Copy link
Member

@Memphiz if you have some time to finish this :)

@Memphiz Memphiz closed this Aug 9, 2018
@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2018

Done ;) - this is not an accepted solution. Bringing the passthrough dependency into the equation is neither.

@Rechi Rechi removed the v18 Leia label Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants