< Previous by Date Date Index Next by Date >
  Thread Index  

[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;