[MPlayer-dev-eng] [PATCH] asf demuxer changes for dvr-ms
John Donaghy
johnfdonaghy at gmail.com
Thu May 3 17:52:17 CEST 2007
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>
More information about the MPlayer-dev-eng
mailing list