[FFmpeg-soc] libavfilter: movie source filter

[Michael Niedermayer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080126/785e57a6/attachment.pgp>