[FFmpeg-cvslog] r19773 - in trunk/libavformat: seek.c seek.h

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sat, Sep 05, 2009 at 09:31:01PM +0200, schreter wrote:
>   
>> Author: schreter
>> Date: Sat Sep  5 21:31:01 2009
>> New Revision: 19773
>>
>> Log:
>> cosmetic changes (indentation, doxygen comments, braces, put structures for API to header, ...)
>>     
> [...]>  /**
>   
>> - * Compare two timestamps exactly, taking into account their respective time bases.
>> + * Compare two timestamps exactly, taking their respective time bases into account.
>>   *
>> - * @param ts_a timestamp A.
>> - * @param tb_a time base for timestamp A.
>> - * @param ts_b timestamp B.
>> - * @param tb_b time base for timestamp A.
>> - * @return -1. 0 or 1 if timestamp A is less than, equal or greater than timestamp B.
>> + * @param ts_a timestamp A
>> + * @param tb_a time base for timestamp A
>> + * @param ts_b timestamp B
>> + * @param tb_b time base for timestamp A
>> + * @return -1, 0 or 1 if timestamp A is less than, equal or greater than timestamp B
>>   */
>>  static int compare_ts(int64_t ts_a, AVRational tb_a, int64_t ts_b, AVRational tb_b)
>>  {
>> @@ -95,9 +63,9 @@ static int compare_ts(int64_t ts_a, AVRa
>>      if (ts_a == INT64_MIN)
>>          return ts_a < ts_b ? -1 : 0;
>>      if (ts_a == INT64_MAX)
>> -        return ts_a > ts_b ? 1 : 0;
>> +        return ts_a > ts_b ?  1 : 0;
>>      if (ts_b == INT64_MIN)
>> -        return ts_a > ts_b ? 1 : 0;
>> +        return ts_a > ts_b ?  1 : 0;
>>      if (ts_b == INT64_MAX)
>>          return ts_a < ts_b ? -1 : 0;
>>  
>> @@ -105,7 +73,7 @@ static int compare_ts(int64_t ts_a, AVRa
>>      b = ts_b * tb_b.num * tb_a.den;
>>  
>>      res = a - b;
>> -    if (res == 0)
>> +    if (!res)
>>          return 0;
>>      else
>>          return (res >> 63) | 1;
>>     
>
> not related to your cosmetic change but this code is complete nonsense
>
>   

Since you quoted timestamp comparison routine, I suppose you are talking 
about it. Can you please explain why it is complete nonsense? It's 
actually exactly what you wanted - exact comparison of timestamps.

I can argument why it works. Consider ts_a and ts_b we want to compare 
with respective timebases tb_a and tb_b. Thus, we compare:

ts_a * N(tb_a) / D(tb_a) =?= ts_b * N(tb_b) / D(tb_b)

Multiply both sides with D(tb_a) * D(tb_b), which is positive, so 
comparison result is the same. Thus:

ts_a * N(tb_a) * D(tb_b) =?= ts_b * N(tb_b) * D(tb_a)

Comparing these two values _is_ exact and correct. The only problem is 
handling special cases with INT64_MIN/INT64_MAX, since those would 
produce undefined result on multiplication, so it has to be done first.

If you are referring to the result computation, we are comparing a and 
b. Thus, if a is less than b, obviously their difference is less than 0. 
If a is greater than b, the difference is greater than 0. If they are 
equal, difference is zero. One could do two ifs, but it works with one. 
If difference is zero, then timestamps are equal. Otherwise, we use a 
trick. Bit 64 is set to 1 for negative numbers, 0 for positive. 
Arithmetic shift right copies high-order bit to other bits. I.e., the 
result is -1 or 0. OR-ing with 1 makes it -1 or 1, thus the result is 
well-defined.

End of the proof.

But you brought me to another point: should the two timestamps be 
unsynchronized (begin at different absolute time), respective start 
times have to be subtracted before timestamp comparison. This is 
currently missing.


> please disable or revert all your recent seeking changes, this code needs
> quite a bit of bugfixing and i dont see it happening.
>   

I only have 1-2 hours per week to work on FFmpeg. I do what I can, but 
first I need to understand which bugs are there in order to fix them. My 
samples do work. As you wish, I will disable the new seeking call in 
mpegts.c by putting in the original routine and selecting old/new 
version by an #ifdef (and put back the old seek regression reference), 
so I can continue improving seeking code and people who wish to use it 
can compile it in by specifying appropriate #ifdef.

> Speaking of that your h264 timestamping code also needs work that does not
> seem to be done ...
>   

Yes. I implemented handling timestamps if the stream specifies 
appropriate SEI messages. To say it bluntly, anyone is welcome to 
continue the work to handle streams which don't specify them. I don't 
have time for that and also no need for that.

I want to invest my very limited time to make AVCHD work correctly with 
FFmpeg, so I can edit it directly. For that, I need seeking to work 
reliably, that's why I work on it in the first place. Current MPEG-TS 
implementation without my patches simply doesn't support seeking 
correctly (nor does MPEG-PS, BTW).

Regards,

Ivan