Skip to content

publish cl_khr_external_memory_android_hardware_buffer #1367

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

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented May 3, 2025

Publish the cl_khr_external_memory_android_hardware_buffer extension (experimental).

Copy link
Contributor Author

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a few comments and questions I found while editing this specification.

@bashbaug
Copy link
Contributor Author

bashbaug commented Jun 3, 2025

I ended up completely refactoring the error conditions for clCreateBufferWithProperties and clCreateImageWithProperties as part of this PR, following the guideance from #1320. This makes for bigger diffs, but I think the end result is much more readable. Please take a look.

Copy link
Contributor Author

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are two minor issues I found while refactoring the error conditions for clCreateImageWithProperties.

Comment on lines 707 to 708
** if _properties_ includes an AHardwareBuffer external memory handle and
_size_ is non-zero and greater than the AHardwareBuffer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few additional questions about this error code:

  1. Elsewhere in this PR we say for the AHardwareBuffer case: "If importing as a buffer, the implementation will infer the size of the buffer." What does this mean for the passed-in size? Specifically, what happens if the passed-in size is smaller than the size of the AHardwareBuffer? Does the inferred size replace the passed-in size?
  2. In the AHardwareBuffer case is a size of zero valid? Note that this is a CL_INVALID_BUFFER_SIZE error in general (without AHardwareBuffer), so we would need to explicitly allow this if a size of zero is valid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agree that for the AHardwareBuffer the passed-in size should be always zero and the buffer size inferred, so we should remove the CL_INVALID_BUFFER_SIZE error for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I think I have clarified this now - please take a look.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think now it is good. The sample code of the extension should also be changed as the size is still being passed to clCreateBufferWithProperties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I fixed the one example code snip I found. Please take a look - is this the only one? dbe13e7

@github-project-automation github-project-automation bot moved this from In progress to Reviewer approved in OpenCL specification maintenance Jul 1, 2025
@paulfradgley paulfradgley self-requested a review July 1, 2025 15:35
@bashbaug
Copy link
Contributor Author

bashbaug commented Jul 1, 2025

Merging as discussed in the July 1st teleconference.

@bashbaug bashbaug merged commit 6ec237b into KhronosGroup:main Jul 1, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from Reviewer approved to Done in OpenCL specification maintenance Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants