viz: pass swap_trace_id to trace a frame submission [chromium/src : main]
Maksim Sisov (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
Maksim Sisov would like Vasiliy Telezhnikov, Chromium IPC Reviews, Peter McNeeley and Nico Weber to review this change.
viz: pass swap_trace_id to trace a frame submission
Pass swap_trace_id from viz thread to gpu and back to
track the flow of the frame submission.
It was chosen to use SkiaOutputSurfaceImplOnGpu::PostSubmit
as it calls to different gpu output device implementations.
That specific point helps to avoid adding this
trace event to different implementations. Moreover, that's
the point where calls to different backends to swap buffers
is called. Happens on the gpu thread.
SkiaOutputDevice::FinishSwapBuffers was also chosen as it's
called by all the gpu implementations after swap is finished.
Happens on the gpu thread.
In case of the sw path, SoftwareOutputSurface was chosen as
the common point.
PS all the backends are free to use FrameData::swap_trace_id
to extend the flow of the trace. My next CL will contain such
a change for Ozone/Wayland.
Bug: 1432641
Change-Id: Iaa8b560778a5a70ecd8dc7ad814381c5228b95d6
---
M components/viz/service/display/direct_renderer.h
M components/viz/service/display/display.cc
M components/viz/service/display/skia_renderer.cc
M components/viz/service/display/software_renderer.cc
M components/viz/service/display_embedder/skia_output_device.cc
M components/viz/service/display_embedder/skia_output_device.h
M components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc
M components/viz/service/display_embedder/software_output_surface.cc
M components/viz/service/display_embedder/software_output_surface.h
M gpu/command_buffer/common/swap_buffers_complete_params.h
M ui/gfx/frame_data.h
M ui/gfx/mojom/frame_data.mojom
M ui/gfx/mojom/frame_data_mojom_traits.h
13 files changed, 70 insertions(+), 6 deletions(-)
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
1 comment:
Patchset:
Hi. This is a CL that adds additional trace flow to track swap. It's needed for the LaCros project's swap latency task. I was suggested to upstream this by Peter so that other developers can work on a similar task using the same trace flow.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Kinuko Yasuda, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
gwsq would like Kinuko Yasuda to review this change authored by Maksim Sisov.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
Maksim Sisov has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: kin...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Stephen Nusko (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
1 comment:
Patchset:
Couple generic questions
1) Why call this Graphics.Pipeline (we already have events called Graphics.Pipeline which tracks the pipeline). Is this part of the same flow (just tracking to the end?) or completely separate?
2) (if it remains Graphics.Pipeline) You should update the ChromeGraphicsPipeline proto if you intend to use this: https://source.chromium.org/chromium/chromium/src/+/main:base/tracing/protos/chrome_track_event.proto;l=1367;drc=9abd133de3ea3325766a01b6576d3d53f250ee64
3) (if it remains Graphics.Pipeline) Should we add a value to the STEP enum to denote these parts?
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
Thanks for your comments.
1) Why call this Graphics.Pipeline (we already have events called Graphics.Pipeline which tracks the pipeline). Is this part of the same flow (just tracking to the end?) or completely separate?
I called the same name as I'm not sure it deserves a different one. Though, it doesn't continue the flow that one gets from the begin frame source to frame sinks and back to viz, which use the begin frame trace id. Logically, it's the continuation of the same flow. This is how it looks like - https://drive.google.com/file/d/1X1cPJ6qRjxWwyaDdvpeU9hU2nuoniauV/view?usp=sharing
2) (if it remains Graphics.Pipeline) You should update the ChromeGraphicsPipeline proto if you intend to use this: https://source.chromium.org/chromium/chromium/src/+/main:base/tracing/protos/chrome_track_event.proto;l=1367;drc=9abd133de3ea3325766a01b6576d3d53f250ee64
Ok, if you agree with the Graphics.Pipeline naming
3) (if it remains Graphics.Pipeline) Should we add a value to the STEP enum to denote these parts?
As I said, logically it's the same flow. I'll give other reviewers time to review this and then decide.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Nusko (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
1 comment:
Patchset:
Thanks for your comments. […]
Can we not connect them them?
1) Could we use the same flow id?
2) if not could we access the same flow id on the first one after SurfaceAggregation here: https://drive.google.com/file/d/1X1cPJ6qRjxWwyaDdvpeU9hU2nuoniauV/view?
If so we could put both flow IDs onto this event then even so they use different IDs they would be connected.
I.E.
TRACE_EVENT("Graphics.Pipeline", "...,graphics.pipeline", perfetto::TerminatingFlow::Global(<begin_frame_arg_trace_id>), perfetto::Flow::Global(viz_swap_id))
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
1) Could we use the same flow id?
Not really. Those STEP_SURFACE_AGGREGATION may use different begin_frame_ack.trace_ids[1][2]. That's because how they are generated for each frame sink [3]. Moreover, if I connect this flow to the root surface's frame's trace_id, it may stuck at one value as it doesn't produce any frames, but is still part of an AggregatedFrame (for example, the browser is running in a fullscreen mode) [4]
[1] https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/surface_aggregator.cc;l=2200?q=components%2Fviz%2Fservice%2Fdisplay%2Fsurface_aggregator.cc&ss=chromium
[2] https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/surface_aggregator.cc;l=883?q=components%2Fviz%2Fservice%2Fdisplay%2Fsurface_aggregator.cc&ss=chromium
[3] https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/frame_sinks/compositor_frame_sink_support.cc;l=984?q=STEP_ISSUE_BEGIN_FRAME&ss=chromium%2Fchromium%2Fsrc:components%2Fviz%2Fservice%2Fframe_sinks%2F[4]
[183441:183546:0907/124347.533830:ERROR:surface_aggregator.cc(2200)]
root_surface_frame.metadata.begin_frame_ack.trace_id 4294967586
[183441:183546:0907/124347.534248:ERROR:surface_aggregator.cc(881)]
frame.metadata.begin_frame_ack.trace_id 1970333426909556
[183441:183546:0907/124347.550494:ERROR:surface_aggregator.cc(2200)]
root_surface_frame.metadata.begin_frame_ack.trace_id 4294967586
[183441:183546:0907/124347.550918:ERROR:surface_aggregator.cc(881)]
frame.metadata.begin_frame_ack.trace_id 1970333426909557
[183441:183546:0907/124347.567124:ERROR:surface_aggregator.cc(2200)]
root_surface_frame.metadata.begin_frame_ack.trace_id 4294967586
[183441:183546:0907/124347.567534:ERROR:surface_aggregator.cc(881)]
frame.metadata.begin_frame_ack.trace_id 1970333426909558
If so we could put both flow IDs onto this event then even so they use different IDs they would be connected.
That's a good idea. I've implemented that, but there are some trade offs...
Given swap_id is connected with |begin_frame_ack.trace_id|, we are loosing tracing for frames that are reused if perfetto::TerminatingFlow is used. That is, if a frame sink doesn't do updates, the same frame is reused over and over again. In that case, the trace looks like this [1]. This first Graphics.Pipeline just under tha Display::DrawAndSwap is the aggregation step for the root surface. Given it didn't produce any frames, the same frame was reused (and it has the same begin_frame_ack.trace_id as the previous frames). perfetto::TerminatingFlow results in loosing trace, which actually resulted in submission of this frame from a frame sink.
If perfetto::Flow is used, then trace looks like [2] when I click on one of the SWAP_ACK steps (that's a terminating flow for viz swap id). You can see here that there were 3 frame updates from the root frame sink and just one update (in the very beginning) from the renderer frame sink. Given it was united with swap_id, it gets further united with all the other frames from the root frame sink.
Is there a way to avoid implicitly connecting begin frame trace ids of frame sinks through the swap id?
[1] https://drive.google.com/file/d/109hkOYOi8J_QjE9dKranr3V2moV45aUK/view?usp=drive_link
[2] https://drive.google.com/file/d/1qu5ejhvoBf5ZMv_X7CcS-CtrOAAD3Gar/view?usp=sharing
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Nusko (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
1 comment:
Patchset:
> 1) Could we use the same flow id? […]
Hmm can you explain a bit more (perhaps we should GVC to quickly jam board this). What is wrong with [1]? It looks nice and clear?
Is the concern that the SurfaceAggregation doesn't carry onwards to the next frame it gets reused in? I personally dislike that feature it creates a lot of visual noise without adding a lot of information. Or does it break tracing more then that?
Do you have example traces for [1] & [2] I could play around with?
Flow Ids are global and if you don't have TerminatingFlowId they will always just connect through by default. the swap ID only connects backwards though to the begin_arg so it will basically just be a forked event in [2] (where there are two flow ids coming out. This seems preferable to me then these events not being connected.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
Hmm can you explain a bit more (perhaps we should GVC to quickly jam board this). What is wrong with [1]? It looks nice and clear?
Sure we can if there are more questions 😊
Is the concern that the SurfaceAggregation doesn't carry onwards to the next frame it gets reused in? I personally dislike that feature it creates a lot of visual noise without adding a lot of information. Or does it break tracing more then that?
Yes, my concern is that it changes the previous behaviour.
Do you have example traces for [1] & [2] I could play around with?
Yes, here it is: [1] (with terminating flow) - https://drive.google.com/file/d/1vYXrhYNK3kckD4cNXemcupM4wx2cixjF/view?usp=drive_link and [2] (with non terminating flow) - https://drive.google.com/file/d/1jBPQjEvAGTJZnP1Dv7U7feV1vkZVqlTN/view?usp=drive_link
For the [1], check Graphics.Pipeline at 00:00:05.335620159. For this aggregation, renderer's frame was reused, while the frame from UI was a new one.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Paul Irish (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
File ui/gfx/mojom/frame_data.mojom:
Patch Set #8, Line 14:
int64 seq;Just curious.. What is the relationship between the `swap_trace_id` and the `sequence_number`?
I'm building improved frame tracking for DevTools and am relying on sequence_number to piece together the story. I'm wondering if I'll need to also correlate events using this new id.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Stephen Nusko (Gerrit)
Attention is currently required from: Jonathan Ross, Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
Stephen Nusko would like Jonathan Ross to review this change authored by Maksim Sisov.
viz: pass swap_trace_id to trace a frame submission
Pass swap_trace_id from viz thread to gpu and back to
track the flow of the frame submission.
It was chosen to use SkiaOutputSurfaceImplOnGpu::PostSubmit
as it calls to different gpu output device implementations.
That specific point helps to avoid adding this
trace event to different implementations. Moreover, that's
the point where calls to different backends to swap buffers
is called. Happens on the gpu thread.
SkiaOutputDevice::FinishSwapBuffers was also chosen as it's
called by all the gpu implementations after swap is finished.
Happens on the gpu thread.
In case of the sw path, SoftwareOutputSurface was chosen as
the common point.
PS all the backends are free to use FrameData::swap_trace_id
to extend the flow of the trace. My next CL will contain such
a change for Ozone/Wayland.
Bug: 1432641
Change-Id: Iaa8b560778a5a70ecd8dc7ad814381c5228b95d6
---
M base/tracing/protos/chrome_track_event.proto
M components/viz/service/display/direct_renderer.h
M components/viz/service/display/display.cc
M components/viz/service/display/skia_renderer.cc
M components/viz/service/display/software_renderer.cc
M components/viz/service/display/surface_aggregator.cc
M components/viz/service/display_embedder/skia_output_device.cc
M components/viz/service/display_embedder/skia_output_device.h
M components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc
M components/viz/service/display_embedder/software_output_surface.cc
M components/viz/service/display_embedder/software_output_surface.h
M gpu/command_buffer/common/swap_buffers_complete_params.h
M ui/gfx/frame_data.h
M ui/gfx/mojom/frame_data.mojom
M ui/gfx/mojom/frame_data_mojom_traits.h
15 files changed, 115 insertions(+), 8 deletions(-)
Stephen Nusko (Gerrit)
Attention is currently required from: Jonathan Ross, Kinuko Yasuda, Maksim Sisov, Nico Weber, Peter McNeeley, Vasiliy Telezhnikov.
1 comment:
Patchset:
> Hmm can you explain a bit more (perhaps we should GVC to quickly jam board this). […]
Hmm I think [1] is strictly better but to be honest, Graphics.Pipeline seems broken on linux already? For example: https://drive.google.com/file/d/1gk7P5Wv64MEF3DhWQvvRHoZmuj2DFlIO/view?usp=sharing where the DidNotProduce doesn't connect.
+jonross@ is might know better here, but it seems like we might need an overhaul to really make Graphics.Pipeline be logically consistent.Jon my conception is that Graphics.Pipeline is from vsync -> presentation. this currently includes drawing between reuse arrows.
This patch is adding a separate flow event that tracks it through the SKIA swap id. I suggested joining them up with terminating flow (so that each frame is distinct), but this does mean that we remove the connection between frames being reused (is this ever useful? Couldn't this just be the default assumption?)
The other option is to leave it open (non terminating) but then you have forking flows which add visual noise.
It also appears in the example trace and in other traces I got from linux that Graphics.Pipeline is already not doing a great job of tracking frames when DidNotProduce occurs (the sending of DidNotProduce doesn't connect to the request for it).
Should we
a) leave these skia trace events as a separate unconnected flow (I would suggest not naming them graphics.pipeline then)
b) use terminating flow and lose the connection when reusing frames (seems fine?)
c) use non terminating flow and increase visual noise + forking flows (also seems fine?)
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Jonathan Ross, Kinuko Yasuda, Nico Weber, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
2 comments:
Patchset:
Hmm I think [1] is strictly better but to be honest, Graphics. […]
Tbh, I'd better have swap and begin frame trace flow separated. It looks much cleaner then.
File ui/gfx/mojom/frame_data.mojom:
Patch Set #8, Line 14:
int64 seq;
Just curious.. What is the relationship between the `swap_trace_id` and the `sequence_number`? […]
They are not related. The |sequence_number| is updated when the viz surface changes its size property.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Jonathan Ross (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Nico Weber, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
Tbh, I'd better have swap and begin frame trace flow separated. It looks much cleaner then.
I also concur that [1] was strictly better. Graphics.Pipeline is pretty old and hearing that is has some logical inconsistencies, and some failures on Linux, isn't the biggest surprise.
The frame reuse flow has been used in debugging some issues. However with the strict terminations in [1] I think that seeing no immediate flow to Skia would implicitly mean we are reusing a frame. That should be alright. And if it becomes an issue we could look to adding a separate track for the duration in which we use/reuse a given frame.
So I'd support b)
Afterwards we could figure out the DidNotProduce issues
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Jonathan Ross, Kinuko Yasuda, Nico Weber, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
I also concur that [1] was strictly better. Graphics. […]
Thanks. The latest patch set has the [1] option implemented. ptal
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Nico Weber, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
Maksim Sisov removed Jonathan Ross from this change.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Mikhail Khokhlov, Mitsuru Oshima, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
Maksim Sisov would like Mitsuru Oshima and Mikhail Khokhlov to review this change.
Maksim Sisov removed Nico Weber from this change.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Mikhail Khokhlov, Mitsuru Oshima, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
vasilyt@, please review viz.
kinuko@, please review the frame_data.mojom*
khokhlov@, please review chrome_track_event.proto
oshima@, please review frame_data.h
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Mikhail Khokhlov, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
Mitsuru Oshima would like Peter McNeeley to review this change authored by Maksim Sisov.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Mikhail Khokhlov, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Mikhail Khokhlov, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
Patchset:
petermcneeley@ would be better reviewer.
and I can approve once he approves.
Peter McNeeley (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Mikhail Khokhlov, Paul Irish, Stephen Nusko, Vasiliy Telezhnikov.
1 comment:
File components/viz/service/display_embedder/software_output_surface.cc:
This captures everything by reference is this really required. I understand this is part of tracing so less risk but still this seems heavy handed.
Here is the guidelines:
https://google.github.io/styleguide/cppguide.html#Lambda_expressions
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Maksim Sisov, Mikhail Khokhlov, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
Maksim Sisov uploaded patch set #9 to this change.
15 files changed, 121 insertions(+), 12 deletions(-)
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Kinuko Yasuda, Mikhail Khokhlov, Paul Irish, Peter McNeeley, Stephen Nusko, Vasiliy Telezhnikov.
Patch set 9:Commit-Queue +1
1 comment:
File components/viz/service/display_embedder/software_output_surface.cc:
This captures everything by reference is this really required. […]
Oh. That's a great catch! I just blindly copied that from the viz::SurfaceAggregator. Indeed, referencing everything is not required here.. Moreover, passing by value would be a better preference.
Done!
To view, visit change 4647373. To unsubscribe, or for help writing mail filters, visit settings.