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

Remove GetCurrentTextureStatus_OutOfMemory #465

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Dec 9, 2024

This doesn't seem super likely to be handleable distinctly from Lost (surface loss) or losing the device, but if it is, the implementation (wgpu-native) can expose it as an extension. It isn't used in JS or Dawn.

  • LK: Could maybe return a message string? (Though requires a FreeMembers, or a static string.)
  • (... we didn't really discuss this much but I think we thought it wasn't worth it)

@lokokung to confirm because I forgot to take notes

Issue: #401
(dawn bug https://crbug.com/367401498)

This doesn't seem super likely to be handleable distinctly from Lost,
but if it is the implementations (wgpu-native) can expose it as an
extension. It doesn't exist in JS or Dawn.

Issue: 401
@kainino0x kainino0x added the presentation Presenting images to surfaces like windows and canvases label Dec 9, 2024
@kainino0x
Copy link
Collaborator Author

I can't find any places where wgpu could return SurfaceError::OutOfMemory:
https://github.com/search?q=org%3Agfx-rs%20SurfaceError%3A%3AOutOfMemory&type=code
I could be looking in the wrong repos though.
It doesn't exist in wgpu_hal::SurfaceError or wgpu::SurfaceStatus either.
@cwfitzgerald You mentioned it could be returned, is that still true?

Tried looking for historical references but it's difficult because there have been so many different repositories over time.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Dec 11, 2024

  • LK: Could maybe return a message string? (Though requires a FreeMembers, or a static string.)

Loko and I discussed this a bit more, I think it would be pretty annoying to have a string that needs FreeMembers called on it but only in the error case which would make it really difficult to notice if you had a leak.
Making it a static string could work but then there's not that much value in the string, especially if the error cases are pretty bounded.

It does seem like the "Error" case (The surface is not configured, or there was an @ref OutStructChainError) should (1) be open-ended and (2) come with implementation-defined logging.

@kainino0x kainino0x enabled auto-merge (squash) December 11, 2024 00:02
@kainino0x kainino0x merged commit 2e778e4 into webgpu-native:main Dec 11, 2024
5 checks passed
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Dec 11, 2024
@kainino0x kainino0x deleted the texture-status-out-of-memory branch December 11, 2024 02:30
@kainino0x
Copy link
Collaborator Author

but if it is, the implementation (wgpu-native) can expose it as an extension.

Oh I forgot to note, I realized this is not directly possible. There would need to be a way for the application to signal that it wants to know about OutOfMemory; an implementation can't just return it without being asked.
Could be done with an extension to WGPUSurfaceTexture, maybe an extra field telling you the reason for Lost, or something.

@kainino0x
Copy link
Collaborator Author

Dec 12 meeting:

  • CF: wgpu::SurfaceError can come from any arbitrary device error. Something weird happened that we think shouldn't happened, only reasonable conclusion is that there's no memory left to create a simple object. Have been trying to move away from that anyway because we have cases like "create semaphone failed with out of memory" but actually it was device lost. Think any problems we have can be bucketed into surface loss.
  • Still OK to remove.

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presentation Presenting images to surfaces like windows and canvases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants