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

[John Donaghy]

Hi

Thanks to you and Nico for reviewing the patch. I've attached an
updated version based on your comments...

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

I removed the comments and put the if statements on separate lines

>
> [...]
> > +      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.

I decided the cast was pointless so I removed it.

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

I added some brief doxygen-compatible comments to the two new
functions apart from demux_close

>
> > +    struct asf_priv* asf = demux->priv;
> > +    unsigned char* p=*pp;
>
> p seems to be used only to initialize pi, so IMO useless.

I got rid of it

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

done

>
> [...]
> > +    pi = (uint8_t *) p+4;
>
> useless cast, "uint8_t" and "unsigned char" are the same in all cases
> where MPlayer works at all AFAICT.

cast removed

>
> [...]
> > +    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).

I followed your suggestion here too.

John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: asf_dvrms_b.patch
Type: application/octet-stream
Size: 18423 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070503/f439021c/attachment.obj>