Bug 28687

per content:

 u: rtcp min interval
 b: does avpf

each codec feedback property is an struct:

(
 u: codec ID
 s: ack/nack/ccm/etc.
 s: type of feedback
 a{ss}: params
)

Okay cool check out my branch.

The parameter of  SetLocalFeedbackCodecs() should really be called "Codecs" since it does not actually describe codecs.. I'd call it something like "FeedbackMessages".

Also, what is called "category" is actually called type in the RFC (sorry, my error).. And the currently type should probably be subtype (and be optional).

I've done some more changes, see http://git.collabora.co.uk/?p=user/tester/telepathy-spec.git;a=shortlog;h=refs/heads/rtcp-fb

1. Renamed FeedbackCodec to Feedback Messages
2. Removed the extra params map, it seems that in the RFC all we can know is that its a string, so let's leave it as a string until the component that can understand it, so we avoid having to have parsers for each in the CM.
3. RTCPMinimumReportingInterval is per-codec it turns out, so move it there.

Replace CodecOffer with MediaDescription

Remove method (it all goes in Accept)

Drafted in

http://git.collabora.co.uk/?p=user/tester/telepathy-spec.git;a=shortlog;h=refs/heads/rtcp-fb-mediadesc

The only thing I'm not sure about here is if we should keep the word "Remote" in the property name since we use the same name in Accept().

I decided to review the full interface, as defined at http://git.collabora.co.uk/?p=user/tester/telepathy-spec.git;a=blob;f=spec/Call_Content_Media_Description_Interface_RTCP_Feedback.xml;h=94d2fc725b7600723e288a9a6813cee9b7e40470;hb=c7fa8b8ab095c816749d1b89e4163f66df85487d
A few points:

Why is MAXUINT (0xFFFFFFFF) used to mean "I don't care" rather than 0?

I agree about the property name:
s/RemoteFeedbackMessages/SupportedFeedbackMessages/?

(In reply to comment #7)
> Why is MAXUINT (0xFFFFFFFF) used to mean "I don't care" rather than 0?

0 is a valid value (ie, the timing is then such that RTCP packets dont exceed 5% of the available bandwidth). So if we want a "DEFUALT" we have to use something else, like MAXUINT. The alternative is to give up on the default.. But that would force every CM to know that in the standard AVP profile, the minimum is defined as 5 seconds, which I would find a bit sad.

> I agree about the property name:
> s/RemoteFeedbackMessages/SupportedFeedbackMessages/?

That or just "FeedbackMessages"

I just ran gitg and set it to look at everyone's branches.

ocrete/streamhandler-exthdr-rtcpfb looks good, given the above comments.

I fixed the things that we discussed, and pushed two patches to:

http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/rtcp-fb-mediadesc-ftfy

Actually, MAXUINT means "according to the bandwidth" for AVPF and 5 seconds for AVP.

We should have the bandwidth properties at some point.

Is http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=commitdiff;h=17b5f2148644caf5b923ebfb1b3531e776f95761 any better? (note that I won't close this bug until I've worked out what to do with the bandwidth property)

Negotiating bandwidth for the session should be on its own MD interface. Sjoerd thinks that we should just have one bandwidth int that applies to the content, and ignore/fudge per-call or per-stream limits or explicit RTCP bandwidth limits. After thinking it over, I think that's actually quite a workable solution. Will spec it up and see what it looks like.

The SIP bandwidth limits are per-content

that patch is wrong.

In RTP/AVP, MAXUINT means 5 seconds (even if there is spare bandwidth), in RTP/AVPF, it means 0 seconds (send as much as the bandwidth allows).

I agree that the bandwidth is probably a separate interface to support RFC 3556 (as well as the basic b= lines from RFC 4566).

(In reply to comment #14)
> that patch is wrong.
> 
> In RTP/AVP, MAXUINT means 5 seconds (even if there is spare bandwidth), in
> RTP/AVPF, it means 0 seconds (send as much as the bandwidth allows).
> 
> I agree that the bandwidth is probably a separate interface to support RFC 3556
> (as well as the basic b= lines from RFC 4566).

*sighs* Let's try again. If http://git.collabora.co.uk/?p=user/alsuren/telepathy-spec.git;a=shortlog;h=refs/heads/rtcp-fb-mediadesc-ftfy is still wrong, I might cry.

Don't cry!

+          The minimum interval between two regular RTCP packets for this
+          content, in milliseconds. If no special value is desired (for

+          signalling) one should put MAXUINT (0xFFFFFFFF). In RTP/AVP, the
+          setting this property to 0 means "Do not let RTCP packets exceed 5%
+          of the available bandwidth" and MAXUINT means "use the recommended
+          interval time of 5 seconds as specified by RFC 3550 section 6.2".
+          In RTP/AVPF, both 0 and MAXUINT are used to avoid specifying an
+          additional limit on the rate of RCTP packets (other than that
+          specified by bandwidth constraints for the session). Other profiles
+          may assign different meanings to the value 0 or MAXUINT.

"The special value MAXUINT means according to profile, in RTP/AVP, it means 5 seconds, in RTP/AVPF it means 0 seconds. In both cases, the interval between RTCP should be large enough to respect the RTCP bandwidth."

That said, maybe we want to get rid of MAXUINT and just force CM to know about profiles ?

> "The special value MAXUINT means according to profile, in RTP/AVP, it means 5
> seconds, in RTP/AVPF it means 0 seconds. In both cases, the interval between
> RTCP should be large enough to respect the RTCP bandwidth."
> 
That's not even valid english. I'm not merging that.

> That said, maybe we want to get rid of MAXUINT and just force CM to know about
> profiles ?

It seems like a huge cop-out, just because we can't agree on the language. I am quite tempted to agree though.

Updated proposal:

http://cgit.collabora.com/git/user/tester/telepathy-spec.git/log/?h=rtcp-fb-mediadesc-ftfy

When merging, I noticed the following:

    Note that StreamedMedia gained RTCP feedback capabilities
    while we were bikeshedding about RTCPMinimumInterval,
    so I have deferred to these types and copied the docstring of
    RTCPMinimumInterval across.
    
    RTCP_Feedback_Message_Map is now a{u(ua(sss))} rather than
    the invalid type a{uua(sss)} (we should probably make the
    spec parser check for invalid types really)

The interesting part of the diff is:

http://cgit.collabora.com/git/user/alsuren/telepathy-spec.git/diff/spec/Media_Stream_Handler.xml?h=rtcp-fb-mediadesc-28687

cgit isn't capable of showing the following changes (copy-pasted from gitk because gitg segfaults on merge commits):

 -    <tp:struct name="RTCP_Feedback_Message_Properties">
...
 -    </tp:struct>
 -
 -    <tp:struct name="RTCP_Feedback_Message"
...
 -    </tp:struct>
 -
 -    <tp:mapping name="RTCP_Feedback_Message_Map">
...
 -    </tp:mapping>
 -
 -    <property name="FeedbackMessages" type="a{uua(sss)}"
++    <property name="FeedbackMessages" type="a{u(ua(sss))}"

++ merge !

merged to alsuren/call

merged to master

merged to master