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

Replay frame delimiters for EGL #2708

Merged
merged 1 commit into from
May 13, 2019
Merged

Conversation

hevrard
Copy link
Contributor

@hevrard hevrard commented Apr 5, 2019

When export_replay creates a standalone APK to replay a trace, we want
to be able to trace the replay itself, and we want to detect frame
delimiters during this tracing. Hence the replay of EGL frame
delimiters.

Copy link
Member

@pmuetschard pmuetschard left a comment

Choose a reason for hiding this comment

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

I am very worried that this will cause issues for regular replays. As we've not been replaying any EGL calls, we are not remapping the EGLDisplay, nor the EGLSurface. Simply using the pointers that the application had received at trace time during replay seems dangerous and I suspect would cause the replay to crash on some drivers.

I would recommend that instead we add a synthetic call that will do the eglSwapBuffers call on the replay surface. This way, we also would have the additional benefit of being able to only add the calls when we need them and not on every replay.

@hevrard
Copy link
Contributor Author

hevrard commented Apr 9, 2019

OK, starting to replay some EGL calls may cause mayhem in the replay. I will have a go at writing synthetic calls later as I focus on testing now. Thanks for the feedback!

@hevrard hevrard force-pushed the replay-frame-starters branch from 8f82536 to 7ab8c36 Compare April 23, 2019 15:16
@hevrard
Copy link
Contributor Author

hevrard commented Apr 23, 2019

@pmuetschard can you tell me if this is the right direction?

I'm not sure if you meant to have synthetics for the actual EGL calls, or for other made-up commands that would replace the EGL SwapBuffers calls?

@pmuetschard
Copy link
Member

Yes, this is getting there. However, you still need to actually have the replayer call eglSwapBuffers, otherwhise this will not do anything. To do that add a swapBuffers method to the renderer and have the synthetic function call it.

Also, consider making the synthetic a separate API function and have the API call it. Similar to how the eglMakeCurrent calls the bindRenderer synthetic. I'm not certain, however, if that is necessary/the right thing, but please consider it and decide.

@hevrard
Copy link
Contributor Author

hevrard commented Apr 25, 2019

Let me try to wrap up a chat with Ben:

  • GAPIR needs to make a call to signal a frame delimiter
  • GAPII needs to intercept that call as a frame delimiter
  • This call may be eglSwapBuffers, but it can also be:
    i. an other API call (e.g. glFinish), such that it can be replayed by the driver with minimal impact and without having to deal with EGL
    ii. a new made-up API call implemented by GAPII, e.g. gapidFrameDelimiter, which acts as a delimiter when intercepted, but which is not passed onto the driver

(i) may be an issue as we basically modify the trace to replace an EGL call by a different GL call which may have impact on various things. Also, glFinish would need to be marked as a frame delimiter, which may create weird frames delimitations in applications that expect frame delimiters to be only at EGLSwapBuffers.
(ii) may be an issue for tools other than GAPID, which will not recognize gapidFrameDelimiter

A solution is to byte the bullet and do tracing and replay of all EGL-related things -- that will take some time.

In the meantime, shall we go with option (2)?

@ben-clayton
Copy link
Contributor

ben-clayton commented Apr 25, 2019 via email

@AWoloszyn
Copy link
Contributor

Following up on @ben-clayton's comment.
a is what we would do in an ideal world. This would mean we have to extract a significant amount of information from the original application in order to correctly replay. We would need to have surface/display information that we are currently lacking, and then re-create it correctly/exactly. Furthermore we would have to track this over time in order to handle cases like screen rotation/etc.

glFinish should cause a flush of the GPU, meaning we would be introducing stalls into the pipeline, so that is not an ideal option.

gapidFrameDelimiter may work, but has an the problem that we will still be changing the behavior of the program. Ignoring swap may have unintended side-effects. However this may work if we just want to be able to trace a replay for debug purposes (rather than performance purposes).

@pmuetschard
Copy link
Member

I'd say we simplify it all and simply call swap buffers on the currently bound renderer (which has a display and a single surface). Isn't that what we are assuming already anyways? I mean, how are we currently associating an eglSwapBuffers call with a context? AFAICT, we just assume that it's called from the same thread that owns the context.

Maybe there will be that one app that'll swap buffers from a different thread, but it won't hurt to then just ignore it in the replay if there is no renderer bound on the current thread.

@hevrard
Copy link
Contributor Author

hevrard commented Apr 29, 2019

@pmuetschard I think I implemented what you had in mind, and now tracing replay on GLES seems to work! Could you have a look and let me know what you think?

A few comments:

  • how can we know which EGL context is currently bound? (Here I iterate over renderers until one of them has a valid surface to call swapBuffers on)
  • if the current approach looks OK, we should we do something similar for eglSwapBuffersWithDamageKHR().
  • only the Android platform actually replays the eglSwapBuffers, we can extend to other platforms
  • do we want to not replay such frame delimiters unless we need to (e.g. generic replay does not need to, but a replay from a standalone APK needs to)?

@hevrard hevrard requested a review from pmuetschard May 1, 2019 14:03
@hevrard hevrard force-pushed the replay-frame-starters branch from 1a65b93 to 3dc7eab Compare May 8, 2019 15:52
Copy link
Contributor

@ben-clayton ben-clayton left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@pmuetschard pmuetschard left a comment

Choose a reason for hiding this comment

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

a few nits, and a simple change.

Please squash when you commit, as there is some back-and-forth in this PR.

gapis/api/gles/api/egl.api Show resolved Hide resolved
gapis/api/gles/custom_replay.go Show resolved Hide resolved
gapis/api/gles/synthetic.api Outdated Show resolved Hide resolved
@hevrard
Copy link
Contributor Author

hevrard commented May 10, 2019

Thanks @pmuetschard for the review!

Could you also let me know what you think on these points?

  • if the current approach looks OK, should we do something similar for eglSwapBuffersWithDamageKHR()?
  • only the Android platform actually replays the eglSwapBuffers, should we extend to other platforms?
  • do we want to not replay such frame delimiters unless we need to (e.g. generic replay does not need to, but a replay from a standalone APK needs to)?

@pmuetschard
Copy link
Member

  • if the current approach looks OK, should we do something similar for eglSwapBuffersWithDamageKHR()?

i don't think so.

  • only the Android platform actually replays the eglSwapBuffers, should we extend to other platforms?

Only Android for now is fine with me.

  • do we want to not replay such frame delimiters unless we need to (e.g. generic replay does not need to, but a replay from a standalone APK needs to)?

For now, unless we find a reason not to replay them, let's keep it as you have implemented it.

EGL frame delimiters need to be replayed to enable frame limit
detection, used in both:
- tracing of a replay
- profiling: some counters are (re-)set at frame boundaries

The replay of EGL Swap Buffers is implemented using @Custom which
calls a @Synthetic primitive. Only Android platform actually replay
this call for now.
@hevrard hevrard force-pushed the replay-frame-starters branch from 8f259f4 to 3ebf6c5 Compare May 13, 2019 09:55
@hevrard hevrard merged commit 5bcf385 into google:master May 13, 2019
@hevrard hevrard deleted the replay-frame-starters branch June 21, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants