[MPlayer-dev-eng] Patches for NUT

[Oded Shimon]

On Thu, Feb 09, 2006 at 12:03:35PM +0100, Michael Niedermayer wrote:
> Hi
> 
> On Thu, Feb 09, 2006 at 07:55:54AM +0200, Oded Shimon wrote:
> > On Tue, Feb 07, 2006 at 01:48:25PM +0200, Oded Shimon wrote:
> > > On Tue, Feb 07, 2006 at 12:04:07PM +0100, Michael Niedermayer wrote:
> > > > On Tue, Feb 07, 2006 at 06:04:11AM +0200, Oded Shimon wrote:
> > > > > On Mon, Feb 06, 2006 at 03:39:23PM +0100, Michael Niedermayer wrote:
> > > > > > yep, wanted to comment on that anyway ...
> > > > > > adler isnt the best checksum around, but its fast and simple
> > > > > > the adler checksum is made of 2 parts s1 and s2
> > > > > > 
> > > > > > [...]
> > > > > 
> > > > > So, in conclusion.... ?? What should we do? Add adler32 for the syncpoint? 
> > > > > Something else?
> > > > 
> > > > maybe switch to crc32 everywhere, iam not sure
> > > 
> > > I hate the LUT spam of crc32. Although it is ofcourse easy to implement...
> 
> #define G 0x1EDC6F41
> int lutless_crc(uint8_t *buf, int size){
>     int i, j, crc=0;
> 
>     for(i=0; i<size; i++){
>         crc ^= buf[i]<<24;
>         for(j=0; j<8; j++)
>             crc = (crc<<1) ^ (G & (crc>>31));
>     }
>     return crc;
> }

Slow, but cool. :) Weird const.

> > > 
> > > I'm not so paranoid about error detection and this stuff, Adler is good 
> > > enough for me. We can use a funky 24 bit adler for the syncpoint checksum, 
> > > 10 bits for s1, 14 bits for s2. Really not hard to implement and not a big 
> > > complication to spec IMO.
> > 
> > Here's my patch.. Please comment.
> 
> iam strongly against putting the checksum anywhere except the end of the area
> to be checked, (it also violates richs no buffering rule)

Well, forward_ptr of headers already violates that. I think he meant no 
buffering of frames, which is still kept, you just need to write the frame 
header in memory, which you would have to anyway if you did checksum.

> and iam not happy about the 24bit hack either, either store adler32 or crc32
> everywhere IMHO

I suppose crc32 then, it's more common...

> > Other than this, the last issues I'm aware of are:
> > 1. too big pts - stuck player
> 
> several possile solutions
> A: decide some pts_threshold, and put syncpoints (with checksums) before
>    all frames which have a pts diff above the threshold
>    subtitle streams will have large pts diff -> 1 syncpoint per
>    subtitle packet in not so unlikely cases or pts_threshold large but then
>    we wont catch many errors, sure the really problematic cases are caught...

I like this... You can set high pts distance for subtitles and it wouldn't 
cause stuck player with damage, so that's ok. maybe the pts_threshold 
should be global and be a pts difference from last frame of any stream.

Makes it kind of hard to measure though, I suppose real time would be the 
right way... Or basically just specify a timebase and the max pts 
difference would be a single unit of that timebase (so, 1/2 for making max 
pts diff without a syncpoint never more than half a second.)

I like this solution the most.. In implementation:

tmp = convert_ts(max_last_pts, max_last_pts_timebase, this_timebase);
if (compare_ts(ABS(this_pts - tmp), this_timebase, 1, max_pts_timebase) >= 0) put_syncpoint();

Maybe without the ABS. Whatever.

> B: store max_pts after muxing
>    same problem as A (we wont catch many errors, sure the really 
>    problematic cases are caught) and theres the issue with storing max_pts
>    either we seek back or we risk loosing it if the file is truncated

Obviously this idea has many flaws...

> C: give syncpoints and frame headers their own checksums, add a checksum
>    flag to the frame header table
>    this avoids the 1 checksum covers syncpoint and following header 
>    complexity and the overhead of case A for large pts frames is 
>    significantly reduced, there is also no issue with storing the max_pts
>    after muxing (seeking back or truncation issue)
> 
> personally iam in favor of C

The complexity is litte, syncpoint are always accompanied by their 
following frame, you can't have a syncpoint without a frame. But I am 
a bit confused, you want the ability to store a 4 byte checksum for a 3 
byte frame header? And now both syncpoint and the following frame header 
needs a total of 8 bytes of checksum? This sounds like horrible overhead. 
(Or, is the checksum only for frame which need a big pts diff, but we 
wanted a checksum for every frame after a syncpoint, because it is excused 
from max_* rules, so it has to be correct.)

I am rather confused by this idea. However, I am willing the possibility of 
a frame flag saying add a checksum of frame header, or maybe even frame 
data. As for the pts problem, I vote for A with my addition.

> > 2. index_ptr comes out of nowhere when reading the file linearly
> > 3. split up index?
> 
> yes that would be nice

Well, best I got, just split em up at some syncpoint position, it could 
work just fine, but I don't really see the point. Damage usually comes in 
blocks, and yes the index is big, but now you can repeat it. (which is also 
highly non trivial if you repeat anywhere except EOF and begginning of 
file, the vlc's will keep changing lengths and hence changing all other 
vlc's...)
What is the rule for splitting? Every 5,000 syncpoint? When the index 
reaches 4kb? (not a good idea, near impossible to measure, because there 
are 2 parts.)

> > 4. info streams and back ptr - imo nothing should really be done about 
> >    this, just suggest to either use eor or repeat info frames.
> > 5. droppable flag for frames
> 
> i dont strongly care either way, and it seems we can add this later

Well, it's not much of a big deal, we can just add it to possible stream 
flags. The only important rule about it, it is not allowed with keyframe. I 
think that's about it.

> > 6. split up index somehow?
> 
> duplicate of 3. ...

Oops. :) The counting in my todo list didn't add up. The missing item 
instead was - fourcc for pcm.. - bah, never mind that. Not relavent for 
finalizing spec.

> > 7. frame repetition flag?
> 
> iam in favor of it ...

Well, yes, you brought it up :) I don't really like it, it's weird 
complication, I suppose it is similar to drppable flag. One important 
thing: it should have a new pts to fit its position, not the same pts as 
before, as it creates (possiblity of) weirdness of 2 syncpoints with the 
same pts but a different back_ptr, which makes no sense.

- ods15