[FFmpeg-devel] [PATCH] Video decoder and demuxer for AMV files

[Aurelien Jacobs]

On Mon, 24 Sep 2007 20:14:36 +0700
"Vladimir Voroshilov" <voroshil at gmail.com> wrote:

> Hi, All.
> 
> amv_codec_ffmpeg.patch: Decoder for modified MJPEG, used it AMV files.
> 
> All video frames in this file are JPEG's without header, with applied
> coded entropy data (i.e. added 0x00 bytes after each 0xff), enclosed
> in SOI and EOI markers.
> sp5x decoder code was reused.

This looks mostly ok.

> Index: ../mplayer/libavcodec/sp5xdec.c
> ===================================================================
> --- ../mplayer/libavcodec/sp5xdec.c	(revision 10560)
> +++ ../mplayer/libavcodec/sp5xdec.c	(working copy)
> @@ -72,6 +72,12 @@
>      memcpy(recoded+j, &sp5x_data_sos[0], sizeof(sp5x_data_sos));
>      j += sizeof(sp5x_data_sos);
>  
> +    if(avctx->codec_id==CODEC_ID_AMVVIDEO)
> +    for (i = 2; i < buf_size-2 && j < buf_size+1024-2; i++)
> +    {
> +        recoded[j++] = buf[i];
> +    }

Unneeded brackets, and the for() needs to be indented.

> +AVCodec amvvideo_decoder = {
> +    "amvv",
> +    CODEC_TYPE_VIDEO,
> +    CODEC_ID_AMVVIDEO,
> +    sizeof(MJpegDecodeContext),
> +    ff_mjpeg_decode_init,
> +    NULL,
> +    ff_mjpeg_decode_end,
> +    sp5x_decode_frame,
> +    CODEC_CAP_DR1,
> +    NULL
> +};

The trailing NULL is useless.

> Index: ../mplayer/libavcodec/avcodec.h
> ===================================================================
> --- ../mplayer/libavcodec/avcodec.h	(revision 10560)
> +++ ../mplayer/libavcodec/avcodec.h	(working copy)
> @@ -68,6 +68,7 @@
>      CODEC_ID_MJPEGB,
>      CODEC_ID_LJPEG,
>      CODEC_ID_SP5X,
> +    CODEC_ID_AMVVIDEO,
>      CODEC_ID_JPEGLS,
>      CODEC_ID_MPEG4,
>      CODEC_ID_RAWVIDEO,

This breaks ABI ! You must put new codec at the end of the
appropriate enum section.

> amv_demux_ffmpeg.patch: AMV file demuxer.

Better post one email per patch.

> AMV files contains ugly headers: strh sections are filled by zero,
> strf sections contains movie duration, image size and frame duration
> for video and waveformatex structure for audio respectively.
> Index is absent.
> Patch uses simplified version of avi_read_header routine for
> extracting needed info.

I didn't looked carefully, but according to your description, this
looks mostly like duplication of avi_read_header. This is not
acceptable. You should modify avi_read_header directly instead.

> Index: ../mplayer/libavformat/avidec.c
> ===================================================================
> --- ../mplayer/libavformat/avidec.c	(revision 10560)
> +++ ../mplayer/libavformat/avidec.c	(working copy)
> @@ -26,8 +26,8 @@
>  #undef NDEBUG
>  #include <assert.h>
>  
> -//#define DEBUG
> -//#define DEBUG_SEEK
> +#define DEBUG
> +#define DEBUG_SEEK

This shouldn't be part of this patch.

> Index: ../mplayer/libavformat/allformats.c
> ===================================================================
> --- ../mplayer/libavformat/allformats.c	(revision 10560)
> +++ ../mplayer/libavformat/allformats.c	(working copy)
> @@ -64,6 +64,7 @@
>      REGISTER_MUXDEMUX (AU, au);
>      REGISTER_MUXDEMUX (AUDIO_BEOS, audio_beos);
>      REGISTER_MUXDEMUX (AVI, avi);
> +    REGISTER_DEMUXER (AMV, amv);

Please keep alignment.

> P.S. I also use new fourcc (AMVV) to allow further interaction with
> mplayer.

IIRC, this is not needed.

Aurel