[NUT-devel] Suggestions [PATCH]

[Oded Shimon]

On Tue, Feb 28, 2006 at 10:34:22PM +0100, Michael Niedermayer wrote:
> Hi
> 
> the Attached patch does the following:
> 
> 1. cleanup packets so all have forward_ptr
> significantly improves extendability and simplifies stuff
> slightly larger overhead (~0.006% which is negligible)

No objection...

> 2. split checksum into frame and syncpoint checksum
> simplification
> overhead changes by + 4byte for every packet >64kb (<0.006% worst case)
> overhead changes by - syncpoint size + 4 for every packet with dts_diff > 1sec
> overall this could even reduce the overhead, the worst case increase is also
> negligible

Generally OK with this, some nitpiks later...

> 3. split index
> simplification for lavf muxer
>     currently we just write the data out seek back and update the forward
>     pointer, this is safe as long as the packet is < the io buffer size in
>     lavf, if its larger we need to alloc&realloc a buffer and work within that
>     or do 2 passes to find the forward ptr value, both are more messy
> index_ptr becomes unneccesary by this
> improves error resistance

Dislike. :(
It's a complication... It also makes it harder to put the index in the 
begginning cause you have to change the offset in several packets, not just 
one...

> 4. always store chapter_start/len
> simplification
> 5. change file structure definion so extendability is restored
> 6. drop info streams
> i see no sense in them ...
> 7. dissalow width or height == 0
> 8. add leading coeff to the crc polynom

OK.

> +note, demuxers MUST be able to deal with new and unknown headers

One thing about this, what if the forward_ptr of this unkown header is 
100mb and you just lost half the file... Oh, just noticed, even new headers 
have checksums, ok then...

> +prolog
> +        startcode                               f(64)
> +        forward_ptr                             v
> +
> +epilog
> +        reserved_bytes
> +        checksum                                u(32)

I slightlydislikes the names :) It sounds like a book... How about 
packet_header and packet_footer ....

> +file:
> +    file_id_string
> +    while(!eof){
> +        prolog, main_header, epilog
> +        reserved_headers
>          for(i=0; i<stream_count; i++){
> -            stream_header
> +            prolog, stream_header, epilog
> +            reserved_headers
>          }
>          while(next_code == info_startcode){
> -            info_packet
> +            prolog, info_packet, epilog
> +            reserved_headers
>          }
> -        if(next_code == index_startcode){
> -            index
> +        while(next_code == index_startcode){
> +            prolog, index_packet, epilog
> +            reserved_headers
>          }
>          if (!eof) while(next_code != main_startcode){
> -            if(next_code == syncpoint_startcode)
> -                syncpoint
> +            if(next_code == syncpoint_startcode){
> +                prolog, syncpoint, epilog
> +            }
>              frame
> +            reserved_headers
>          }
>      }

Does this mean info packets can only come after headers?... What if the 
headers are large and the radio station wants to switch songs, repeat the 
entire headers and then the info packets?

> @@ -476,12 +500,15 @@
>        1  is_key             if set, frame is keyframe
>        2  end_of_relevance   if set, stream has no relevance on
>                              presentation. (EOR)
> +      4  has_checksum       if set then the frame header contains a checksum

Maybe this should be a NUT-flag?

>      EOR frames MUST be zero-length and must be set keyframe.
>      All streams SHOULD end with EOR, where the pts of the EOR indicates the
>      end presentation time of the final frame.
>      An EOR set stream is unset by the first content frames.
>      EOR can only be unset in streams with zero decode_delay .
> +    has_checksum must be set if the frame is larger then 64kb or its

2*max_distance ?

> +    dts differs by more then 1 second from the dts of the last frame

Can we use last_pts/max_pts_distance ? What I liked so much about that 
solution is that it worked even after a seek or damage, it required no 
additional variable or context.
Also, does the syncpoint rule of max_pts_distance still apply? or is it 
just this checksum rule.

>  checksum
>      crc32 checksum
>      checksum is calculated for the area pointed to by forward_ptr not
>      including the checksum itself (from first byte after the
>      forward_ptr until last byte before the checksum).

You should probably mention what area the checksum of frame headers 
cover...

> @@ -729,21 +743,30 @@
>  If an index is written anywhere in the file, it MUST be written at end of
>  file as well.
>  
> +Each index packet SHOULD be <4kb, that way a demuxer can simply:

This is extremely hard to enforce, the index has 2 parts, the syncpoint 
positions and the keyframe pts stuff. The keyframe pts is usually pretty 
small so you could just stop when you reach 4kb when writing syncpoint 
positions, and hope that the other part doesn't grow past 4kb...

> +for(x=fileend-8192; ; x-=8192){
> +    search index startcode
> +    if(failure)
> +        break;
> +    read_index
> +}

Also, as Rich noted, this is a bad way to find an index...

- ods15