[jitsi-dev] Handling of Conference Information documents


#1

Hi,

I've been working on improving handling of Conference Information XML documents (RFC4575) in Jitsi. The main goals are to:
1. Not send unnecessary documents (currently we sometimes send multiple documents that only differ in their version)
2. Support 'partial notifications'
3. Prevent bursts of documents being sent to a single peer in a very short period of time (e.g. when a peer's status goes from (none) -> dialing-out -> pending -> alerting)

Point 2 has two parts: receiving and sending. Currently the code doesn't handle either. Therefore, the plan is to implement both, but only enable sending later on.

I've committed my changes the 'coin-refactor' branch (although they aren't just refactoring. I couldn't think of a more appropriate name...). It's a work in progress and it hasn't been well tested yet. Still, I'd really appreciate any comments.

This is, in short, what I've done so far:

Added a ConferenceInfoDocument class which wraps around a DOM Document. This is in order to merge the implementations for SIP and XMPP, which currently generate conference info documents in their own way. The class currently resides in net.java.sip.communicator.util, which is probably not a good place for it.

Added fields to MediaAwareCallPeer which keep the last conference info document sent to the peer, and the time at which it was sent. This is needed in order to detect whether a document we intend to send has actual changes, and also if we want to send partial notifications.

Added a field to MediaAwareCallPeer which keeps the last conference info document sent to us by the peer. This is in order to support receiving partial notifications.

Added a getCurrentConferenceInfo(CallPeer callPeer) method to AbstractOperationSetTelephonyConferencing which generates a "full" conference information document for callPeer.

Added a getConferenceInfoDiff(from, to) method to AbstractOperationSetTelephonyConferencing. Semantically, it takes two "full" (with "state=full", as opposed to partial) documents. It returns a document, such that when it is applied to "from" using the procedure defined in section 4.6 of RFC4575, the result is "to". The method may return null if "from" and "to" are determined to be equal.
http://tools.ietf.org/html/rfc4575#section-4.6

The current implementation just returns "to", which is the laziest possible thing to do :). The plan is to improve it to detect equal documents first, achieving goal 1 above, and later actually generate partial notifications.

Updated the XMPP and SIP code to use these methods. Until now the document version to use was maintained in the OpSetTelephonyConferencing implementations and shared between CallPeer's. Now, the last sent document to a peer is used to determine the version of the next document, so the 'version' fields in OpSetTelephonyConferencing implementations have been dropped.

Added a "buffer" to the XMPP implementation, which prevents more than one COIN to be sent to a given call peer in 200ms (subsequent requests for notification trigger a single notification to be scheduled for 200ms after the last sent COIN). This part is supposed to be protocol-independent, and will be made so later.

Refactored handling of received conference info documents by: adding setConferenceInfoDocument(CallPeer, ConferenceInfoDocument) and
updateConferenceInfoDocument(CallPeer, ConferenceInfoDocument) methods to AbstractOperationSetTelephonyConferencing. The first takes a "full" document and updates the CallPeer state with it. The second, which isn't implemented yet, is supposed to do the same with a partial document. The plan is to implement it by constructing a "full" document first and then reusing setConferenceInfoDocument.

Regards,
Boris


#2

Hey,

I'm planning to merge this to master soon, unless there are objections.

Anyone (Ingo?:)) have any advice/preference as to whether I should rebase it onto the then-current master or merge it at it is?

Regards,
Boris


#3

I'm planning to merge this to master soon, unless there are objections.

Anyone (Ingo?:)) have any advice/preference as to whether I should
rebase it onto the then-current master or merge it at it is?

I tried to review these series, but I don't know Coin or the code and there
were just too many patches. But I noticed some "smells" when it comes to
exception handling:

+ try
+ {
+ document = XMLUtils.createDocument(xml);
+ }
+ catch (Exception e)
+ {
+ logger.error("Failed to create a new document.", e);
+ }
+ //XXX this is not tested yet. do we need to set a namespace?
+ try
+ {
+ conferenceInfo = document.getElementById("conference-info");

If createDocument excepts, you're later guaranteed to have an NPE. Or

+ public int getVersion()
+ {
+ String versionString =
conferenceInfo.getAttribute(VERSION_ATTR_NAME);
+ int version = -1;
+ try
+ {
+ version = Integer.parseInt(versionString);
+ }
+ catch (Exception e){}

···

+
+ return version;
+ }

I don't want so say this is a blocker or that you should fix it before
merging, but generally speaking about exception handling:
- (almost) never do catch-all
- (almost) never do nothing (means not even logging) in a catch block
- Only handle exceptions that a particular piece of code can deal with. So
for example, when the XMLUtils cannot parse a certain (assumed-to-be)
xml-string, then there's nothing you can do about it. Let the exception
bubble up to a location where it can be gracefully handled: This could be an
information to the user, ending the call, logging and then ignoring,
retrying a request, etc.

As for git: I'm fine with either way (rebasing and then merge -no-ff OR
merging the branch)

Regards,
Boris

Ingo


#4

I'm planning to merge this to master soon, unless there are objections.

Anyone (Ingo?:)) have any advice/preference as to whether I should
rebase it onto the then-current master or merge it at it is?

I tried to review these series

Thank you!

, but I don't know Coin or the code and there

were just too many patches. But I noticed some "smells" when it comes to
exception handling:

+ try
+ {
+ document = XMLUtils.createDocument(xml);
+ }
+ catch (Exception e)
+ {
+ logger.error("Failed to create a new document.", e);
+ }
+ //XXX this is not tested yet. do we need to set a namespace?
+ try
+ {
+ conferenceInfo = document.getElementById("conference-info");

If createDocument excepts, you're later guaranteed to have an NPE. Or

+ public int getVersion()
+ {
+ String versionString =
conferenceInfo.getAttribute(VERSION_ATTR_NAME);
+ int version = -1;
+ try
+ {
+ version = Integer.parseInt(versionString);
+ }
+ catch (Exception e){}
+
+ return version;
+ }

Agreed, these are bad. I will go over all the exception handling.

I don't want so say this is a blocker or that you should fix it before
merging, but generally speaking about exception handling:
- (almost) never do catch-all
- (almost) never do nothing (means not even logging) in a catch block
- Only handle exceptions that a particular piece of code can deal with. So
for example, when the XMLUtils cannot parse a certain (assumed-to-be)
xml-string, then there's nothing you can do about it. Let the exception
bubble up to a location where it can be gracefully handled: This could be an
information to the user, ending the call, logging and then ignoring,
retrying a request, etc.

As for git: I'm fine with either way (rebasing and then merge -no-ff OR
merging the branch)

Alright.

Thanks,
Boris

···

On 6/26/13 11:47 AM, Ingo Bauersachs wrote: