RE: 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