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

Zero copy and get direct3d device and context #90

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

mycrl
Copy link
Contributor

@mycrl mycrl commented Oct 19, 2024

Zero copy texture

First, get ID3D11Texture2D directly via Frame, without making a copy internally, and just return the texture in the current frame.

https://github.com/mycrl/windows-capture/blob/main/src/frame.rs#L181

pub unsafe fn as_raw_texture(&self) -> &ID3D11Texture2D {
    &self.frame_texture
}

Then, it's not really necessary to clone once to get surface and texture, although ID3D11Texture2D and IDirect3DSurface are actually pointers internally, and it doesn't really matter if you return a reference or not, I think it's good to return a reference to clarify the semantics, and to indicate that surface and texture are only available during the current frame's lifetime.

Also I removed the CPU write flag for textures, because for the usage scenario here, there is no need to make the CPU writable to the texture, only readable, and keeping only the read flag makes it easier for the Direct3D underlayer to deal with resource mutexes and improve performance, Because if it is possible to write, the underlayer needs to think more about resource contention and introduce unnecessary locks or similar mechanism.

https://github.com/mycrl/windows-capture/blob/main/src/frame.rs#L205

CPUAccessFlags: D3D11_CPU_ACCESS_READ.0 as u32

External access to Direct3D devices and contexts

Then there's the second big change, adding a RawDirect3DDevice parameter to the new method of the GraphicsCaptureApiHandler trait.

https://github.com/mycrl/windows-capture/blob/main/src/capture.rs#L516

fn new(device: RawDirect3DDevice, flags: Self::Flags) -> Result<Self, Self::Error>;

First, let me explain why this parameter is needed. In direct3d11, devices and contexts are independent, if windows-capture doesn't expose its own devices and contexts, then external parties can't use your textures or have a hard time using your textures, of course, external parties can use the software buffers, but this goes against the zero-copy and performance-first principles.
For example, if you need to use the texture directly for some post-processing, scaling and texture format conversion needs, the only way to do this is to keep creating cross-device shared resource handles for each frame. At the same time, creating shared resource handles for each frame is also a high overhead operation (acceptable when both sides are using dx11).
Therefore, it is necessary to perform this operation in windows-capture's own device and context, because the library aims for high performance and you need to enable users to use your library in a high performance way.

@NiiightmareXD
Copy link
Owner

It looks good, but the only problem I see is having RawDirect3DDevice in the new function parameters. I think this is unnecessary for most people. Do you have any less verbose way to get this?

@mycrl
Copy link
Contributor Author

mycrl commented Oct 24, 2024

It looks good, but the only problem I see is having RawDirect3DDevice in the new function parameters. I think this is unnecessary for most people. Do you have any less verbose way to get this?

I've actually thought about this, I actually wanted to implement getting the device and context without breaking the existing api at first, but the existing api looks harder to implement.
But there could be several alternatives:

1, it's the kind I'm implementing now, by passing one more parameter.

2, Do a little wrapping of Flags by wrapping Self::Flags into a struct to be passed into the new method as a new context.

pub struct Context<T> {
    pub flags: T,
    pub device: ID3D11Device,
    pub context: ID3D11DeviceContext,
}

pub trait GraphicsCaptureApiHandler: Sized {
    // Omission of other statements...
    fn new(ctx: Context<Self::Flags>) -> Result<Self, Self::Error>;
}

3, In Settings, the external can pass the device and context by itself, and windows-capture can use the externally provided device instead of creating it by itself.

@NiiightmareXD In my opinion, the first and second are the best, and the third, while feasible, brings a lot of uncontrollable factors with externally created devices, so which do you think is better, or is there a better way to do it.

@NiiightmareXD
Copy link
Owner

It looks good, but the only problem I see is having RawDirect3DDevice in the new function parameters. I think this is unnecessary for most people. Do you have any less verbose way to get this?

I've actually thought about this, I actually wanted to implement getting the device and context without breaking the existing api at first, but the existing api looks harder to implement. But there could be several alternatives:

1, it's the kind I'm implementing now, by passing one more parameter.

2, Do a little wrapping of Flags by wrapping Self::Flags into a struct to be passed into the new method as a new context.

pub struct Context<T> {
    pub flags: T,
    pub device: ID3D11Device,
    pub context: ID3D11DeviceContext,
}

pub trait GraphicsCaptureApiHandler: Sized {
    // Omission of other statements...
    fn new(ctx: Context<Self::Flags>) -> Result<Self, Self::Error>;
}

3, In Settings, the external can pass the device and context by itself, and windows-capture can use the externally provided device instead of creating it by itself.

@NiiightmareXD In my opinion, the first and second are the best, and the third, while feasible, brings a lot of uncontrollable factors with externally created devices, so which do you think is better, or is there a better way to do it.

The first one would be the best but I also like the second one

@mycrl
Copy link
Contributor Author

mycrl commented Oct 24, 2024

The first one would be the best but I also like the second one

@NiiightmareXD So keep this pull request as is?
By the way, if you have any other issues with type or function naming, you can change it as you see fit.

@NiiightmareXD
Copy link
Owner

The first one would be the best but I also like the second one

@NiiightmareXD So keep this pull request as is? By the way, if you have any other issues with type or function naming, you can change it as you see fit.

I'm looking into ways so the function could accept either 1 or 2 arguments and be a generic but I think this is also good, do you want to change anything? Should I merge?

@NiiightmareXD
Copy link
Owner

I think the Context<T> would be better I'm going to implement that, thanks.

@NiiightmareXD NiiightmareXD merged commit 9d3109d into NiiightmareXD:main Oct 24, 2024
2 checks passed
@NiiightmareXD
Copy link
Owner

it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants