[MPlayer-dev-eng] [PATCH] unified timing patch for H264
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Sat Aug 21 13:44:27 CEST 2010
On Sat, Aug 21, 2010 at 12:37:10PM +0200, Pásztor Szilárd wrote:
> On Sat, 21 Aug 2010 08:24:35 +0200, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> >> if (cur->field_poc[0]==INT_MAX || cur->field_poc[1]==INT_MAX) {
> >> /* Wait for second field. */
> >> - *data_size = 0;
> >> + *data_size = -1;
> >
> > You can't use it like that, that's a size field and setting it like
> > this would mean *data points to almost 4 GB of data - at least unless
> > you change the API which requires a major verson bump, and that
> > won't happen so soon.
> > That's why I suggested changing the return value.
>
> Ok, here it is corrected. I introduced -2 as retval.
> Magic numbers are not nice, but that's how it is in
> lavc anyway.
Not, that's not at all how it is, -1 is the only special
value with no meaning, AVERROR(E*) or AVERROR_* is to be
used for anything else.
Some value was suggest on ffmpeg-devel the last time
this was suggested, but I don't remember which,
maybe AVERROR(EAGAIN) or AVERROR(EINVAL) or...
> @@ -796,6 +796,7 @@ static mp_image_t *decode(sh_video_t *sh
> AVFrame *pic= ctx->pic;
> AVCodecContext *avctx = ctx->avctx;
> mp_image_t *mpi=NULL;
> + static mp_image_t mympi;
At least make it a proper "singleton" e.g. by declaring it
as static const in mp_image.c/h
> @@ -826,7 +827,7 @@ static mp_image_t *decode(sh_video_t *sh
> ret = avcodec_decode_video2(avctx, pic, &got_picture, &pkt);
>
> dr1= ctx->do_dr1;
> - if(ret<0) mp_msg(MSGT_DECVIDEO, MSGL_WARN, "Error while decoding frame!\n");
> + if(ret == -1) mp_msg(MSGT_DECVIDEO, MSGL_WARN, "Error while decoding frame!\n");
Not printing anything for e.g. AVERROR(ENOMEM) certainly is not a good idea.
> if (vfilter) {
> + int real;
> int softskip = (vfilter->control(vfilter, VFCTRL_SKIP_NEXT_FRAME, 0) == CONTROL_TRUE);
Indentation is off.
> @@ -1826,15 +1828,16 @@ static int generate_video_frame(sh_video
> if (in_size > max_framesize)
> max_framesize = in_size;
> current_module = "decode video";
> - decoded_frame = decode_video(sh_video, start, in_size, drop_frame, pts);
> + decoded_frame = decode_video(sh_video, start, in_size, drop_frame, pts, &real);
> if (decoded_frame) {
> + was_frame = 1;
> update_subtitles(sh_video, sh_video->pts, mpctx->d_sub, 0);
> update_teletext(sh_video, mpctx->demuxer, 0);
> update_osd_msg();
> current_module = "filter video";
> if (filter_video(sh_video, decoded_frame, sh_video->pts))
> break;
> - } else if (drop_frame)
> + } else if (was_frame && drop_frame) // disable framedropping at the very first frame
This doesn't make any sense at all to me.
You can't just continue processing when decoded_frame is NULL.
> @@ -2381,7 +2384,9 @@ static double update_video(int *blit_fra
> void *decoded_frame = NULL;
> int drop_frame=0;
> int in_size;
> + int real;
>
> + do {
> current_module = "video_read_frame";
> frame_time = sh_video->next_frame_time;
> in_size = video_read_frame(sh_video, &sh_video->next_frame_time,
> @@ -2402,15 +2407,7 @@ static double update_video(int *blit_fra
> return -1;
> if (in_size > max_framesize)
> max_framesize = in_size; // stats
> - sh_video->timer += frame_time;
> - if (mpctx->sh_audio)
> - mpctx->delay -= frame_time;
> - // video_read_frame can change fps (e.g. for ASF video)
> - vo_fps = sh_video->fps;
> drop_frame = check_framedrop(frame_time);
> - update_subtitles(sh_video, sh_video->pts, mpctx->d_sub, 0);
> - update_teletext(sh_video, mpctx->demuxer, 0);
> - update_osd_msg();
> current_module = "decode_video";
> #ifdef CONFIG_DVDNAV
> decoded_frame = mp_dvdnav_restore_smpi(&in_size,&start,decoded_frame);
> @@ -2418,11 +2415,41 @@ static double update_video(int *blit_fra
> if (in_size > 0 && !decoded_frame)
> #endif
> decoded_frame = decode_video(sh_video, start, in_size, drop_frame,
> - sh_video->pts);
> + sh_video->pts, &real);
> +
> + if (real)
Huh? Wouldn't that possibly use "real" uninitialized in the dvdnav case?
More information about the MPlayer-dev-eng
mailing list