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

Fix setting max fps and fps count for renderer #1477

Merged
merged 10 commits into from
Aug 10, 2022

Conversation

kiryldz
Copy link
Contributor

@kiryldz kiryldz commented Jul 5, 2022

Summary of changes

This PR fixes 2 problems that are pretty close to each other.

  • when adding FPS listener via MapView.setOnFpsChangedListener previously callback was triggered immediately after render call was executed. That was actually not correct as docs stated that callback should return average amount of rendered frames during last second. Now this is fixed and FPS values are much more readable and reliable. Assuming we use rendering on demand (and not continuous) corner situations are also covered:
    • when not interacting with the map for some time and starting to interact after some delay - we will not report huge amount of missed VSYNC events, instead we will report last FPS update after 50 ms update of map being IDLE and reset counters.
    • when going to background we will also report last FPS update and reset counters so that when resuming the map number of VSYNC missed events will not be huge
  • when setting maximum custom FPS via MapView.setMaximumFps correct FPS value will be now applied. Previously without frame pacing algorithm when setting let's say 59 FPS on 60 Hz screen actual FPS was 30 (closest divider). Now we do have basic frame pacing algorithm in place that spreads moments of time when we actually wish to render the frame evenly across one second timeframe.

User impact (optional)

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
  • Add example if relevant.
  • Document any changes to public APIs.
  • Run make update-api to update generated api files, if there's public API changes, otherwise the verify-api-* CI steps might fail.
  • Update CHANGELOG.md or use the label 'skip changelog', otherwise check changelog CI step will fail.
  • If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

@kiryldz kiryldz added the bug 🪲 Something isn't working label Jul 5, 2022
@kiryldz kiryldz self-assigned this Jul 5, 2022
@kiryldz kiryldz removed the bug 🪲 Something isn't working label Jul 5, 2022
@@ -49,6 +50,7 @@ class MapSurface @JvmOverloads constructor(
*/
fun surfaceCreated() {
renderer.surfaceCreated()
mapController.setScreenRefreshRate((context.getSystemService(Context.WINDOW_SERVICE) as WindowManager).defaultDisplay.refreshRate)
Copy link
Member

Choose a reason for hiding this comment

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

How about multi-display use cases?

Copy link
Member

Choose a reason for hiding this comment

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

In a typical Android Auto use case, the MapSurface is not the default display, and does not necessarily have the same refresh rate as the primary display.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def worth checking, any ideas how to check this real use-case? we could simulate secondary display (we even have example activity for that) but to properly check it we need diff screen refresh rate on another screen...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pengdev I did not manage to actually simulate primary display with one refresh rate and secondary with another but I did make sure that applying different user refresh rates (via setMaximumFps) to map on primary and secondary displays works fine. Guess that's all we could do now, if some extraordinary case will pop-up (although it's indeed very specific use-case but it should be handled).

Also I did check via our SurfaceActivity that methods mapSurface.setMaximumFps and mapSurface.setOnFpsChangedListener do work as expected.

@kiryldz kiryldz force-pushed the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch from 8451450 to 56d90b9 Compare July 20, 2022 11:51
@kiryldz kiryldz force-pushed the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch from c10ad7c to a7fea31 Compare July 21, 2022 13:15
@kiryldz kiryldz added the bug 🪲 Something isn't working label Jul 21, 2022
@kiryldz kiryldz marked this pull request as ready for review July 21, 2022 14:56
@kiryldz kiryldz requested a review from a team as a code owner July 21, 2022 14:56
@kiryldz kiryldz requested review from yunikkk and pengdev August 9, 2022 15:42
@kiryldz kiryldz force-pushed the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch from 2f3d60a to 18e81e7 Compare August 9, 2022 15:52
@kiryldz kiryldz requested a review from yunikkk August 9, 2022 15:53
@kiryldz kiryldz force-pushed the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch 2 times, most recently from ad51a8e to 6cfd58a Compare August 10, 2022 09:10
@kiryldz kiryldz force-pushed the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch from 6cfd58a to 7f97d14 Compare August 10, 2022 09:12
@kiryldz kiryldz merged commit 13c3e0c into main Aug 10, 2022
@kiryldz kiryldz deleted the MAPSAND-187-fix-setting-max-fps-for-map-rendering branch August 10, 2022 09:40
kiryldz pushed a commit that referenced this pull request Aug 2, 2023
…D-765] [MAPSAND-810][MAPSAND-525]. (#1477)

* Return cancelable from *FeatureState APIs.

* Add resetFeatureState.

* Differentiate qRF and qSF return and callback types.

* Return cancelable from async get methods.

* Cleanup.

* Remove render cache test, update metalava.

* Update test.

* Update changelog.

* Fix tests.

* Add UI test for FeatureState APIs.

* Ktlint

* Fix unit test.

* Fix benchmark app build.

* Fix changelog.

---------

Co-authored-by: ank27 <ankur.khandelwal@mapbox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants