Fix send final stats to event handlers before hanging up

[zodiak83]

If the stats_period in janus.conf is set large(about 10s), it will not be sent statistics summary when session is terminated

Before the janus_ice_outgoing_stats_handle is called, janus_ice_webrtc_hangup is called first.

Cannot send audio and video stats from janus_ice_outgoing_stats_handle because janus_ice_webrtc_hangup resets JANUS_ICE_HANDLE_WEBRTC_HAS_AUDIO and JANUS_ICE_HANDLE_WEBRTC_HAS_VIDEO flags

And the call to janus_events_notify_handlers seems to be wrong when combine_media_stats is true

[januscla]

Thanks for your contribution, @zodiak83! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

While I understand the issue you're encountering, I don't think the way you're addressing it is the right way to go: you're not clearing flags that should be cleared when a call ends.

At the time we did add a way for stats to be sent before the call was closed, in 238d585, which I guess is not triggering for you when the frequency of stats sent to handlers is low? In case, that's what we should fix, not the tweaks you're making. It may be we're calling janus_ice_outgoing_stats_handle too late, e.g., after we've clearer the PeerConnection resources instead of right before that happens. I'll have a look at that, so that you can test if it works for you.

[lminiero]

Looking at the code, I think you can just get rid of the

 && janus_flags_is_set(&handle->webrtc_flags, JANUS_ICE_HANDLE_WEBRTC_HAS_AUDIO)

and

 && janus_flags_is_set(&handle->webrtc_flags, JANUS_ICE_HANDLE_WEBRTC_HAS_VIDEO)

checks in janus_ice_outgoing_stats_handle, since we already check if the audio and video RTCP contexts exist before we add any stat in the first place. This should solve both issues you tried to address in your original PR, namely:

1. clearing the audio and video flags before janus_ice_hangup_peerconnection is invoked, that would prevent the last stat from being triggered, and
2. the combined stats not being fired if the PeerConnection is audio only (which is an actual bug)

I can prepare a quick PR with those small changes for you to try shortly.

[lminiero]

@zodiak83 I prepared a different patch for your problem in #2874. I've verified that I can't replicate the problem anymore, but please let me know if that's true for you as well. This only modifies janus_ice_outgoing_stats_handle itself, so it doesn't touch the flags you modified in your code.

As a side note, I already verified the multistream branch is not impacted by this problem, as we already do the rigth things there.

[zodiak83]

I tested your #2874 patch.
My problem is gone.
Thanks for the quick patch

[lminiero]

Ack, merging then 👍