-
Notifications
You must be signed in to change notification settings - Fork 130
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
[XB1] Add device lost handling #4628
base: 24.lts.1+
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
9582b7d
to
ebd1127
Compare
ebd1127
to
f5df859
Compare
@jasonzhangxx Can you take a look at this? |
starboard/egl.h
Outdated
@@ -62,6 +62,10 @@ typedef void* SbEglSurface; | |||
typedef void* SbEglSync; | |||
typedef uint64_t SbEglTime; | |||
|
|||
#ifdef HANDLE_EGL_CONTEXT_LOST | |||
void SbHandleEglContextLost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a starboard interface change. It would need approval from @y4vor, @kaidokert, @xiaomings. I don't think we should change it at this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@y4vor, @kaidokert, @xiaomings please review this PR. It's extremely important to us, becuase we beleive this fix would really decrease the number of Cobalt crashes on Xboxes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do Starboard API changes. Try using a Starboard extension. Take a look at the following example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've remade the PR using Starboard extension. Please, take a look.
ApplicationUwp::Get()->Inject( | ||
new ApplicationUwp::Event(kSbEventTypeReveal, NULL, NULL)); | ||
ApplicationUwp::Get()->Inject( | ||
new ApplicationUwp::Event(kSbEventTypeFocus, NULL, NULL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've seen regression b/370914120 for a similar change https://lbshell-internal-review.git.corp.google.com/c/cobalt_src/+/281544. Is that a safe way to reset DX11 device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonzhangxx, unfortunately we don't have access to b/370914120, could you give it us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't guaranty that this approach will not have any issues, however, DX11 device lost renders cobalt completely unfunctional, so that it can't be any worse. And right now, DX11 device lost is not handled at all.
f5df859
to
c36afe7
Compare
starboard/egl.h
Outdated
@@ -62,6 +62,10 @@ typedef void* SbEglSurface; | |||
typedef void* SbEglSync; | |||
typedef uint64_t SbEglTime; | |||
|
|||
#ifdef HANDLE_EGL_CONTEXT_LOST | |||
void SbHandleEglContextLost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@y4vor, @kaidokert, @xiaomings please review this PR. It's extremely important to us, becuase we beleive this fix would really decrease the number of Cobalt crashes on Xboxes
@@ -198,6 +198,8 @@ static_library("starboard_platform") { | |||
"shared/configuration.cc", | |||
"shared/configuration.h", | |||
"shared/configuration_constants.cc", | |||
"shared/egl_context_lost_handler.cc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an extension test.
c36afe7
to
a201f1c
Compare
This change adds handler extension that triggers graphics recreation if EGL_CONTEXT_LOST error occurred. b/329326128
a201f1c
to
6f0c36e
Compare
This change adds handler function that trigger graphics recreation if EGL_CONTEXT_LOST error occurred.
b/329326128