sipsess: refactor and simplify SDP negotiation state #1016
Conversation
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
da089a3 to
30c31d6
Compare
| @@ -74,3 +83,4 @@ void sipsess_close_all(struct sipsess_sock *sock); | |||
| struct sip_dialog *sipsess_dialog(const struct sipsess *sess); | |||
| void sipsess_abort(struct sipsess *sess); | |||
| bool sipsess_ack_pending(const struct sipsess *sess); | |||
| enum sdp_neg_state sipsess_sdp_neg_state(const struct sipsess *sess); | |||
There was a problem hiding this comment.
Does the state have to be exposed to the application?
There was a problem hiding this comment.
I believe so. E.g. the application can check if the state is SDP_NEG_PREVIEW_ANSWER (unreliable SDP answer was sent in 1XX response) and in this case it MUST send the exact same SDP in the 200 OK. Ideally, re would not allow non RFC conformant usage, but we're not there yet. That would take more refactoring that could be done in a subsequent step.
There was a problem hiding this comment.
The following BareSIP PR makes use of the state:
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
30c31d6 to
811668a
Compare
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
4 times, most recently
from
5e945aa to
daae405
Compare
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
daae405 to
755bdb7
Compare
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
755bdb7 to
4ff4bab
Compare
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
4ff4bab to
a55bdba
Compare
|
Looks good now. |
| SDP_NEG_REMOTE_OFFER = (1 << 1), /** SDP offer received */ | ||
| SDP_NEG_PREVIEW_ANSWER = (1 << 2), /** SDP preview answer sent */ | ||
| SDP_NEG_DONE = (1 << 3) /** SDP negotiation done */ | ||
| }; |
There was a problem hiding this comment.
is this a "state" or "flags" ?
If it is flags then the stored value can contain a bitmask of many values ORed together.
If it is a state, the value can only contain one value and the states values 1<<N is not needed.
There was a problem hiding this comment.
Initially I thought it could be useful in some cases to OR the values (like flags), but I didn't end up using it anywhere. I have removed the explicit enum values.
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
2 times, most recently
from
5dff203 to
f03d626
Compare
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
f03d626 to
8c7d2e6
Compare
The SDP negotiation state is now tracked in a single enum which is used as a state machine to keep track of the current state of the SDP negotiation.
maximilianfridrich
force-pushed
the
sdp_neg_refactoring
branch
from
8c7d2e6 to
776c8c9
Compare
The SDP negotiation state is now tracked in a single enum which is used as a state machine to keep track of the current state of the SDP negotiation. This makes reasoning about the SDP negotiation much easier and less error prone.
Some details about SDP offer/answer are now enforced more strictly, e.g. the first reliable non-failure response to an INVITE MUST contain an SDP offer, if the INVITE contained no offer.
Read about SDP negotiation in the excellent informational RFC 6337.