w3c / largest-contentful-paint

Specification for the LargestContentfulPaint API
https://w3c.github.io/largest-contentful-paint/
Other
88 stars 17 forks source link

requestAnimationFrame callback and LCP renderTime should be the same #104

Open sefeng211 opened 2 years ago

sefeng211 commented 2 years ago

So the spec states that we store the now time as the render time for LCP entries when doing the update the rendering step

Since rAF also uses same now time as the argument for its callback, they should be equal, however it doesn't.

Test case https://mozilla.seanfeng.dev/files/images/lcp.html, there is also a discrepancy between what LCP and rAF report.

yoavweiss commented 2 years ago

/cc @mmocny @philipwalton

sefeng211 commented 2 years ago

I guess this could be a chromium bug however this could also be a potential spec issue.

If we agree on that they should be equal, then what timestamp should be used? Update the rendering steps don't state which timestamp this is, however, it seems that implementations use vsync timestamp. If vsync timestamp is being used, then this leads to a LCP issue because the load time of the entry could potentially be larger/later than the render time.

It's going to be something like vsync -> image completely available -> update the rendering (uses vsync timestamp, this is timestamp in the past

mmocny commented 2 years ago

Thanks Sean for poking at this. My understanding is that there are three distinct timings:

  1. The start of a new frame (typically vsync), which is used as the time value for requestAnimationFrame callbacks.
  2. Mark Paint Timing algorithm, which marks when the main thread Paint stage is completed.
  3. element-timing-processing algorithm, which marks when contentful elements are finished presenting, i.e. the vsync of the next frame.

(Indeed it seems that the term now is used a few times in different contexts but represents different time points...)

(1) Is I think Step 13 of Update the rendering and now refers to a value set in Step 5.

(2) Is I think Step 15 of Update the rendering, and clarified in mark paint timing to use paintTimestamp at that point.

(3) Is I think Step 17 of Update the rendering, which is added via element timing. (See: "In the update the rendering step of the event loop processing model, add the following substep at the end").

Critically, Step 15 and 17 just compute 2 different time points, and are separated by Step 16 which states: "update the rendering or user interface of that Document and its browsing context to reflect the current state." While element timing spec does use now, it should be from the vsync time of the frame after the content is presented. (It's possible the wording needs changing since it does seem a bit ambiguous if now is from the frame before).

Not related to LCP but still worth mentioning, we do have https://github.com/w3c/paint-timing/issues/62 as an open issue in paint timing. Chromium does expose the Element Timing interpretation of renderTime (3) rather than (2), which Safari exposes. AFAIK Chromium does not expose (2) at all.


Using your scenario of an image, I think we could have many timings:

The paint/decode/presentation timestamps are inconsistent across implementers.

I think we would serve developers better to expose more of those time points more consistently (I think exposing both Paint end + vsync after Presentation, would be very worthwhile).

sefeng211 commented 2 years ago

Thanks @mmocny, the clarification indeed clears things out.

So I am not sure if we want (2) and (3) to be different timestamps because that would end up having different timestamps for FCP and LCP when there's only one contentful element. This test will no longer holds, right?

In any case, I agree the usage of now in element-timing needs changing because it currently refers to the same now from Step 5 of Update the rendering

mmocny commented 2 years ago

Correct! We wouldn't want FCP and LCP to report different timings. Chromium reports (3) for both, but Safari reports (2) and does not report LCP or element timing, AFAIK. Hence the issue to clean up the paint timing spec to resolve the interop mismatch. Good catch on the element timing misuse of now (I think, I'll poke some folks who have more experience here).


My thinking on how we may want to move forward was expressed in longer form here, but since that issue mixes a few problems, let me try to summarize:

Given that already established pattern, I think that means we could start by defining both versions of timing in (2) and (3) with specific names (Strawman: paintTime and presentationTime).

Then refine startTime and duration to be also in terms of those new time values.

If cross platform interoperable timings are desired, you could now directly compare those specifically named timings. (Though, rendering pipelines may still be different enough that even under these conditions its not directly comparable.)


As an aside, the requestAnimationFrame time value is also worth pushing on separately I think... While the vsync time of the frame is probably the most useful time point for rAF based animations... The current spec-defined time value seems also very useful to me: it represents the rendering task startTime (strawman: renderingStart). A significant number of performance issues are due specifically to a large amount of time between renderingStart and paintTime and it can be hard to measure that today.


I'm glad you are poking this, since I'm really keen to get more input here. I'm hoping we can push on these questions prior to TPAC and then have a working session there!

noamr commented 2 years ago

Great summary, @mmocny, thanks! A few thought about the above, I hope I come across clearly.

The "presentation time" as mentioned here is somewhat equivalent to the next rAF time, or now when there is a render opportunity? Perhaps that's an interoperable way to define the "real" presentation time.

The issue though is that the actual rendering is done in parallel. This is not really defined in the aforementioned step 17 attached from element-timing and makes it kind of handwavy. In the case of long tasks for example, the actual vsync time might be quite a bit before rAF time and quite a bit after "Mark the paint timing" or any synchronous steps you put in the main thread.

