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

pjsua2: add destroyPlayer to AudioMediaPlayer #3789

Closed
wants to merge 2 commits into from

Conversation

xhit
Copy link
Contributor

@xhit xhit commented Nov 24, 2023

With this, the dev can destroy a specific player when needed without wait GC in some languages with not Dispose

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2023

CLA assistant check
All committers have signed the CLA.

@sauwming
Copy link
Member

This looks like a clear violation of Object Oriented Programming.

You're not supposed to destroy something that belongs to another object.

Comment on lines +292 to +300

#if ((defined(_MSVC_LANG) && _MSVC_LANG <= 199711L) || __cplusplus <= 199711L)
/* C++98 does not have Vector::data() */
if (frame->size > 0)
pj_memcpy(frame->buf, &frame_.buf[0], frame->size);
#else
/* Newer than C++98 */
/* Newer than C++98 */
pj_memcpy(frame->buf, frame_.buf.data(), frame->size);
#endif
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: these lines only changes CRLF to LF

@xhit
Copy link
Contributor Author

xhit commented Nov 24, 2023

Fixed.

Also updated comment to specify use with other languages, like Golang, to not wait for GC to acts.

@sauwming sauwming requested a review from nanangizz November 28, 2023 02:30
@sauwming
Copy link
Member

sauwming commented Nov 28, 2023

It's essentially the same as what the class destructor does.

I can understand though why this is needed, especially for GC languages such as Java, Python, C#, when you may need to release resources immediately without having to wait for GC to kick in or manually forcing GC (in C#, like you described, this mechanism will be similar to implementing IDisposable (or AutoClosable in Java)).

Unfortunately, in my opinion, it's unlikely to be merged to the master branch since the implementation is only limited to AudioMediaPlayer, while if we really do need this feature, it probably needs to be more comprehensive than this, i.e.:

  • encompass all PJSUA2 classes (or at least, all the classes that require this feature)
  • need to safeguard against double destruction, and against race condition (i.e. manual disposal and GC-initiated destruction).

@xhit
Copy link
Contributor Author

xhit commented Nov 28, 2023

In C# or Java not problem because the Dispose, but in Go I need to wait the GC.

You are correct about expand more this, I only covers my use case.

And about race conditions, GC will be triggered only if not referenced, so, calling a virtual destructor like this cannot interfere with GC, because to be called the object need to be referenced. I think this should be in dev side.

@sauwming sauwming closed this Dec 11, 2023
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.

3 participants