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

wpe_fdo_egl_exported_image: add destroy-notify callback support. #45

Closed

Conversation

zdobersek
Copy link
Contributor

This should allow caching and subsequently safely destroying relevant
resources in user-facing applications.

This should allow caching and subsequently safely destroying relevant
resources in user-facing applications.
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

More than requesting changes, I would like some opinions on whether it would make sense to run the destroy-notify callback when calling wpe_fdo_egl_exported_image_set_destroy_notify() more than once on the same image object (see below).

wpe_fdo_egl_exported_image_set_destroy_notify(struct wpe_fdo_egl_exported_image* image, wpe_fdo_egl_exported_image_destroy_notify_t destroyNotify, void* destroyData)
{
image->destroyNotify = destroyNotify;
image->destroyData = destroyData;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense that if a destroyNotify was already set previously, that the old callback is called before storing the new values? Not doing that could easily result in leaking the data passed as destroyData, if the callback is supposed to free it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense. I'll update it to do that.

@@ -30,6 +30,14 @@

extern "C" {

__attribute__((visibility("default")))
void
wpe_fdo_egl_exported_image_set_destroy_notify(struct wpe_fdo_egl_exported_image* image, wpe_fdo_egl_exported_image_destroy_notify_t destroyNotify, void* destroyData)
Copy link
Member

Choose a reason for hiding this comment

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

This could use an API documentation comment. If we the missing ones little by little with each PR we would incrementally tackle #27 — I wouldn't block merging this PR due to this, though 😉

@aperezdc
Copy link
Member

@zdobersek Do you have time to do the changes, or should we merge this as-is? I could do a follow-up PR myself with them myself.

@aperezdc
Copy link
Member

This has been stagnating for a long while so I am going to close it for now. Feel free to update and reopen again if/when this is relevant at some point.

@aperezdc aperezdc closed this Mar 15, 2021
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