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

Timestamps should be specified in an interoperative way #62

Open
smfr opened this issue Mar 5, 2020 · 16 comments
Open

Timestamps should be specified in an interoperative way #62

smfr opened this issue Mar 5, 2020 · 16 comments
Assignees

Comments

@smfr
Copy link

smfr commented Mar 5, 2020

Instead of:

the timestamp should be the latest timestamp the user agent is able to note in this pipeline (best effort). Typically the time at which the frame is submitted to the OS for display is recommended for this API.

the timestamps should be the time at the end of the "update the rendering" steps, namely the same timestamp used for long task reporting, in section 12.1 of https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

Trying to guess when the OS is going to present the frame is fraught with peril and adds no value.

@npm1
Copy link
Contributor

npm1 commented Mar 5, 2020

Sounds good to me! Actually I think the order of paint timing call and update the rendering is incorrect. That is, the step "For each fully active Document in docs, update the rendering or user interface of that Document and its browsing context to reflect the current state." would need to go before the paint timing step. And then we can compute the timestamp right there, as there is now a step in between regarding requestIdleCallback which should not affect the computations. I'll need to update the order in HTML for this to work.

@rniwa
Copy link

rniwa commented Mar 5, 2020

Well, step 10.13 already involves mark painting time so we just need to get rid of this informal note.

I don't think we should use step 12.1 since that exposes the precise painting time, which is dangerous for timing attacks.

@npm1
Copy link
Contributor

npm1 commented Mar 5, 2020

You're saying that the timestamp should be the now that is passed on to the other callbacks? That one does not seem to include anything about the "update the rendering" step though.

@rniwa
Copy link

rniwa commented Mar 6, 2020

You're saying that the timestamp should be the now that is passed on to the other callbacks? That one does not seem to include anything about the "update the rendering" step though.

I'm not saying that. In step 10.13, updating the rendering invokes mark painting time. I'm saying that we should use the current time at that point.

Now, there are a few editorial bugs here.

First, mark painting time takes an input timestamp, that's not being passed to the algorithm in the event loop processing model. We need to either specify the timestamp in the event loop processing model, or use the current time in the mark painting time algorithm itself.

Second, the event loop processing model says that mark painting time is invoked on each document but the algorithm doesn't take a document as an argument. Once we've fixed it so that mark painting time takes a document as an argument, then we need to refine the definitions of the first paint and the first contentful paint so that those timings are well defined for a given document.

Finally, we need to make mark painting time pass this document to reporting paint timing, and the report paint timing algorithm needs to create PerformancePaintTiming object with that document.

@npm1
Copy link
Contributor

npm1 commented Mar 6, 2020

Thanks for pointing out the editorial issues, I also noticed them when going over the steps. Will fix soon.

npm1 added a commit that referenced this issue Mar 13, 2020
Provide the realm when constructing the PerformancePaintTiming object. This way, the queue algorithm will use the correct relevant global object. For #62
noamr pushed a commit that referenced this issue Mar 19, 2020
Provide the realm when constructing the PerformancePaintTiming object. This way, the queue algorithm will use the correct relevant global object. For #62
@yoavweiss
Copy link
Contributor

Was this fixed by #67 and/or #69?

@rniwa
Copy link

rniwa commented Mar 25, 2020

No, that text is still in the note:

NOTE: The rendering pipeline is very complex, and the timestamp should be the latest timestamp the user agent is able to note in this pipeline (best effort). Typically the time at which the frame is submitted to the OS for display is recommended for this API.

This note should be removed now that mark the paint timing is now called at the very precise timing in update the reddening step. There is no leeway left for UA to report a best-effort time, and that's a good thing both for security & interoperability.

@noamr
Copy link
Contributor

noamr commented Apr 19, 2024

Coming back to this issue from 4 years ago, I think I have a proposal that would work for both tihs, #104, and w3c/largest-contentful-paint#104.

The idea is that the paint timestamp would be the rAF timestamp of the next frame. So if the rendering opportunity that produces the relevant paint creates some sort of presentation delay, it would be reflected in paint-timing/LCP in an way that's already speified and observable. This would also mitigate anything that has to do with exposing render time of cross-origin images as it's an already exposed timestamp.

@smfr @sefeng211 @mmocny @yoavweiss WDYT?

@mmocny
Copy link

mmocny commented Apr 19, 2024

I like it.


Marking timestamps aside, there is also the decision of which rAF to mark. I don't know if we need to clarify further, or if its already specified sufficiently in paint timing, but it should be the rAF frame time of the animation frame that follows after the content was presented.

It might only be best-effort or somewhat UA specific, but there are situations (i.e. image decode) where I think a first paint merely requests decode to start, and then there is a future paint (perhaps many rAF's afterwards), that actually paints the decoded image.

@anniesullie
Copy link

anniesullie commented Apr 19, 2024

@bdekoz too (see above comment)

@smfr
Copy link
Author

smfr commented Apr 19, 2024

How do you know what the rAF timestamp for the next frame will be? That frame might be delayed.

@noamr
Copy link
Contributor

noamr commented Apr 19, 2024

How do you know what the rAF timestamp for the next frame will be? That frame might be delayed.

We wait to queue the entry and set its timing until that happens, or something like hiding the document happens.

@mstange
Copy link

mstange commented May 17, 2024

So if nothing triggers a paint for a while, this would mean that the timestamp can be much delayed, no?

I don't think this proposal adds a lot of value over using what Simon proposed, i.e. getting the timestamp at the end of the "update the rendering" steps. In Firefox, waiting for the next frame would add an arbitrary delay to the reported timestamp. The delay would be unrelated to the time when the compositor thread is done processing the current frame: If compositing is slow, then we do the "update the rendering" steps for the next frame before compositing has finished (on a different thread) for the current frame, because we allow compositing to fall behind by up to two frames. And if compositing is fast, but the page isn't triggering any more paints, then the next frame would happen long after compositing finishes for the current frame.

I would prefer missing the compositing part entirely over adding an arbitrary delay that is unrelated to the time that compositing takes.

@noamr
Copy link
Contributor

noamr commented Jul 19, 2024

Summary from WebPerfWG call: the general direction is to align Chromium FCP time with the spec. Chromium still intends to report the current number as presentationTime.
Before doing so we will produce some data as to the difference between these numbers in the wild.

@mmocny
Copy link

mmocny commented Jul 22, 2024

I like the general idea of exposing both paint timings for easier comparison and better interop. However:

  • The specced paint time is currently only exposed via startTime (or as renderTime or duration in other "paint timings")
  • Changing this value in Chromium would have a significant effect on developers, even if the current value is still exposed via a new presentationTime

In the past, we discussed exposing both both timings explicitly as new properties:

  • paintTime and
  • presentationTime (optional)

Then, defining startTime (and/or duration where applicable) as based on max(paintTime, presentationTime).

This type of approach already happens for LCP via startTime = max(element.loadTime, element.renderTime, fcp.startTime) or something like that.

This would have no breaking change for Chromium, would improve interop, and enable more direct comparison between implementations (where desired).

(There was a thread about the above in the past, I guess somewhere else...)

@noamr
Copy link
Contributor

noamr commented Jul 22, 2024

I like the general idea of exposing both paint timings for easier comparison and better interop. However:

  • The specced paint time is currently only exposed via startTime (or as renderTime or duration in other "paint timings")
  • Changing this value in Chromium would have a significant effect on developers, even if the current value is still exposed via a new presentationTime

In the past, we discussed exposing both both timings explicitly as new properties:

  • paintTime and
  • presentationTime (optional)

Then, defining startTime (and/or duration where applicable) as based on max(paintTime, presentationTime).

This type of approach already happens for LCP via startTime = max(element.loadTime, element.renderTime, fcp.startTime) or something like that.

This would have no breaking change for Chromium, would improve interop, and enable more direct comparison between implementations (where desired).

(There was a thread about the above in the past, I guess somewhere else...)

This is fine with me

@smfr how do you feel about this? It basically means that WebKit exposes paintTime which is an alias for startTime, and the spec says that there's an optional presentationTime and that startTime maps to it if it is provided.

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 a pull request may close this issue.

8 participants