[reSIProcate] SdpContents rewrite for supporting simcap (RFC3407)
Hi!
We have recently changed from the Vocal SIP stack to resiprocate, and we
are satisfied with it so far altough we only use it for parsing and
encoding messages for now.
The most important reasons for the switch were being able to support new
features. One of the ones we are going to implement is the simple
capability support as specified in RFC3407 (and probably support for the
older versions of the capabilities that used the X- prefix before the
parameters).
Unfortunately I have found that it cannot be done with the current SDP
implementation of resiprocate. The simcap specification relies on the
order of the attributes, but resiprocate stores all parsed attributes in a
hash map which does not keep the order.
I was thinking about adding a list beside the map to keep the order, but
after some consideration I have decided that it would not work, because if
attributes with the same key do not appear one after the other, then the
map could not store them.
So I have decided to drop the map completely and only use a list of
key,value pairs. This unfortunately means an interface change, but I
suppose that the attribute map was mostly used for parsing the rtpmap and
fmtp attributes and of course I have rewritten that part also, so most
users of the stack won't need to change anything.
Note that the addAttributes method still works almost the same as before.
I'm attaching the svn diff of the change. I would like it if could get into
svn repository. If you have some suggestions of improvements, then it
would be good.
Also I have some questions about the coding style I have encountered:
- there are usually almost no comments, not even for the interface - Is
that intentional, and if so, then where should/is the interface described.
- for some reason inline functions are not used almost nowhere, there are
many cases where simple one liners are put in the .cxx file - Is there a
reason for that? I would think that putting those in the header would
allow more optimizations for the compiler without much negative side
effects.
I tried to follow this convention, but I'm not sure if it was a good idea.
Regards,
Zsolt
Index: resip/stack/SdpContents.cxx
===================================================================
--- resip/stack/SdpContents.cxx (revision 5817)
+++ resip/stack/SdpContents.cxx (working copy)
@@ -75,40 +75,25 @@
return *this;
}
-bool
-AttributeHelper::exists(const Data& key) const
+const list<std::pair<Data, Data> >&
+AttributeHelper::getValues() const
{
- return mAttributes.find(key) != mAttributes.end();
+ return mAttributes;
}
-const list<Data>&
-AttributeHelper::getValues(const Data& key) const
-{
- if (!exists(key))
- {
- static const list<Data> emptyList;
- return emptyList;
- }
- return mAttributes.find(key)->second;
-}
-
ostream&
AttributeHelper::encode(ostream& s) const
{
- for (HashMap< Data, list<Data> >::const_iterator i = mAttributes.begin();
+ for (std::list<std::pair<Data, Data> >::const_iterator i = mAttributes.begin();
i != mAttributes.end(); ++i)
{
- for (list<Data>::const_iterator j = i->second.begin();
- j != i->second.end(); ++j)
+ s << "a="
+ << i->first;
+ if (!i->second.empty())
{
- s << "a="
- << i->first;
- if (!j->empty())
- {
- s << Symbols::COLON[0] << *j;
- }
- s << Symbols::CRLF;
+ s << Symbols::COLON[0] << i->second;
}
+ s << Symbols::CRLF;
}
return s;
}
@@ -132,22 +117,33 @@
pb.data(value, anchor);
}
- if(!pb.eof()) skipEol(pb);
+ if(!pb.eof()) skipEol(pb);
- mAttributes[key].push_back(value);
+ mAttributes.push_back(std::make_pair(key, value));
}
}
void
AttributeHelper::addAttribute(const Data& key, const Data& value)
{
- mAttributes[key].push_back(value);
+ mAttributes.push_back(std::make_pair(key, value));
}
void
AttributeHelper::clearAttribute(const Data& key)
{
- mAttributes.erase(key);
+ for (std::list<std::pair<Data, Data> >::iterator i = mAttributes.begin();
+ i != mAttributes.end(); )
+ {
+ if (i->first == key)
+ {
+ std::list<std::pair<Data, Data> >::iterator toDel = i;
+ i++;
+ mAttributes.erase(toDel);
+ }
+ else
+ i++;
+ }
}
SdpContents::SdpContents() : Contents(getStaticType())
@@ -1157,44 +1153,20 @@
SdpContents::Session::addAttribute(const Data& key, const Data& value)
{
mAttributeHelper.addAttribute(key, value);
-
- if (key == rtpmap)
- {
- for (list<Medium>::iterator i = mMedia.begin();
- i != mMedia.end(); ++i)
- {
- i->mRtpMapDone = false;
- }
- }
}
void
SdpContents::Session::clearAttribute(const Data& key)
{
mAttributeHelper.clearAttribute(key);
-
- if (key == rtpmap)
- {
- for (list<Medium>::iterator i = mMedia.begin();
- i != mMedia.end(); ++i)
- {
- i->mRtpMapDone = false;
- }
- }
}
-bool
-SdpContents::Session::exists(const Data& key) const
+const list<std::pair<Data, Data> >&
+SdpContents::Session::getValues() const
{
- return mAttributeHelper.exists(key);
+ return mAttributeHelper.getValues();
}
-const list<Data>&
-SdpContents::Session::getValues(const Data& key) const
-{
- return mAttributeHelper.getValues(key);
-}
-
SdpContents::Session::Medium::Medium(const Data& name,
unsigned long port,
unsigned long multicast,
@@ -1490,6 +1462,16 @@
}
}
+void
+SdpContents::Session::Medium::clearAttribute(const Data& key)
+{
+ mAttributeHelper.clearAttribute(key);
+ if (key == rtpmap)
+ {
+ mRtpMapDone = false;
+ }
+}
+
const list<SdpContents::Session::Connection>
SdpContents::Session::Medium::getConnections() const
{
@@ -1502,43 +1484,13 @@
return connections;
}
-bool
-SdpContents::Session::Medium::exists(const Data& key) const
+const list<std::pair<Data, Data> >&
+SdpContents::Session::Medium::getValues() const
{
- if (mAttributeHelper.exists(key))
- {
- return true;
- }
- return mSession && mSession->exists(key);
+ return mAttributeHelper.getValues();
}
-const list<Data>&
-SdpContents::Session::Medium::getValues(const Data& key) const
-{
- if (exists(key))
- {
- return mAttributeHelper.getValues(key);
- }
- if (!mSession)
- {
- assert(false);
- static list<Data> error;
- return error;
- }
- return mSession->getValues(key);
-}
-
void
-SdpContents::Session::Medium::clearAttribute(const Data& key)
-{
- mAttributeHelper.clearAttribute(key);
- if (key == rtpmap)
- {
- mRtpMapDone = false;
- }
-}
-
-void
SdpContents::Session::Medium::clearCodecs()
{
mFormats.clear();
@@ -1572,27 +1524,44 @@
// prevent recursion
mRtpMapDone = true;
- if (exists(rtpmap))
+ for (list<std::pair<Data, Data> >::const_iterator i = getValues().begin();
+ i != getValues().end(); ++i)
{
- for (list<Data>::const_iterator i = getValues(rtpmap).begin();
- i != getValues(rtpmap).end(); ++i)
+ if (i->first != rtpmap)
+ continue;
+
+ //DebugLog(<< "SdpContents::Session::Medium::getCodec(" << *i << ")");
+ ParseBuffer pb(i->second.data(), i->second.size());
+ int format = pb.integer();
+ // pass to codec constructor for parsing
+ try
{
- //DebugLog(<< "SdpContents::Session::Medium::getCodec(" << *i << ")");
- ParseBuffer pb(i->data(), i->size());
- int format = pb.integer();
- // pass to codec constructor for parsing
- // pass this for other codec attributes
- try
- {
- mRtpMap[format].parse(pb, *this, format);
- }
- catch (ParseBuffer::Exception&)
- {
- mRtpMap.erase(format);
- }
+ mRtpMap[format].parse(pb, format);
}
+ catch (ParseBuffer::Exception&)
+ {
+ mRtpMap.erase(format);
+ }
}
+ for (list<std::pair<Data, Data> >::const_iterator i = getValues().begin();
+ i != getValues().end(); ++i)
+ {
+ if (i->first != rtpmap)
+ continue;
+
+ ParseBuffer pb(i->second.data(), i->second.size());
+ int payload = pb.integer();
+ RtpMap::iterator codec = mRtpMap.find(payload);
+ if (codec != mRtpMap.end())
+ {
+ const char* anchor = pb.skipWhitespace();
+ pb.skipToEnd();
+ pb.data(codec->second.parameters(), anchor);
+ break;
+ }
+ }
+
for (list<Data>::const_iterator i = mFormats.begin();
i != mFormats.end(); ++i)
{
@@ -1703,9 +1672,7 @@
}
void
-Codec::parse(ParseBuffer& pb,
- const SdpContents::Session::Medium& medium,
- int payloadType)
+Codec::parse(ParseBuffer& pb, int payloadType)
{
const char* anchor = pb.skipWhitespace();
pb.skipToChar(Symbols::SLASH[0]);
@@ -1713,24 +1680,6 @@
pb.skipChar(Symbols::SLASH[0]);
mRate = pb.integer();
mPayloadType = payloadType;
-
- // get parameters if they exist
- if (medium.exists(fmtp))
- {
- for (list<Data>::const_iterator i = medium.getValues(fmtp).begin();
- i != medium.getValues(fmtp).end(); ++i)
- {
- ParseBuffer pb(i->data(), i->size());
- int payload = pb.integer();
- if (payload == payloadType)
- {
- const char* anchor = pb.skipWhitespace();
- pb.skipToEnd();
- pb.data(mParameters, anchor);
- break;
- }
- }
- }
}
const Data&
Index: resip/stack/SdpContents.hxx
===================================================================
--- resip/stack/SdpContents.hxx (revision 5817)
+++ resip/stack/SdpContents.hxx (working copy)
@@ -24,14 +24,13 @@
AttributeHelper(const AttributeHelper& rhs);
AttributeHelper& operator=(const AttributeHelper& rhs);
- bool exists(const Data& key) const;
- const std::list<Data>& getValues(const Data& key) const;
+ const std::list<std::pair<Data, Data> >& getValues() const;
std::ostream& encode(std::ostream& s) const;
void parse(ParseBuffer& pb);
void addAttribute(const Data& key, const Data& value = Data::Empty);
void clearAttribute(const Data& key);
private:
- HashMap< Data, std::list<Data> > mAttributes;
+ std::list<std::pair<Data, Data> > mAttributes;
};
class SdpContents : public Contents
@@ -57,9 +56,7 @@
Codec(const Codec& rhs);
Codec& operator=(const Codec& codec);
- void parse(ParseBuffer& pb,
- const SdpContents::Session::Medium& medium,
- int payLoadType);
+ void parse(ParseBuffer& pb, int payLoadType);
const Data& getName() const;
int getRate() const;
@@ -387,8 +384,7 @@
const Encryption& getEncryption() const {return mEncryption;}
const Encryption& encryption() const {return mEncryption;}
Encryption& encryption() {return mEncryption;}
- bool exists(const Data& key) const;
- const std::list<Data>& getValues(const Data& key) const;
+ const std::list<std::pair<Data, Data> >& getValues() const;
void clearAttribute(const Data& key);
const Codec& findFirstMatchingCodecs(const std::list<Codec>& codecs) const;
@@ -471,8 +467,7 @@
void clearMedium() { mMedia.clear(); }
void clearAttribute(const Data& key);
void addAttribute(const Data& key, const Data& value = Data::Empty);
- bool exists(const Data& key) const;
- const std::list<Data>& getValues(const Data& key) const;
+ const std::list<std::pair<Data, Data> >& getValues() const;
private:
int mVersion;