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 average pass duration algorithm #328

Closed

Conversation

beaufortfrancois
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Nov 21, 2023

As discovered in https://bugs.chromium.org/p/chromium/issues/detail?id=1504013#c5, the average pass duration algorithm was misbehaving because the t variable was not captured properly. This PR fixes this issue.

This diff will hopefully help reviewers:
image

@kainino0x Please review.

@toji
Copy link
Contributor

toji commented Nov 21, 2023

Ah, sorry I didn't see this before implementing my own fix: #329

My code includes rejection for negative durations, which was causing some of the timestamp instability. It also coincidentally sidesteps this capture problem by no longer using t to decide when to update the UI.

@beaufortfrancois
Copy link
Collaborator Author

I wasn't able to get negative durations on my Pixel 7 device. Is there a specific config that is needed to reproduce?

@toji
Copy link
Contributor

toji commented Nov 21, 2023

I haven't tested with the Pixel 7 but on a Pixel 4, 6, and 8 I've seen them come through occasionally, as well as devices from other manufacturers. Also worth noting that timestamps on Android are a bit bugged right now, which I'm in the process of fixing.

@kainino0x
Copy link
Collaborator

Sorry, I only saw this after Brandon sent me his change. Nice catch about the broken t variable! Fortunately, Brandon's change also fixes this capture issue by using a different variable that only changes inside the promise callback.

Though this does have me wondering if we need to strengthen the ordering guarantees in the spec - ring of map buffers is going to be a common pattern and if they resolve out-of-order then I could imagine strange things happening in an application.

This diff will hopefully help reviewers:

FYI GitHub can also do this:
image

@kainino0x kainino0x closed this Nov 21, 2023
@kainino0x
Copy link
Collaborator

kainino0x commented Nov 21, 2023

BTW when I originally prototyped the averaging code I did not see this bug on Mac, but then yesterday when I loaded it up again I was noticing it there too. (M1 Pro)

@kainino0x
Copy link
Collaborator

Wait, no, that was probably just this bug, not the negative timestamp one.

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.

3 participants