Prevent unnecessary memory copying in nvwave

[gexgd0419]

Link to issue number:

None

Summary of the issue:

#18207 introduced these two lines in nvwave.py:

		if not isinstance(data, bytes):
			data = string_at(data, size)

string_at creates another copy of the data buffer stored as bytes, which is not needed, since it will only be passed to wasPlay_feed. As many built-in synths pass in c_void_p or a ctype array instead of bytes, this would create a lot of unnecessary memory copies. data should be passed to wasPlay_feed directly if possible.

As it turned out, those two lines were added because of the argtypes set for wasPlay_feed:

wasPlay_feed = dll.wasPlay_feed
wasPlay_feed.restype = HRESULT
wasPlay_feed.argtypes = (
	HWasapiPlayer,  # player
	c_char_p,  # data
	c_uint,  # size
	POINTER(c_uint),  # id
)

c_void_p cannot be implicitly converted to c_char_p, so string_at was added as a fix.

Description of user facing changes:

None.

Description of developer facing changes:

None. data still accepts anything convertible to c_void_p and bytes.

Description of development approach:

This PR chooses another way to fix that problem: cast data to c_char_p explicitly. Anything that can be converted to c_void_p can be converted to c_char_p this way, including c_void_p, bytes, and ctypes array.

Testing strategy:

Tested manually.

Known issues with pull request:

None

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

[LeonarddeR]

I think it's better to keep the cast out of the wasPlay_feed call, otherwise it is hard to track issues caused by cast failures.

[SaschaCowley]

This change would have been made due to a ctypes type incompatibility, but I don't remember the specific trigger. I'll see if I can find it, because I am reasonably sure I tried this and it didn't work.

[SaschaCowley]

Is there a reason not to perform this cast directly in the call to wasapi.wasPlay_feed?

[gexgd0419]

See comment from @LeonarddeR :

I think it's better to keep the cast out of the wasPlay_feed call, otherwise it is hard to track issues caused by cast failures.

[gexgd0419]

I tried the launcher built by GitHub Actions. At least all the built-in synths work on my system.

Before the 64-bit transition, wasPlay_feed had no argtypes, which meant anything could be passed as data. Now the data is set to be c_char_p (to be more precise, it's pointer to unsigned char).

After searching the codebase, I found those possible data types for the data argument, from all the built-in synths.

• bytes
• c_void_p
• array of c_short
• None

And casting to c_char_p works on all of them, although static type checkers might disagree.

I might need to dig deeper.

[gexgd0419]

@SaschaCowley Or what about changing the argtype of data to c_void_p instead of c_char_p?

Using c_char_p for unsigned char* is not accurate anyway, since c_char_p is assumed to point to a zero-terminated string, while wave buffers are not necessarily terminated with zero. From documentation of ctypes.c_char_p:

Represents the C char* datatype when it points to a zero-terminated string. For a general character pointer that may also point to binary data, POINTER(c_char) must be used.

Wave buffers can be an array of raw bytes, or an array of 16-bit samples, but actually we don't care about the exact "data type", we just want to send the raw bytes to the system. If we use c_void_p, we can save a cast call on the Python's side. Or if consistency with the C++ part is preferred, use POINTER(c_ubyte) with an explicit cast.

[seanbudd]

@gexgd0419 - is this ready for review?

[SaschaCowley]

I think this was waiting on me to try and find why we went with this approach in the first place. Unfortunately I've lost that branch, so I'm not sure.