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

update to 2.16.0 #46

Merged
merged 28 commits into from Jun 19, 2019
Merged

update to 2.16.0 #46

merged 28 commits into from Jun 19, 2019

Conversation

msach22
Copy link

@msach22 msach22 commented Nov 1, 2018

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I have resolved any merge conflicts. Confirmation: ____

This fixes issue #___.

What's in this pull request?

Updates library to 2.15.0

...

@msach22 msach22 changed the title update to 2.15.0 update to 2.16.0 May 2, 2019
@msach22
Copy link
Author

msach22 commented Jun 5, 2019

@jtiet shouldn't we use 2.16.1 so we don't have the logging issue that we had with 2.16.0?

@jtiet
Copy link
Contributor

jtiet commented Jun 5, 2019

Thanks for the reminder @msach22, I changed it to use 2.16.1.

@@ -166,12 +175,33 @@
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
E33A3600D0F40630ED4C88C9 /* libPods-OTAcceleratorCoreTests.a in Frameworks */,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a good practice to run pod deintegrate before checking the project file.

This way we remove pod reference in the project file

Copy link
Contributor

@jtiet jtiet Jun 13, 2019

Choose a reason for hiding this comment

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

Thank you for the suggestion @robjperez! Fixed, please check again

@@ -140,8 +141,8 @@ - (int32_t)stopCapture
_capturing = NO;

dispatch_sync(_queue, ^{
if (_timer) {
dispatch_source_cancel(_timer);
if (self->_timer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@jtiet jtiet Jun 13, 2019

Choose a reason for hiding this comment

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

thanks @robjperez. Fixed, please check again

dispatch_sync(_queue, ^{
if (_timer) {
dispatch_source_cancel(_timer);
__strong __typeof__(self) *strongSelf = weakSelf;
Copy link
Contributor

@robjperez robjperez Jun 13, 2019

Choose a reason for hiding this comment

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

Actually you don't need a strong reference here.

weak reference means that if when you execute this block, self is already disposed, it won't crash when you try to access timer property.

IMHO, it should be something like this:

__weak __typeof__(self) *weakSelf = self;
dispatch_sync(_queue, ^{
      dispatch_source_cancel([weakSelf timer]);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, fixed it.

Copy link
Contributor

@jtiet jtiet Jun 13, 2019

Choose a reason for hiding this comment

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

Could you please share why a weak reference is needed in this case? I understand that weak should be used within blocks because if self references the block and the block references self, this creates a reference loop. In this case, it looks like the block would retain self, but since the block is not assigned to a variable, I don't think that self retains the block. So if one or the other is released, everything gets deallocated properly.

So that would mean the following should work:

    _capturing = NO;
    dispatch_sync(_queue, ^{
        if (self->_timer) {
            dispatch_source_cancel(self->_timer);
        }
    });

I'd also like to add that since _timer is a property, [weakSelf _timer] would throw the following error (in travis tests):
bad receiver type 'OTScreenCapture *const __weak *'

Copy link
Contributor

Choose a reason for hiding this comment

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

the weak reference is useful in two ways. one is for the capture part, and to prevent cycles that could make self never be released as you have explained.

The other one is to prevent crashes. As blocks are executed in a different thread, when the block is executed it could happen that self has been released so self->_timer will have undefined behavior. That's where weak helps, if you have a weak reference, if self is released, when you do [weakSelf timer] it won't crash since weakSelf is set to nil automatically. (you should not use direct dereference too).

In this particular case probably it doesn't matter since it is a dispatch_sync block and self will not be released when the block is executed. But it is usually a good practice to do the weak thing in any case. That way, if someone in the future, decides to change dispatch_sync with dispatch_async the block is safe.

@robjperez robjperez self-requested a review June 13, 2019 17:43
@jtiet jtiet requested a review from robjperez June 18, 2019 16:04
@jtiet jtiet merged commit 032c484 into opentok:master Jun 19, 2019
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.

None yet

3 participants