RE: CR: Fix to drop initial RTP packets when packettime stamp < RTP offset time (Fix only for live streaming)

Ok - this looks good to me.

=============================================
Eric Hyche ([email protected])
Technical Lead
RealNetworks, Inc.  

> -----Original Message-----
> From: [email protected] 
> [mailto:[email protected]] 
> Sent: Friday, June 06, 2008 6:52 PM
> To: [email protected]; [email protected]; 
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: RE: [Helix-client-dev] CR: Fix to drop initial RTP 
> packets when packettime stamp < RTP offset time (Fix only for 
> live streaming)
> 
> Hi,
> 
> I am checking two places whether the packet timestamp < RTPOffset time
> and dropping the packet if the timestamp < RTP offset time. 
> Sometimes we
> receive RTP packets before receiving the RTPOffsettime time. In this
> case the first check is going to fail and the packet get added to
> StartInfoWaitQueue. The second check going to tak care of the 
> packets in
> the queue.
> 
> Added condition to check whether received RTP offset time in the play
> response. Ie, if (m_bIsLive && m_bRTPTimeSet). This should 
> take care of
> the specified failure case. All other case we don't drop the packets.
> 
> Thanks
> Saleem
> 
> Index: rtptran.cpp
> ===================================================================
> RCS file: /cvsroot/protocol/transport/rtp/rtptran.cpp,v
> retrieving revision 1.101.2.4.2.1
> diff -u -w -r1.101.2.4.2.1 rtptran.cpp
> --- rtptran.cpp	8 May 2008 13:41:10 -0000	1.101.2.4.2.1
> +++ rtptran.cpp	6 Jun 2008 22:29:38 -0000
> @@ -1560,6 +1560,13 @@
>      // stamps
>      if (m_bWaitForStartInfo)
>      {
> +        if (m_bIsLive && m_bRTPTimeSet)
> +        {
> +            if (CompareRTPTimestamp(pkt.timestamp, 
> m_lTimeOffsetRTP) <
> 0)
> +            {
> +               return HXR_OK;
> +            }
> +        }
>          if (m_StartInfoWaitQueue.GetCount() == 0)
>          {
>              // First packet received
> @@ -1661,19 +1668,29 @@
>  
>                  if (pStoredBuffer)
>                  {
> -                    if (m_bFirstSeqNumLocked)
> -                    {
> -                        // If sequence  number is locked, we drop any
> packet that precedes the locked in sequence number
>                          RTPPacket pkt;
>                          if (pkt.unpack(pBuffer->GetBuffer(),
> pBuffer->GetSize()) == 0)
>                          {
> +                        if (m_bFirstSeqNumLocked)
> +                        {
> +                            // If sequence  number is locked, we drop
> any packet that precedes the locked in sequence number
>                              LONG32 lSeqNumDelta = ((LONG32) 
> (((UINT16)
> pkt.seq_no) - m_uFirstSeqNum));
>                              if (lSeqNumDelta  < 0)
>                              {
>                                  HX_RELEASE(pStoredBuffer);
>                              }
>                          }
> +
> +                        if (pStoredBuffer && m_bIsLive &&
> m_bRTPTimeSet)
> +                        {
> +                            if (CompareRTPTimestamp(pkt.timestamp,
> m_lTimeOffsetRTP) < 0)
> +                            {
> +                                HX_RELEASE(pStoredBuffer);
> +                            }
> +
> +                        }
>                      }
> +
>                      if (pStoredBuffer)
>                      {
>                          _handlePacket(pStoredBuffer, FALSE);
> @@ -4654,3 +4671,22 @@
>  
>      return rc;
>  }
> +
> +INT16 RTPBaseTransport::CompareRTPTimestamp(ULONG32 x, ULONG32 y)
> +{
> +	ULONG32 min = y;
> +
> +	if ( (x <= y && y - x < ULONG32( 1 << 31 ) ) ||
> +	     (x > y && x - y > ULONG32( 1 << 31 ) ) )
> +	{
> +	    min = x;
> +	}
> +
> +    INT16 nResult = -1;
> +	if ( x != min )
> +	   nResult = 1;
> +	else if ( x == y )
> +	   nResult = 0;
> +
> +	return nResult;
> +}
> 
> 
> 
> -----Original Message-----
> From: ext Eric Hyche [mailto:[email protected]] 
> Sent: Thursday, June 05, 2008 9:33 AM
> To: Adookkattil Saleem (Nokia-D-MSW/Dallas);
> [email protected]; [email protected];
> [email protected]
> Cc: 'Gregory Wright'; 'Henry Ping'; Milko Boic
> Subject: RE: [Helix-client-dev] CR: Fix to drop initial RTP 
> packets when
> packettime stamp < RTP offset time (Fix only for live streaming)
> 
> 
> Saleem,
> 
> Here are my comments. Greg/Henry/Milko: I would appreciate it 
> if another
> set of eyeballs could take a look at this change, since it's in code
> that I'm not the most familiar with.
> 
> I'm not sure why this change is needed:
> 
> @@ -1661,19 +1668,29 @@ 
>   
>                  if (pStoredBuffer) 
>                  { 
> -                    if (m_bFirstSeqNumLocked) 
> -                    { 
> -                        // If sequence  number is locked, we drop any
> packet that precedes the locked in sequence number
> 
>                          RTPPacket pkt; 
>                          if (pkt.unpack(pBuffer->GetBuffer(),
> pBuffer->GetSize()) == 0) 
>                          { 
> +                        if (m_bFirstSeqNumLocked) 
> +                        { 
> +                            // If sequence  number is 
> locked, we drop 
> + any packet that precedes the locked in sequence number
> 
>                              LONG32 lSeqNumDelta = ((LONG32) 
> (((UINT16)
> pkt.seq_no) - m_uFirstSeqNum)); 
>                              if (lSeqNumDelta  < 0) 
>                              { 
>                                  HX_RELEASE(pStoredBuffer); 
>                              } 
>                          } 
> + 
> +                        if (pStoredBuffer && m_bIsLive ) 
> +                        { 
> +                            if (CompareRTPTimestamp(pkt.timestamp,
> m_lTimeOffsetRTP) < 0) 
> +                            { 
> +                                HX_RELEASE(pStoredBuffer); 
> +                            }
> + 
> +                        }
>                      } 
> + 
>                      if (pStoredBuffer) 
>                      { 
>                          _handlePacket(pStoredBuffer, FALSE); 
> 
> Since it is checking packets in the m_StartInfoWaitQueue queue.
> However, there is only one place that packets can be added to the
> m_StartInfoWaitQueue queue, and that's here:
> 
>         pBuffer->AddRef();
>         m_StartInfoWaitQueue.AddTail(pBuffer);
> 	m_ulWaitQueueBytes += pBuffer->GetSize();
> 
> But this code is below this change:
> 
> @@ -1560,6 +1560,13 @@ 
>      // stamps 
>      if (m_bWaitForStartInfo) 
>      { 
> +        if (m_bIsLive) 
> +        { 
> +            if (CompareRTPTimestamp(pkt.timestamp, 
> m_lTimeOffsetRTP) <
> 0) 
> +            { 
> +               return HXR_OK; 
> +            } 
> +        }
>          if (m_StartInfoWaitQueue.GetCount() == 0) 
>          { 
>              // First packet received 
> 
> which means that if the CompareRTPTimestamp check failed, then these
> packets could never get added to the StartInfoWaitQueue queue.
> 
> Also, what happens if the PLAY response doesn't have a play range? I
> guess in that case lTimeOffsetRTP will be 0 in which case the
> CompareRTPTimestamp check will never fail. If that correct?
> 
> Eric
> 
> =============================================
> Eric Hyche ([email protected])
> Technical Lead
> RealNetworks, Inc.  
> 
> > -----Original Message-----
> > From: [email protected]
> > [mailto:[email protected]] On Behalf Of 
> > [email protected]
> > Sent: Tuesday, June 03, 2008 3:49 PM
> > To: [email protected];
> > [email protected]; [email protected]
> > Subject: [Helix-client-dev] CR: Fix to drop initial RTP 
> packets when 
> > packettime stamp < RTP offset time (Fix only for live streaming)
> > 
> > 	"Nokia submits this code under the terms of a commercial
> contribution 
> > agreement
> > 	with RealNetworks, and I am authorized to contribute this code
> under 
> > said agreement."
> > 
> > 	Modified by: [email protected]
> > 	  
> > 	Reviewed by:  
> > 
> > 	Date: 02-June-2008
> > 	  
> > 	Project: SymbianMmf_Rel
> > 
> > 	TSW: NGRN-7DRASY
> > 
> > 	Synopsis: Fix to drop initial RTP packets when packet time stamp
> < 
> > RTP offset time (Fix only for live streaming)
> > 
> > 	Overview: Sometimes helix client receives few initial RTP
> packets 
> > with timestamp < RTP offset time. As these packets are 
> already delayed
> 
> > packets there is no need to store these packets to the 
> buffer. Added 
> > code to drop these packet after comparing the timestamp with RTP 
> > offset time.
> > 
> > 	  
> > 	Files Modified: 
> > 	  
> > 	/cvsroot/protocol/transport/rtp/rtptran.cpp
> > 
> > 	Image Size and Heap Use impact: None
> > 	  
> > 	Module Release testing (STIF) : Pass
> > 	  
> > 	Test case(s) Added  :  No
> > 
> > 	Memory leak check performed : No new leaks introduced.  
> > 
> > 	Platforms and Profiles Build Verified: 
> > 	Profile -> helix-client-s60-32-mmf-mdf-arm 
> > 	BIF branch  -> helix_restricted 
> > 	SYSTEM_ID -> symbian-91-armv5 
> > 	Target -> symbianMmf_rel
> > 	  
> > 	Platforms and Profiles Functionality verified: armv5, winscw
> > 	  
> > 
> >       Branch: 210CayS,  221CayS& head
> > 
> > Index: rtptran.cpp
> > ===================================================================
> > RCS file: /cvsroot/protocol/transport/rtp/rtptran.cpp,v
> > retrieving revision 1.101.2.4.2.1
> > diff -u -w -r1.101.2.4.2.1 rtptran.cpp 
> > --- rtptran.cpp 8 May 2008 13:41:10 -0000       1.101.2.4.2.1 
> > +++ rtptran.cpp 3 Jun 2008 19:16:26 -0000
> > @@ -1560,6 +1560,13 @@ 
> >      // stamps 
> >      if (m_bWaitForStartInfo) 
> >      {
> > +        if (m_bIsLive) 
> > +        { 
> > +            if (CompareRTPTimestamp(pkt.timestamp,
> > m_lTimeOffsetRTP) < 0)
> > +            { 
> > +               return HXR_OK; 
> > +            } 
> > +        }
> >          if (m_StartInfoWaitQueue.GetCount() == 0) 
> >          { 
> >              // First packet received @@ -1661,19 +1668,29 @@
> >   
> >                  if (pStoredBuffer) 
> >                  { 
> > -                    if (m_bFirstSeqNumLocked) 
> > -                    { 
> > -                        // If sequence  number is locked, we 
> > drop any packet that precedes the locked in sequence number
> > 
> >                          RTPPacket pkt; 
> >                          if (pkt.unpack(pBuffer->GetBuffer(),
> > pBuffer->GetSize()) == 0)
> >                          {
> > +                        if (m_bFirstSeqNumLocked) 
> > +                        { 
> > +                            // If sequence  number is
> > locked, we drop any packet that precedes the locked in 
> sequence number
> > 
> >                              LONG32 lSeqNumDelta = ((LONG32)
> > (((UINT16) pkt.seq_no) - m_uFirstSeqNum)); 
> >                              if (lSeqNumDelta  < 0) 
> >                              { 
> >                                  HX_RELEASE(pStoredBuffer); 
> >                              } 
> >                          }
> > + 
> > +                        if (pStoredBuffer && m_bIsLive ) 
> > +                        { 
> > +                            if
> > (CompareRTPTimestamp(pkt.timestamp, m_lTimeOffsetRTP) < 0)
> > +                            { 
> > +                                HX_RELEASE(pStoredBuffer); 
> > +                            }
> > + 
> > +                        }
> >                      }
> > + 
> >                      if (pStoredBuffer) 
> >                      { 
> >                          _handlePacket(pStoredBuffer, FALSE); @@ 
> > -4654,3 +4671,22 @@
> >   
> >      return rc;
> >  }
> > + 
> > +INT16 RTPBaseTransport::CompareRTPTimestamp(ULONG32 x, ULONG32 y) {
> > +       ULONG32 min = y;
> > + 
> > +       if ( (x <= y && y - x < ULONG32( 1 << 31 ) ) || 
> > +            (x > y && x - y > ULONG32( 1 << 31 ) ) ) 
> > +       { 
> > +           min = x; 
> > +       }
> > + 
> > +       INT16 nResult = -1; 
> > +       if ( x != min ) 
> > +          nResult = 1; 
> > +       else if ( x == y ) 
> > +          nResult = 0;
> > + 
> > +       return nResult;
> > +}
> > 
> > Index: rtptran.h
> > ===================================================================
> > RCS file: /cvsroot/protocol/transport/rtp/pub/rtptran.h,v
> > retrieving revision 1.51.2.1.2.1
> > diff -u -w -r1.51.2.1.2.1 rtptran.h 
> > --- rtptran.h   8 May 2008 13:41:11 -0000       1.51.2.1.2.1 
> > +++ rtptran.h   3 Jun 2008 19:19:12 -0000 
> > @@ -355,6 +355,7 @@
> >   
> >  private: 
> >      HX_RESULT _handlePacket(IHXBuffer* pBuffer, HXBOOL 
> bIsRealTime);
> > +    INT16 CompareRTPTimestamp(ULONG32 x, ULONG32 y);
> >  };
> >   
> >  /*
> > 
> > 
> 


_______________________________________________
Helix-client-dev mailing list
[email protected]
http://lists.helixcommunity.org/mailman/listinfo/helix-client-dev