[prev in list] [next in list] [prev in thread] [next in thread] 

List:       helix-datatype-dev
Subject:    Re: [datatype-dev] Fix to avoid negative ctts table values.
From:       Jamie Gordon <jgordon () real ! com>
Date:       2010-02-22 22:54:25
Message-ID: 4B830B21.7000507 () real ! com
[Download RAW message or body]

On 2/22/2010 1:07 PM, Steve Blanding wrote:
> There are many instances where I have seen this assert fire with no
> adverse effects to the output. Our code seems to correctly deal with
> the condition. Leaving it in makes debugging a nuisance and serves no
> useful purpose that I can see.

I am not so concerned about whether it asserts or not, but the fact is
that sample times MUST be strictly increasing, so the condition of the
assert does need to be met for the file to be valid. It actually needs
to meet both conditions (uiTime > m_uiLastTimeStamp) or else the file
being created is not valid and may very well have issues playing or
streaming properly with various players/servers. (I am assuming that
here uiTime is the sample time for the current sample and
m_uiLastTimeStamp is the sample time for the previous sample.)

If you are hitting this condition, then it seems likely that something
is wrong further upstream.

Thanks,
Jamie

> 
>> -----Original Message----- From: Jamie Gordon Sent: Monday,
>> February 22, 2010 10:53 AM To: Gregory Wright Cc: Steve Blanding;
>> player-dev@real.com; datatype- dev@helixcommunity.org Subject: Re:
>> [datatype-dev] Fix to avoid negative ctts table values.
>> 
>> I don't think this is correct, I believe the sample time must be 
>> strictly increasing. Composition time has no such restriction and 
>> can be out of order.
>> 
>> On 2/22/2010 8:46 AM, Gregory Wright wrote:
>>>> //sample timestamp must be strictly incremental -
>>>> HX_ASSERT(uiTime != m_uiLastTimeStamp); +
>>>> HX_ASSERT(uiTime >= m_uiLastTimeStamp);
>>> Little thing, but the comment should read monotonically
>>> increasing, not strictly increasing. The assert change changes
>>> from always increasing (well, always different anyway) to
>>> increasing or staying the same. I assume that was an intended
>>> change.
>>> 
>>> rest looks good. --greg.
>>> 
>>> 
>>> 
>>> 
>>> 
>>> On Feb 19, 2010, at 11:21 AM, Steve Blanding wrote:
>>> 
>>>> We need to include the ctts table to handle out of order frames
>>>> 
>> properly, that much was already checked in, but we also need to be
>> sure that we don't ever put negative values in the ctts table.
>>>> This fix avoids that situation by adjusting the decoding time
>>>> as
>> necessary to avoid a negative  ctts time.
>>>> There are also a couple of minor tweaks to make it easier to
>>>> debug
>> situations like this (using some local variables to avoid redundant
>>  method calls and break down some mathematical logic).
>>>> And finally, there's a tweak to an assertion to avoid asserting
>>>> on a
>> benign condition.
>>>> Index: mp4sm.cpp 
>>>> ===================================================================
>>>>  RCS file: /cvsroot/datatype/mp4/filewriter/mp4sm.cpp,v 
>>>> retrieving revision 1.3.2.10 diff -u -w -r1.3.2.10 mp4sm.cpp 
>>>> --- mp4sm.cpp  11 Feb 2010 21:00:51 -0000           1.3.2.10 
>>>> +++ mp4sm.cpp               19 Feb 2010 19:16:33 -0000 @@
>>>> -231,7 +231,8 @@ m_uiSampleCounter++;
>>>> 
>>>> // Determine the sample time and composition time -    uiTime =
>>>> (UINT32)(((UINT64)pPacket->GetTime() * m_uiTimescale)
>> / 1000);
>>>> +    UINT32 uiPacketTime = pPacket->GetTime(); +    uiTime =
>>>> (UINT32)(((UINT64)uiPacketTime * m_uiTimescale) /
>> 1000);
>>>> if (bIsRTP) { IHXRTPPacket *rp = NULL; @@ -240,17 +241,24 @@ 
>>>> HX_ASSERT(rp != NULL); ct = rp->GetRTPTime(); HX_RELEASE(rp); -
>>>> if (abs((int)(((UINT64)ct * 1000) / m_uiTimescale -
>> pPacket->GetTime())) <= 1 &&
>>>> +             int iDelta = (int)((UINT32)((UINT64)ct * 1000 /
>> m_uiTimescale ) - uiPacketTime);
>>>> +             if (abs(iDelta) <= 1 && ct >= m_uiLastTimeStamp) 
>>>> { uiTime = ct; } + +        if (ct < uiTime) +        { +
>>>> uiTime = ct; +        } } else { ct = uiTime; }
>>>> 
>>>> + UINT32 coff = ct - uiTime;
>>>> 
>>>> // Create a ctts box if necessary @@ -287,7 +295,7 @@ else { 
>>>> //sample timestamp must be strictly incremental -
>>>> HX_ASSERT(uiTime != m_uiLastTimeStamp); +
>>>> HX_ASSERT(uiTime >= m_uiLastTimeStamp);
>>>> 
>>>> //update sample duration for the PREVIOUS sample //this leaves
>>>> the last sample without an entry in
>> stss
>>>> @@ -438,6 +446,9 @@ // stts variable part ulEstimatedSize +=
>>>> ulEstimatedPackets * (sizeof(UINT32)*2);
>>>> 
>>>> +        // ctts variable part +        ulEstimatedSize +=
>>>> ulEstimatedPackets * (sizeof(UINT32)*2); + // stco variable
>>>> part ulEstimatedSize +=
>> (ulEstimatedPackets/SAMPLE_WRAP_COUNT + 1) * sizeof(UINT32);
>>>> <ATT00001..txt>
>>> 
>>> _______________________________________________ Datatype-dev
>>> mailing list Datatype-dev@helixcommunity.org 
>>> http://lists.helixcommunity.org/mailman/listinfo/datatype-dev

_______________________________________________
Datatype-dev mailing list
Datatype-dev@helixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/datatype-dev
[prev in list] [next in list] [prev in thread] [next in thread]