svn commit: r7024 - (AVPF PLI support)

Emil Ivov emcho at sip-communicator.org
Thu Apr 22 10:02:28 CEST 2010


Hey Seb,

This is indeed a great feature and the tests that I've conducted so far
are quite positive! I did have a few notes regarding your changes though:

На 21.04.10 18:11, s_vincent at dev.java.net написа:
> Modified: trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java?view=diff&rev=7024&p1=trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java&p2=trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java&r1=7023&r2=7024
> ==============================================================================
> --- trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java	(original)
> +++ trunk/src/net/java/sip/communicator/impl/neomedia/MediaUtils.java	2010-04-21 16:11:49+0000
> @@ -220,6 +228,7 @@
>              MediaType mediaType,
>              String jmfEncoding,
>              Map<String, String> formatParameters,
> +            Map<String, String> advancedParameters,

Missing javadocs.

> Added: trunk/src/net/java/sip/communicator/impl/neomedia/RTCPConnectorInputStream.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/neomedia/RTCPConnectorInputStream.java?view=auto&rev=7024
> ==============================================================================
> +    /**
> +     * Add an <tt>RTCPFeedbackListener</tt>.
> +     */
> +    public void addRTCPFeedbackListener(RTCPFeedbackListener listener)

missing @param

> +    /**
> +     * Removve an <tt>RTCPFeedbackListener</tt>.
> +     */
> +    public void removeRTCPFeedbackListener(RTCPFeedbackListener listener)
> +    {

missing @param

> Added: trunk/src/net/java/sip/communicator/impl/neomedia/RTCPFeedbackPacket.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/neomedia/RTCPFeedbackPacket.java?view=auto&rev=7024
> ==============================================================================
> --- (empty file)
> +++ trunk/src/net/java/sip/communicator/impl/neomedia/RTCPFeedbackPacket.java	2010-04-21 16:11:49+0000
> +import javax.media.rtp.*;
> +
> +public class RTCPFeedbackPacket

class javadocs? :)

I won't be reporting further javadoc conflicts so could please enable
error notifications for missing javadocs in your IDE and double check
all classes that your last commit modifies?

> +    /**
> +     * Write packet to output stream.
> +     *
> +     * @param out <tt>OutputDataStream</tt>

well that's a bit short. a few words explaining what the stream is about
would be nice.

> Modified: trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java?view=diff&rev=7024&p1=trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java&p2=trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java&r1=7023&r2=7024
> ==============================================================================
> --- trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java	(original)
> +++ trunk/src/net/java/sip/communicator/impl/protocol/sip/CallPeerMediaHandler.java	2010-04-21 16:11:49+0000
> @@ -868,10 +868,17 @@
>              //this is a reinit
>          }
>  
> -        /* set negociated output size for video stream */
> +
>          if(device != null && device.getMediaType() == MediaType.VIDEO)
>          {
> +            /* set negociated output size for video stream */
>              ((VideoMediaStream)stream).setOutputSize(size);
> +
> +            /* set rtcp-fb */
> +            if(format.getAdvancedParameters().containsKey("rtcp-fb"))
> +            {
> +                ((VideoMediaStream)stream).setRtcpFeedbackPLI(true);
> +            }
>          }

Both the size and the advanced parameters concern the stream
implementation and should be transparent to the signalling. In other
words CallPeerMediaHandler's job in this case is to set the format to
the streams. So in this case the VideoMediaStream implementation should
be the one detecting presence of size and rtcp-fb parameters.


> Modified: trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java?view=diff&rev=7024&p1=trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java&p2=trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java&r1=7023&r2=7024
> ==============================================================================
> --- trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java	(original)
> +++ trunk/src/net/java/sip/communicator/impl/protocol/sip/sdp/SdpUtils.java	2010-04-21 16:11:49+0000
> @@ -304,25 +304,21 @@

I noticed a few javadoc conflicts in here.

> @@ -484,6 +481,20 @@
>              return mediaFmts;
>          }
>  
> +        try
> +        {
> +            Attribute rtcpFbAsterisk = findPayloadTypeSpecificAttribute(
> +                    mediaDesc.getAttributes(false), "rtcp-fb", "*");

The point of defining the "advancedParemeters" tables in media formats
was to avoid exactly this type of parameter specific handling. SDPUtils
should not be aware of the specifics of the "rtcp-fb" param. It should
simply add to the advanced params map all format specific attributes
that are neither rtpmap or fmtp.

At least that was the idea. Is there anything here that prevented you
from implementing things that way?

> +
> +            if(rtcpFbAsterisk != null)
> +                advp.add(rtcpFbAsterisk);

same comment as above.

> +                rtcpFb = findPayloadTypeSpecificAttribute(
> +                        mediaDesc.getAttributes(false), "rtcp-fb", ptStr);
> +
> +                if(rtcpFb != null)
> +                    advp.add(rtcpFb);

Same as above - rtcp-fb specific handling.

> +            /* RTCP feedback support */
> +            if (attr.getName().equals("rtcp-fb"))
> +            {
> +                advancedParamsMap = parseRTCPFeedbackAttribute(attr);
> +            }
> +        }

same as above.

>      /**
> +     * Parses the value of <tt>rtcpAttr</tt> attribute.
> +     *
> +     * @param rtcpAttr SDP attribute containing rtcp-fb parameters.
> +     * @return
> +     */
> +    private static Map<String, String> parseRTCPFeedbackAttribute(
> +            Attribute rtcpAttr)
> +    {

same as above. this whole method should be handling generic format
specific attributes.

(there are other occurrences of rtcp-fb specific code but I guess you
see what I mean).


> Modified: trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java
> Url: https://sip-communicator.dev.java.net/source/browse/sip-communicator/trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java?view=diff&rev=7024&p1=trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java&p2=trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java&r1=7023&r2=7024
> ==============================================================================
> --- trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java	(original)
> +++ trunk/src/net/java/sip/communicator/plugin/aimaccregwizz/FirstWizardPage.java	2010-04-21 16:11:49+0000
> @@ -313,7 +313,7 @@
>  
>      /**
>       * The simple form for this wizard.
> -     * @return
> +     * @return the simple form for this wizard.
>       */
>      public Object getSimpleForm()
>      {
> @@ -322,7 +322,8 @@
>  
>      /**
>       * Whether is committed.
> -     * @return
> +     * @return <tt>true</tt> if the form is committed, <tt>false</tt>
> +     * otherwise
>       */

Aha! Duely noted! Thanks for taking care of these ;)


> +
> +    /**
> +     * Use or not RTCP feedback Picture Loss Indication.
> +     *
> +     * @param use use or not PLI
> +     */
> +    public void setRtcpFeedbackPLI(boolean use);

change to set/addAdvancedAttributes();

Cheers,
Emil


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe at sip-communicator.dev.java.net
For additional commands, e-mail: commits-help at sip-communicator.dev.java.net





More information about the commits mailing list