pjnath: fix ICE failure when one media stream is rejected (port=0)

[pknellore]

When a SDP answer rejects a media section (e.g. audio codec mismatch
sets m=audio port=0), the corresponding ICE components have no
candidates in the checklist. The existing code looped over all
comp_cnt components unconditionally, causing two bugs:

1. ICE always fails: the nominated-pair scan found comp[0].nominated_check == NULL for the rejected stream and broke early, preventing the active stream (e.g. video) from ever completing ICE.

2. Crash ~5s after ICE success: ice_keep_alive() hit pj_assert(comp->nominated_check != NULL) for the disabled component.

Fix: introduce comp_has_candidates() which returns PJ_TRUE if the
checklist contains at least one entry for a given comp_id. Since checks
are only created when both a local and remote candidate exist, this
reliably identifies components belonging to rejected m= sections.

Apply the guard at every ICE completion decision point:

• check_ice_complete(): skip disabled comps in all four scanning loops (early-success check, controlled valid-pair scan, controlling valid-pair scan, controlling mid-check scan)
• start_nominated_check(): skip disabled comps, assert valid_check==NULL
• start_periodic_check(): skip disabled comps during nomination, assert valid_check==NULL
• ice_keep_alive(): advance comp_ka past disabled comps before sending the Binding Indication; if all comps are disabled, jump to schedule_next to rearm the timer without crashing

[Copilot]

Pull request overview

Fixes ICE negotiation failure and a delayed keep-alive crash when one “component” has no checklist entries (e.g., due to an SDP answer rejecting a media section with m= port 0), by skipping such components in ICE completion and nomination/keep-alive logic.

Changes:

• Adds comp_has_candidates() to detect components with at least one checklist entry.
• Updates check_ice_complete(), nomination paths, and ice_keep_alive() to skip components considered “disabled” (no checklist entries).
• Prevents ice_keep_alive() from asserting/crashing when cycling over a disabled component by advancing comp_ka to an active component.

Comments suppressed due to low confidence (1)

pjnath/src/pjnath/ice_session.c:1592

• In ice_keep_alive(), the timer interval is still divided by ice->comp_cnt even when some components are skipped as disabled. If only 1 of N components is active, the active component will receive keep-alives ~N times more frequently than before (since each tick still schedules at 1/N of the base interval but always sends on the same active comp). Consider basing the keep-alive interval on the number of active components (or keeping the original per-component cadence) to avoid unnecessary traffic.

schedule_next:
    if (ice->timer.id == TIMER_NONE) {
        pj_time_val delay = { 0, 0 };

        delay.msec = (PJ_ICE_SESS_KEEP_ALIVE_MIN +
                      (pj_rand() % PJ_ICE_SESS_KEEP_ALIVE_MAX_RAND)) * 1000 /
                     ice->comp_cnt;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This change alters ICE completion/keep-alive behavior for sessions where some component IDs have no candidate pairs. There’s an existing pjnath ICE test suite, but it doesn’t appear to cover “hole” cases (e.g., comp_cnt=2 with only comp_id=2 candidates, or port=0-rejected section). Adding a regression test for this scenario would help prevent reintroducing the early-fail and keep-alive assert/crash paths described in the PR.

[Copilot]

In the “all components have nominated pair” scan, skipping components based on comp_has_candidates() can make ICE complete successfully when the checklist is empty (e.g., trickle ICE before any pairs exist, or when the remote never provides candidates and end-of-candidates fires). With clist.count==0, every component is treated as “disabled” and the loop falls through to on_ice_complete(PJ_SUCCESS), which looks like an incorrect success state. Consider only treating a component as disabled once trickling has ended and the remote has definitively provided zero candidates for that comp_id, and ensure there is at least one active component before declaring ICE success.

[Copilot]

comp_has_candidates() logs at LOG4 every time it returns false. Since it’s called inside multiple loops (completion checks, nomination, keep-alive), this can generate very noisy logs for each disabled component. Consider downgrading the log level and/or logging only once per comp_id (e.g., via a per-component flag) to avoid repeated spam under normal operation.

Suggested change

	unsigned i;
	for (i = 0; i < ice->clist.count; ++i) {
	if (ice->clist.checks[i].lcand->comp_id == comp_id) {
	return PJ_TRUE;
	}
	}
	LOG4((ice->obj_name,
	"comp_id=%u has NO candidates - disabled m= section, will skip",
	comp_id));
	static pj_bool_t logged_no_cand[PJ_ICE_MAX_COMP];
	unsigned i;
	for (i = 0; i < ice->clist.count; ++i) {
	if (ice->clist.checks[i].lcand->comp_id == comp_id) {
	return PJ_TRUE;
	}
	}
	/* Log at most once per comp_id to avoid log spam from repeated checks. */
	if (comp_id == 0 || comp_id > PJ_ICE_MAX_COMP) {
	LOG4((ice->obj_name,
	"comp_id=%u has NO candidates - disabled m= section, will skip",
	comp_id));
	} else {
	unsigned idx = comp_id - 1;
	if (!logged_no_cand[idx]) {
	LOG4((ice->obj_name,
	"comp_id=%u has NO candidates - disabled m= section, will skip",
	comp_id));
	logged_no_cand[idx] = PJ_TRUE;
	}
	}