On Sat, Jan 26, 2008 at 04:39:58PM +0100, Víctor Paesa wrote: > Hi, > > Here is attached a source filter to read video files. > > Could you please review it? [...] > Index: Makefile > =================================================================== > --- Makefile (revision 1840) > +++ Makefile (working copy) > @@ -1,6 +1,6 @@ > include ../config.mak > > -CFLAGS+=-I$(SRC_PATH)/libavcodec -I$(SRC_PATH)/libswscale > +CFLAGS+=-I$(SRC_PATH)/libavformat -I$(SRC_PATH)/libavcodec > -I$(SRC_PATH)/libswscale > > OBJS = avfilter.o \ > avfiltergraph.o \ > @@ -23,11 +23,13 @@ > vf_split.o \ > vf_transpose.o \ > vf_vflip.o \ > + vsrc_movie.o \ > avfiltergraphfile.o \ > > HEADERS = avfilter.h > > -EXTRALIBS := -L$(BUILD_ROOT)/libavcodec -lavcodec$(BUILDSUF) \ > +EXTRALIBS := -L$(BUILD_ROOT)/libavformat -lavformat$(BUILDSUF) \ > + -L$(BUILD_ROOT)/libavcodec -lavcodec$(BUILDSUF) \ > -L$(BUILD_ROOT)/libswscale -lswscale$(BUILDSUF) \ > -L$(BUILD_ROOT)/libavutil -lavutil$(BUILDSUF) $(EXTRALIBS) >
i think unconditionally adding a dependancy on libavformat is a little extreem
[...]
> +typedef struct {
> + // Filter parameters
> + double seek_point;
no floats please, floats make regression testing harder
[...]
> + if (av_open_input_file(&mv->pFormatCtx, mv->file_name, file_iformat, 0,
> NULL) != 0) {
> + av_log(NULL, AV_LOG_ERROR,
> + "movie_init() Failed to av_open_input_file '%s'\n",
> mv->file_name);
no av_log(NULL
please find some context!
> + return -1;
> + }
> + if(av_find_stream_info(mv->pFormatCtx)<0) {
> + av_log(NULL, AV_LOG_ERROR, "movie_init() Failed to find stream
> info\n");
> + return -1;
> + }
> +
> + /* if seeking requested, we execute it */
> + if (mv->seek_point > 0.0) {
> + timestamp = mv->seek_point * AV_TIME_BASE;
> + /* add the stream start time, should it exist */
> + if (mv->pFormatCtx->start_time != AV_NOPTS_VALUE)
> + timestamp += mv->pFormatCtx->start_time;
> + if (av_seek_frame(mv->pFormatCtx, -1, timestamp,
> AVSEEK_FLAG_BACKWARD) < 0) {
> + av_log(NULL, AV_LOG_ERROR, "%s: could not seek to position
> %0.3f\n",
> + mv->file_name, (double)timestamp / AV_TIME_BASE);
> + }
> + }
> +
> + /* To make things nice and easy, we simply use the first video stream we
> find */
> + mv->videoStream=-1;
> + for(i = 0; i < mv->pFormatCtx->nb_streams; i++)
> + if(mv->pFormatCtx->streams[i]->codec->codec_type==CODEC_TYPE_VIDEO)
> + {
> + mv->videoStream = i;
> + break;
> + }
the {} placement is inconsistant
[...]
> + if(pCodec == NULL) {
if(!pCodec)
[...]
> + // Hack to correct the wrong frame rates generated by some codecs
> + if (mv->pCodecCtx->time_base.den>1000 && mv->pCodecCtx->time_base.num==1)
> + mv->pCodecCtx->time_base.num=1000;
sorry this is not ok
you MUST NOT ever write into any time_base field for demuxing and decoding
[...]
> + mv->pic->pts = packet.pts * AV_TIME_BASE *
> + av_q2d(mv->pCodecCtx->time_base);
its the timebase of the AVStream you must use!
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
signature.asc
Description: Digital signature
_______________________________________________ FFmpeg-soc mailing list [email protected] http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
