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

Extensibility of wgpuSurfaceGetCurrentTexture #261

Closed
1 task
kainino0x opened this issue Jan 5, 2024 · 5 comments
Closed
1 task

Extensibility of wgpuSurfaceGetCurrentTexture #261

kainino0x opened this issue Jan 5, 2024 · 5 comments
Labels
extensibility Adding features without breaking API changes presentation Presenting images to surfaces like windows and canvases

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Jan 5, 2024

Meeting Jan 4:

  • Make WGPUSurfaceTexture extensible so user can request extra stuff in the future?
  • KN: Prefer to make it extensible just in case
  • (discussion, don’t know of a case where we might extend it)
  • Keep as is, but make WGPUSurfaceTexture extensible
  • Related to: Should more functions take extensible structs? #216
    • Actually we already talked about making GetCurrentTexture extensible and it seems we wanted to do that.

(Actually looking at the comment there I'm not sure if I meant "OK to make extensible" or "OK to not make extensible". I suspect I actually meant "OK to not make extensible" which is the opposite of what I thought this morning.)

typedef struct WGPUSurfaceTexture {
    WGPUChainedStruct * nextInChain;
    WGPUTexture texture;
    WGPUSurfaceGetCurrentTextureStatus status;
} WGPUSurfaceTexture WGPU_STRUCTURE_ATTRIBUTE;

Though writing this out, I realize making WGPUSurfaceTexture extensible isn't necessarily the same as making GetCurrentTexture extensible, since WGPUSurfaceTexture is an output structure. Nothing stops us from extending it with input components (making it a mixed-input-output structure) but the name "WGPUSurfaceTexture" doesn't make much sense in that case.

Right now we have lots of in-extensibility but only some out-extensibility: wgpuGetInstanceFeatures, wgpuAdapterGetProperties, wgpuAdapterGetLimits, wgpuDeviceGetLimits, wgpuSurfaceGetCapabilities.
None of these are mixed-input-output but they could be if we needed them to be.
(thinking about made me think of #260 ^_^)

A few options:

0a - Do not worry about extensibility now and make a GetCurrentTexture2() if we ever need it.

typedef struct WGPUSurfaceTexture {
    WGPUTexture texture;
    WGPUSurfaceGetCurrentTextureStatus status;
} WGPUSurfaceTexture WGPU_STRUCTURE_ATTRIBUTE;

void wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUSurfaceTexture *);

0b - Alternate form.

WGPUSurfaceTexture wgpuSurfaceGetCurrentTexture(WGPUSurface);

1 - Do the thing we discussed and defer a decision on whether to allow input members in the chain. (EDIT: fixed chain constness here)

typedef struct WGPUSurfaceTexture {
    WGPUChainedStructOut * nextInChain; // tbd if this is out-only or in-out
    WGPUTexture texture;
    WGPUSurfaceGetCurrentTextureStatus status;
} WGPUSurfaceTexture WGPU_STRUCTURE_ATTRIBUTE;

void wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUSurfaceTexture *);

2 - Explicitly make it mixed in-out. I struggle with naming in this one. (EDIT: fixed chain constness here)

typedef struct WGPUGetCurrentTextureArguments {
    WGPUChainedStructOut * nextInChain; // chain may have both in and out members
    WGPUTexture texture; // out
    WGPUSurfaceGetCurrentTextureStatus status; // out
} WGPUGetCurrentTextureArguments WGPU_STRUCTURE_ATTRIBUTE;

void wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureArguments *);

3a - Split in and out and make only 'in' extensible, but notably the 'in' structs could contain out-pointers:

typedef struct WGPUSurfaceTexture {
    WGPUTexture texture;
    WGPUSurfaceGetCurrentTextureStatus status;
} WGPUSurfaceTexture WGPU_STRUCTURE_ATTRIBUTE;

typedef struct WGPUGetCurrentTextureInfo {
    WGPUChainedStruct const * nextInChain;
    // (no other members)
} WGPUGetCurrentTextureInfo WGPU_STRUCTURE_ATTRIBUTE;

WGPUSurfaceTexture wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureInfo const *);

