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

[Michael Niedermayer]

On Sun, Sep 06, 2009 at 04:49:51PM +0200, Ivan Schreter wrote:
> 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.


you multiply 2 32bit values and a 64 bit value, this needs 128bit but it
doesnt have that amount
i belive i quoted code that does work when when suggested this, if not i
can now if needed ...


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

we would need a formal mathematical proof not an argument but given my
observation above i dont think the code works


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

the > / < comparissions are not guranteed to maintain the same value under
multiplication in Z/Zn or if you prefer one could say multiplication can
overflow or one could say that the > / < values can change for multiplications
modulo C


iam snipping the rest of your comment because it makes no sense to me

[...]

> 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.

no, they should be synchronized


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

i suspected something like that already and thats why i requested the code to
be disabled. Thanks again for disabling it ... 2 hours a week is rather
little to have not completely working code in a supposedly stable library


> first I need to understand which bugs are there in order to fix them. My 

Iam myself somewhat busy and so i havnt really fully tested or reviewed the
code, but i do report all issues i see ...


> 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).

i know mpeg seeking is buggy, iam sure you will get it working but that might
take a little time with just 2h per week ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090906/b9d0bc0a/attachment.pgp>