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

IDWriteGlyphRunAnalysis::CreateAlphaTexture uses a &mut [u8] slice, which implies unnecessary initialization of its contents #2106

Open
ennis opened this issue Oct 19, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@ennis
Copy link

ennis commented Oct 19, 2022

It seems that the signature of IDWriteGlyphRunAnalysis::CreateAlphaTexture was changed to take a &mut [u8] slice as an output parameter.
If I'm not mistaken, such slices always contain initialized data, which shouldn't be necessary since CreateAlphaTexture should write to the buffer represented by the slice. IIRC, the previous signature took a combination of *mut u8 and a usize to specify the size of the output buffer, which allowed the use of uninitialized memory (via Vec::with_capacity + set_len or MaybeUninit).

There might be a reason for this change but I couldn't find anything in the issues or the release notes.

@kennykerr
Copy link
Collaborator

The win32 metadata provides information about certain parameters representing arrays that I have transformed into Rust slices for convenience. For example:

unsafe HRESULT CreateAlphaTexture([In] DWRITE_TEXTURE_TYPE textureType, [In][Const] RECT* textureBounds, [Out][MemorySize(BytesParamIndex = 3)] byte* alphaValues, [In] uint bufferSize);

Note how the alphaValues and bufferSize parameters are logically associated with the MemorySize attribute.

which allowed the use of uninitialized memory (via Vec::with_capacity + set_len or MaybeUninit)

I don't think that Vec provides uninitialized memory. I have had another request to support MaybeUninit, but it gets complicated. #2065

Is the concern here that this would be costly? Does the buffer required by CreateAlphaTexture tend to be large?

@kennykerr kennykerr added the question Further information is requested label Oct 20, 2022
@ennis
Copy link
Author

ennis commented Oct 21, 2022

Is the concern here that this would be costly? Does the buffer required by CreateAlphaTexture tend to be large?

Yes, my main concern is the additional cost of having to initialize the output buffer, although in the particular case of CreateAlphaTexture the cost of the function itself probably dwarfs the cost of initializing the buffer. That said there might be other functions where it's more problematic.

Here's the code I used before:

unsafe {
    let mut data = Vec::with_capacity(buffer_size);
    self.analysis.CreateAlphaTexture(texture_type, &bounds, data.as_mut_ptr(), buffer_size as u32)
                 .expect("CreateAlphaTexture failed");
    data.set_len(buffer_size);
}

This skips the initialization of the vec contents, and I think this is OK with regard to UB (a similar use case is presented in the docs for Vec::set_len).

Now I need to use this:

let mut data = vec![0u8; buffer_size];
self.analysis.CreateAlphaTexture(texture_type, &bounds, &mut data).expect("CreateAlphaTexture failed");

and pay the (negligible, in this case) cost of initialization.

@kennykerr kennykerr added enhancement New feature or request and removed question Further information is requested labels Oct 22, 2022
@kennykerr
Copy link
Collaborator

kennykerr commented Oct 24, 2022

Got it, thanks for the example. I may have to turn off the slice transformation for out parameters, only applying it to input parameters...

@poliorcetics
Copy link
Contributor

Slice transformation for out parameters could be &[MaybeUninit<T>] since MaybeUninit is repr(transparent) for T, casting a slice from &[MaybeUninit<T>] to &[T] should be safe as long as the out result and length written are checked ?

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

No branches or pull requests

3 participants