// Example of how an out-pointer would work:

// Can be chained in WGPUGetCurrentTextureInfo
typedef struct WGPUGetCurrentTextureMoreInfo {
    WGPUChainedStruct const * nextInChain;
    WGPUBool preferOverlayCandidate; // in
    size_t * indexInNativeSwapChain; // out pointer, optional
    WGPUBool * isOverlayCandidate; // out pointer, optional
};

3b - Technically the outputs could all be in the Info struct.

typedef struct WGPUGetCurrentTextureInfo {
    WGPUChainedStruct const * nextInChain;
    WGPUTexture * texture; // out pointer, required
    WGPUSurfaceGetCurrentTextureStatus * status; // out pointer, probably required
} WGPUGetCurrentTextureInfo WGPU_STRUCTURE_ATTRIBUTE;

WGPUStatus wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureInfo const *);

There are a bunch more permutations on this (like having both an extensible-in and extensible-out struct) but you get the idea. I'm stopping here because 3a seems actually decent.

  • Also note options 1, 2, and 3a will require adding a new GetCurrentTextureStatus for a validation error (like passing an invalid SType).
@kainino0x kainino0x added presentation Presenting images to surfaces like windows and canvases extensibility Adding features without breaking API changes labels Jan 5, 2024
@kainino0x
Copy link
Collaborator Author

  • Related to: Should more functions take extensible structs? #216
    • Actually we already talked about making GetCurrentTexture extensible and it seems we wanted to do that.

Actually looking at the comment there I'm not sure if I meant "OK to make extensible" or "OK to not make extensible". I suspect I actually meant "OK to not make extensible" which is the opposite of what I thought this morning.

@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Jan 11, 2024
@kainino0x
Copy link
Collaborator Author

Jan 25 meeting:

  • Does WGPUSurfaceTexture need to be extensible? Maybe
  • RM: Make WGPUSurfaceTexture a managed object? Add methods to it to return extra info
    • RM: How would it behave on Present()?
    • KN: Probably it doesn’t change, only its Texture changes
  • Return WGPUSurfaceTexture+Status? Or make status a getter on WGPUSurfaceTexture?
    • RM: Prefer WGPUSurfaceTexture+Status
    • KN: Prefer making status a getter so we don’t have so many definitions
    • KN: Then would be no extensibility if status is an error status
  • KN: Occurs to me we don’t have output extensibility for any of the callbacks right now
  • KN: Not sure this is worth the complexity
    • RM: Find 3a somewhat confusing because both in and out are in the same structure
    • RM: Also unusual to return a struct by value
  • KN: Would prefer to have both input and output extensibility over adding a managed WGPUSurfaceTexture (especially if WGPUSurfaceTexture+Status)
    • (Wish we could just have WGPUSurfaceTexture be a subclass of WGPUTexture.)
  • Separate in-extensibility and out-extensibility
  • void wgpuSurfaceGetCurrentTexture(WGPUSurface, WGPUGetCurrentTextureDescriptor const *, WGPUSurfaceTexture *);

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done and removed !discuss Needs discussion (at meeting or online) labels Jan 25, 2024
@Kangz
Copy link
Collaborator

Kangz commented Aug 28, 2024

Likewise, should be have some form of both in and out extensibility for present? For example to request present timings.

@Kangz Kangz added !discuss Needs discussion (at meeting or online) and removed has resolution Issue is resolved, just needs to be done labels Aug 28, 2024
@kainino0x
Copy link
Collaborator Author

That's a good idea. Let me file that under #216.

@kainino0x kainino0x added has resolution Issue is resolved, just needs to be done and removed !discuss Needs discussion (at meeting or online) labels Aug 30, 2024
@kainino0x kainino0x added the !discuss Needs discussion (at meeting or online) label Nov 7, 2024
@kainino0x kainino0x removed the has resolution Issue is resolved, just needs to be done label Nov 9, 2024
@kainino0x
Copy link
Collaborator Author

kainino0x commented Dec 9, 2024

WGPUSurfaceTexture is extensible; for the rest, we'll deal with it later per #216 (comment).

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

No branches or pull requests

2 participants