[MPlayer-dev-eng] [PATCH] asf demuxer changes for dvr-ms

[Reimar Döffinger]

Hello,
On Tue, Apr 10, 2007 at 10:24:50PM -0500, John Donaghy wrote:
>    while ((pos = find_asf_guid(buf, asf_ext_stream_header, pos, buf_len)) >= 0) {
>      int this_stream_num, stnamect, payct, i, objlen;
> +    int buf_max_index=pos+50;
> +    if (buf_max_index > buf_len) return 0; //overflow check

I'd prefer to have these comments removed, everyone not immediately realizing what
this "if" does has not the slightest chance of understanding the other
code in that file...

>      if (this_stream_num == stream_num) {
> +      buf_max_index+=14;if (buf_max_index > buf_len) return 0; //overflow check

But putting the if in a separate line might be good.
Another possibility would be to just wrap these two things in one macro
with a nice, descriptive name.

[...]
> +      stnamect = AV_RL16(buffer);buffer+=2;
> +      payct = (int) AV_RL16(buffer);buffer+=2;

But this cast might be worth a comment. Or you could make paycnt
int16_t.

> @@ -245,6 +238,150 @@
>    return 0;
>  }
>  
> +static void get_payload_entension_data(demuxer_t *demux, unsigned char** pp, unsigned char id, unsigned int seq, int *keyframe, uint64_t *seg_time){

new functions must have a doxygen-compatible comment (stuff like
demux_close_... is of course not affected by that rule ;-) ).

> +    struct asf_priv* asf = demux->priv;
> +    unsigned char* p=*pp;

p seems to be used only to initialize pi, so IMO useless.

> +    uint64_t payload_time; //100ns units
> +    int i, ext_max, ext_timing_index;
> +    uint8_t *pi;

pi could just be initialized here instead of in an extra line below.

[...]
> +    pi = (uint8_t *) p+4;

useless cast, "uint8_t" and "unsigned char" are the same in all cases
where MPlayer works at all AFAICT.

[...]
> +    if (id==demux->audio->id) {
> +       if (payload_time != -1) {
> +           *seg_time = payload_time;
> +       } else {
> +           *seg_time = asf->last_aud_pts + asf->last_aud_diff;
> +       }
> +       asf->last_aud_diff = *seg_time - asf->last_aud_pts;
> +       asf->last_aud_pts = *seg_time;

Hmmm... How about

> if (payload_time != -1)
>   asf->last_aud_diff = payload_time - asf->last_aud_pts;
> asf->last_aud_pts += asf->last_aud_diff;
> *seg_time = asf->last_aud_pts;

(Note I assumed that *seg_time, asf->last_aud_diff and asf->last_aud_pts
all have the same type).

Greetings,
Reimar Döffinger