How to nicely delete the peerconnection

[Francois Temasys]

Hi,

Here is my problem, I have made some changes on the peerconnection_client example. Now my problem is the program is crashing when I try to delete the peer_connection between me and my client.

Here is the log:

SetOutputScaling to left=0 right=0 for channel 0 and ssrc 826111343
SetRenderer 983245153 reuse default channel #0
Segmentation fault (core dumped)

This is called bye:

void Conductor::DeletePeerConnection()
{
    peer_connection_ = NULL;

    active_streams_.clear();
    main_wnd_ -> StopLocalRenderer();
    main_wnd_ -> StopRemoteRenderer();

    peer_connection_factory_ = NULL;
    peer_id_                 = -1;
}

And more exactly: peer_connection_ = NULL; , on peer_connection_->Release();

I don't think you may find the solution directly but is there anything special that I would do wrong. I changed the original code but I try to stay as close as it.

I also made the test of not going to the end of the handshake: I didn't set the remote offer and it wasn't crashing. The camera was turned off and the peerconnection_ completely deleted.

All advice, help is welcome.

PS: I also have one question in the code, why storing the media stream (_active_streams) in a map. All we do with this map is:

talk_base::scoped_refptr<webrtc::MediaStreamInterface> stream =   peer_connection_factory_ -> CreateLocalMediaStream(kStreamLabel);

    stream -> AddTrack(audio_track);
    stream -> AddTrack(video_track);

    if (!peer_connection_ -> AddStream(stream, NULL))
    {
        LOG(LS_ERROR) << "Adding stream to PeerConnection failed";
    }

    typedef std::pair<std::string, talk_base::scoped_refptr<webrtc::MediaStreamInterface> > MediaStreamPair;

    active_streams_.insert(MediaStreamPair(stream -> label(), stream));

Where stream->label() is a constant. So it's a map of always only one element.

[Leighton Carr]

Hi Francois,

My understanding is that there are event handlers that are attached to the peer connection that aren't too happy if you just dump the peer connection object without first closing them gracefully.  You could try stopping the streams and cleaning out the renderer object first, but for my libjingle code I just send the "BYE" message to the other end and then call peer_connection_->Close();  Similarly, I leave the factory alive with the observer, and if the remote peer reconnects, then I just resurrect the old observer / factory, and use them to create a fresh peer connection object which I set peer_connection_ to point to.

My only concern with this approach at the moment is that it might create a memory leak if the old peer connection doesn't clean itself up, but I think it does?

Cheers,

Leighton.

[Francois Temasys]

Hi Leighton,

That was also what I was thinking about. I tried to make the event handlers happier by doing this:

//Delete all peer connections and stop all remote rendering
 _peer_connection->Close();

 _peer_connection->local_streams()->Release();
 _mainWindow->StopLocalRenderer();

 _peer_connection->remote_streams()->Release();
 _mainWindow->StopRemoteRenderer();


 _peer_connection_factory = NULL;

And here is the output:

Warning(webrtcvideoengine.cc:1392): webrtc: GetSendRtcpStatistics: Could not get remote stats
Session:5332449234890083664 Old state:STATE_INPROGRESS New state:STATE_RECEIVEDTERMINATE Type:urn:xmpp:jingle:apps:rtp:1 Transport:http://www.google.com/transport/p2p
Conductor| state Changed
Channel disabled
Stopping playout for channel #0
Clearing option overrides.
Changing voice state, recv=0 send=0
Destroyed channel
Channel disabled
Changing video state, recv=0 send=0
Warning(webrtcvideoengine.cc:1392): webrtc: Stop: Not running
Removing video stream 0 with VideoEngine channel #0
Destroyed channel
Destroy Video Renderer
Destroy Video Renderer

As the log shows, video and voice states are at recv=0 and send=0. And still when I'm doing the _peer_connection = NULL; I receive the seg fault.

In your approach you "postpone" the destruction of the peer_connection. As you say you set the peer_connection_ to point to the newly created peer_connection (created with the old factory). As the peer_connection is a ref_ptr, the "=" operator will first delete the object and then set to the one given (eg: A = B; is in fact "A = NULL; A = B;"). So I don't think you need to be afraid of memory leak but it doesn't really solve my problem.

I am also curious to know when are you effectively deleting the peer_connection? If you want to close your project properly, shouldn't you do it?

[Leighton Carr]

Hi Francois,

I don't think you want to be calling Release() because (and someone feel free to correct me here), that actually tries to release the pointer memory, which might do something a bit funky with the event cascade from the stream receiving new frames.

Here's basically what I do when I want to close a peer_connection for my native video recorder:

// Check Peer Connection

if(peer_connection_.get())

{

peer_connection_->Close();

}

// Check Data

if(data_channel_.get() && recording_data_)

{

if(data_observer_)

{

data_channel_->UnregisterObserver();

data_observer_->~DataObserver();

}

data_channel_->Close();

}

// Check Video

if(remote_renderer_.get() && recording_video_)

{

this->StopRecord();

remote_renderer_->~VideoRenderer();

}

Note:  I don't set the peer_connection or the peer_connection_factory to NULL. 

The only thing you really need to know extra is that calling ~VideoRenderer() also calls:

rendered_track_->RemoveRenderer(this);

::DeleteCriticalSection(&buffer_lock_);

(also note that BYE messages are handled separately to this).

So what I think is happening in your code is that you're asking the Peer Connection to close gracefully, which it tries to go off and do (in another thread), and then you're deleting the pointer in your main thread, which is breaking some of the event handler references.

As you say you set the peer_connection_ to point to the newly created peer_connection (created with the old factory). As the peer_connection is a ref_ptr, the "=" operator will first delete the object and then set to the one given (eg: A = B; is in fact "A = NULL; A = B;"). So I don't think you need to be afraid of memory leak but it doesn't really solve my problem.

I am also curious to know when are you effectively deleting the peer_connection? If you want to close your project properly, shouldn't you do it?

Yep that was what I meant with delaying the deletion of the peer_connection.  Closing my project I just iterate through my map of Observers, send BYEs, close all the peer connections, and then let the rest of the threads close naturally.  I get few funny return codes from closing threads, but it doesn't seem to have any major worries.

Cheers,

Leighton.

[Francois Temasys]

Hi Leighton,

Thank you for sharing pieces of your code.

Now I understand what you mean by closing the peer_connection naturally. But I think the problem isn't really here. The original example dereferance the peer_connection at first, even before stopping the renderers.

Now in my project, I'm not trying to record the data channel and I'm not rendering the stream. I try to reduce as maximum the functionalities of my peer_connection to detect the error.

Could you also share the log produced when you do the peer_connection->Close?

Another query that may be related to my other thread on the mailing list: is there any asynchronous task running for closing the peer_connection?

Also could someone details the several steps involved when the peer_connection is deleted.

Here is the current output I receive:

Destroyed channel

Error(webrtcsession.cc:813): SetAudioPlayout: No audio channel exists.

Warning(webrtcsession.cc:878): SetVideoPlayout: No video channel exists.

RemoteVideoCapturer::Stop

Error(webrtcsession.cc:834): SetAudioSend: No audio channel exists.

Warning(webrtcsession.cc:860): Video not used in this call.

Warning(webrtcsession.cc:893): SetVideoSend: No video channel exists.

It would help to debug by knowing who did call webrtcsession here?

Thank you

[Francois Temasys]

Hi,

Good news I found a "fix" for that issue but it's clearly handmade.

First I was not having this issue when I was deleting my peerconnection linked to nothing (I am rendering only my local).

So the problem could probably come from the remote here is what I did:

In talk/app/webrtc/mediastreamhandler.cc:

void MediaStreamHandlerContainer::TearDown()
{
  for (StreamHandlerList::iterator it = remote_streams_handlers_.begin(); it != remote_streams_handlers_.end(); ++it)
  {
    (*it)->Stop();
//    delete *it;
  }
  remote_streams_handlers_.clear();
  for (StreamHandlerList::iterator it = local_streams_handlers_.begin(); it != local_streams_handlers_.end(); ++it)
  {
    (*it)->Stop();
    delete *it;
  }
  local_streams_handlers_.clear();
}

I don't delete my remote stream handler just stop it.

Then in talk/media/base/capturemanager.cc

CaptureManager::~CaptureManager()
{
  // Since we don't own any of the capturers, all capturers should have been
  // cleaned up before we get here. In fact, in the normal shutdown sequence,
  // all capturers *will* be shut down by now, so trying to stop them here
  // will crash. If we're still tracking any, it's a dangling pointer.
  if (!capture_states_.empty())
  {
//    ASSERT(false && "CaptureManager destructing while still tracking capturers!");
    // Delete remaining VideoCapturerStates, but don't touch the capturers.
    do {
      CaptureStates::iterator it = capture_states_.begin();
      delete it->second;
      capture_states_.erase(it);
    } while (!capture_states_.empty());
  }
}

And now it's working...

I'm just little confusing on few things:

- why doing assert false and using the solution after?

- why am I not crashing when the comments said I should?

After that I checked about the memory leak or if I was still using any bandwith (that would mean my peer_connection is still working), no problem.

Hope it can help

[Arjav Parikh]

Hi Mr. Francois,

I am trying to do the same thing in peerconnection_client code. I am trying to clear video stream resources once "BYE" is received from another peer by calling peer_connection_->RemoveStream(stream) and then calling DeletePeerConnection(). From logs I observe is video channel is disabled. After calling this I am calling ConnectToPeer(peer_id) to reinitiate communication and start video streaming. The issue I face is on next connect I do not see the streaming as the Encoder thread does not get initiated. Can you please help on this. In your case after disconnect were you able to start streaming on next connect (Note: Both the peers should not get disconnected from Signaling Server).

Thanks.