[FFmpeg-devel] [PATCH] VP8 de/encode via libvpx

[David Conrad]

On May 19, 2010, at 2:28 PM, James Zern wrote:

> VP8 and associated WebM container made public today [1].
> 
> The attached will enable decode and basic encode of VP8 through
> libvpx. I split the encode as some of the options mapping done will
> probably be questionable so I thought that would do with its own
> thread.
> 
> Some notes and questions regarding the sources:
> - libvpx{enc,dec} currently have a BSD style license, if it doesn't
> make sense in this context we can go to LGPL

Doesn't much matter, but I think shorter license headers might be nice

> - the configure test might do to be split for encode/decode

Only if disabling the encoder or decoder in libvpx means the relevant headers aren't installed or linking fails.

> - also related to configure and the sources, currently the library
> does not install includes to vpx/, for example, so the check is done
> just in the current include path

Does the libvpx install target work? It only reruns configure for me and I had to install the headers and libs manually.

> - libvpx can be built with multi-thread support (libpthread). Is it up
> to the user to add --enable-pthreads in this case or should that be
> rolled into this specific test?

It's up to the user for the moment as with libx264, but this is another reason for pthreads autodetection.

I've already applied the CODEC_ID_VP8 and fourcc/V_VP8 patches

The rest of the patches should be combined into one, or at least split into no more than 1. new options+version bump, 2. decoder+version bump, 3. encoder+version bump

Also, ffmpeg's indentation policy is 4 spaces.

> --- /dev/null	2010-02-26 16:50:52.000000000 -0500
> +++ libavcodec/libvpxdec.c	2010-05-17 23:46:16.000000000 -0400
> @@ -0,0 +1,168 @@
> 
> [...]
> 
> +#include "avcodec.h"
> +
> +#ifndef HAVE_STDINT_H
> +# define HAVE_STDINT_H 1
> +#endif
> +#define VPX_CODEC_DISABLE_COMPAT 1

Is all that really needed?

> +#if (defined(CONFIG_LIBPOSTPROC) && CONFIG_LIBPOSTPROC) ||\
> +    (defined(CONFIG_POSTPROC) && CONFIG_POSTPROC)
> +  const vpx_codec_flags_t flags = 0;
> +#else
> +  const vpx_codec_flags_t flags = VPX_CODEC_USE_POSTPROC;
> +#endif
> +
> +  av_log(avctx,AV_LOG_INFO,"%s\n",vpx_codec_version_str());
> +  av_log(avctx,AV_LOG_VERBOSE,"%s\n",vpx_codec_build_config());
> +
> +  if(vpx_codec_dec_init(&ctx->decoder,iface,&deccfg,flags)!=VPX_CODEC_OK) {
> +    const char* error = vpx_codec_error(&ctx->decoder);
> +    av_log(avctx,AV_LOG_ERROR,"Failed to initialize decoder: %s\n",error);
> +    return -1;
> +  }
> +
> +  /*FIXME set based on user parameters. for now we'll disable based on
> +    libpostproc presence in mplayer/ffmpeg based builds*/
> +  if(flags&VPX_CODEC_USE_POSTPROC) {
> +    ppcfg.post_proc_flag   = VP8_DEMACROBLOCK|VP8_DEBLOCK|VP8_ADDNOISE;
> +    ppcfg.deblocking_level = 5;
> +    ppcfg.noise_level      = 1;
> +    vpx_codec_control(&ctx->decoder,VP8_SET_POSTPROC,&ppcfg);
> +  }

IMO, a decoder shouldn't do postprocessing not required by the standard (e.g. in-loop filtering only)

> +  if( (img= vpx_codec_get_frame(&ctx->decoder,&iter)) ) {
> +    assert(img->fmt==IMG_FMT_I420);

Return an error rather than assert()

> +    picture->data[0] = img->planes[0];
> +    picture->data[1] = img->planes[1];
> +    picture->data[2] = img->planes[2];
> +    picture->data[3] = img->planes[3];
> +    picture->linesize[0] = img->stride[0];
> +    picture->linesize[1] = img->stride[1];
> +    picture->linesize[2] = img->stride[2];
> +    picture->linesize[3] = img->stride[3];


No need to copy the 4th plane if it only supports yuv420p

> --- /dev/null	2010-02-26 16:50:52.000000000 -0500
> +++ libavcodec/libvpxenc.c	2010-05-18 21:54:12.000000000 -0400
> @@ -0,0 +1,446 @@
> [...]

> +  void                    *buf;      /**< compressed data buffer */
> +  size_t                   sz;       /**< length of compressed data */
> +  vpx_codec_pts_t          pts;      /**< time stamp to show frame
> +                                          (in timebase units) */
> +  unsigned long            duration; /**< duration to show frame
> +                                          (in timebase units) */
> +  vpx_codec_frame_flags_t  flags;    /**< flags for this frame */
> +  struct FrameListData    *next;
> +} coded_frame_t;
> +
> +typedef struct VP8EncoderContext {
> +  vpx_codec_ctx_t encoder;
> +  vpx_image_t rawimg;
> +  vpx_fixed_buf_t twopass_stats;
> +  unsigned long deadline; //i.e., RT/GOOD/BEST
> +  coded_frame_t* coded_frame_list;
> +} vp8ctx_t;

Don't use the _t namespace, it's reserved by POSIX

> +static int vp8_free(AVCodecContext *avctx);
> +static void free_frame_list(coded_frame_t* list);
> +static void free_coded_frame(coded_frame_t* cx_frame);
> +static void coded_frame_add(void* list, coded_frame_t* cx_frame);
> +static void log_encoder_error(AVCodecContext *avctx, const char* desc);
> +static void dump_enc_cfg(AVCodecContext *avctx, const vpx_codec_enc_cfg_t* cfg);

Arrange functions so forward prototypes aren't needed

> +  { vpx_codec_err_t res;
> +  const vpx_codec_pts_t timestamp = frame?frame->pts:0;
> +  const unsigned long framedur = avctx->ticks_per_frame;
> +  const vpx_enc_frame_flags_t flags = 0; //VPX_EFLAG_FORCE_KF
> +  const unsigned long deadline = ctx->deadline;

Random conditionless subblocks are ugly, and const on local non-pointer variables is unnecessary.