In other words, step 17 is concurrent to the first steps in update the rendering, especially 2.5 (run oldestTask's steps), so specifying it needs to address that as it probably needs to mark the time in that parallel thread and then dispatch a new task back to the main thread that reports it.

The problem with that in-parallel actual "pixels on screen" time, is that it's an implementation-defined time. As such, it's somewhat pointless to specify it - though of course we can all implement it as a useful implementation-defined metric for web developers. I wonder though if this metric is a bit timing-attack prone as it exposes timings of internal common resources such as GPU.

mmocny commented 2 years ago

The "presentation time" as mentioned here is somewhat equivalent to the next rAF time

Yes, except that it all comes down to the question: next rAF time of which frame.

On Chromium the presentation time of a frame is not necessarily the next frame after Paint. That is because Paint is only the last rendering stage on main thread, and remaining rendering work off thread could take more than a single frame. And this is increasingly true as more rendering work is moved off main.

My understanding from studying Paint in Safari while there is also work that is off-main thread, Safari will tend to return to main thread and wait for another Paint after it is done (sorta like an first paint, or pre-paint, and final paint -- I only tested for Image Decode), but perhaps it is also not guaranteed that all work be completed by next frame even on Safari?

The issue though is that the actual rendering is done in parallel.

Exactly. There is of course a rendering stage after which the next rAF time is effectively "presentation time", but it's not really part of the synchronous "Update the rendering".

In other words, step 17 is concurrent to the first steps

Agreed. Though, my interpretation is that step 16 is also already (implicitly) concurrent, which is why I interpreted that the added Step 17 was also (agree that's under-specified). It seems to me at least that Step 16 likely is already concurrent across most implementations?

As discussed with you offline, perhaps Step 16 + 17 should be defined in a concurrent task (and potentially we may need more steps afterwards to report the time values measured back to main?)

The problem with that in-parallel actual "pixels on screen" time, is that it's an implementation-defined time

Yes, the process of getting pixels on screen is certainly implementation-defined, but as you said: perhaps just "the next rAF time after all rendering work is complete" is sufficient? What matters is that the implementation considers the frame submitted to display and no further rendering work will follow -- could that be interoperable enough?

sefeng211 commented 2 years ago

I am trying to summarize the action items based on the current discussion.

We want to have these 3 distinct timings defined.

1.The start of a new frame (typically vsync), which is used as the time value for requestAnimationFrame callbacks.

2.Mark Paint Timing algorithm, which marks when the main thread Paint stage is completed.

element-timing-processing](https://wicg.github.io/element-timing/#element-timing-processing) algorithm, which marks when contentful elements are finished presenting, i.e. the vsync of the next frame.

Once we have those timings defined, what we'll do are

So the end results are FCP becomes cross platform interoperable (though as Michal has said, the render pipeline may still be different). LCP's also has a cross platform interoperable timing (paintTime).

Miscellaneous

Does this sound correct?

mmocny commented 2 years ago

Thanks Sean! FYI: I'm also trying to make an interop 2023 proposal based on this thread and TPAC discussion, I would love your feedback on that (will post and link shortly).


1.The start of a new frame (typically vsync), which is used as the time value for requestAnimationFrame callbacks.

This is equivalent to the now time defined in the spec, so no action item is needed.

I am not sure, but I think implementers do not actually expose the now time as specced exactly for rAF callbacks. I agree that there is spec language that says they should, but I'm not sure that's exactly how it works in practice. For Chromium for example, the time passed to rAF callbacks is a vsync aligned time, even if the rAF callback is invoked not rAF aligned. I tested across browsers and get different values for the following snippet:

function raf() {
  return new Promise(resolve => requestAnimationFrame(resolve));
}

async function measureGaps() {
  const start = performance.now();
  const raf1 = await raf();
  const mid1 = performance.now();
  const raf2 = await raf();
  const mid2 = performance.now();
  const raf3 = await raf();
  const mid3 = performance.now();

  const delta1 = raf1-start;
  const delta2 = raf2-raf1;
  const delta3 = raf3-raf2;

  const gap1 = mid1-raf1;
  const gap2 = mid2-raf2;
  const gap3 = mid3-raf3;

  console.table({
    //start, raf1, raf2, raf3,
    delta1, delta2, delta3,
    gap1, gap2, gap3,
  });
}

measureGaps();

(No doubt some of those are just specifics to rendering pipeline differences, but it looks to me like what the time value represents is a bit unique.)

However, perhaps more to your point -- I do think for paint timings, the most important value is indeed the actual now time as specced, rather than the time passed to rAF (which perhaps is only interesting for running animations smoothly).

As far as I know, there is no way to measure this time without actually calling requestAnimationFrame today, which seems like an antipattern for measuring paint timings. So I think we could consider also attaching that timestamp to paint timing entries.

Strawman name: renderingStart.


Spec PR to make the change (assume this is named as paintTime) Spec PR to make the change (assume this is named as presentationTime)

Both Sgtm.


Once we have those timings defined, what we'll do are Ensure FCP uses paintTime Update LCP to make it expose both paintTime and presentationTime (optionally)

So, I would say that FCP/LCP (and element timing and event timings) are the concepts that help pick/represent a specific animation frame for which to expose paint timing information. The actual paint timings should probably be similar: all should expose renderingStart, paintTime, presentationTime etc... (and perhaps more: loadTime for images, processingStart/End for events, firstAnimatedFrame for animated images..., something for progressive images).

Net/net, I think FCP should also expose presentationTime (optionally).

While not all timings are necessarily exposed always or on all platforms-- it will always be the largest available timing that is used as the overall end point (i.e. startTime + duration).


Miscellaneous

You are correct that picking the right animation frame during Image Decode is one issue. There may be others:

Those seem less directly related than the main points above.

noamr commented 7 months ago

See this gist for how I think this should be spec'ed roughly.