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

[Diego Biurrun]

On Wed, May 19, 2010 at 02:28:10PM -0400, James Zern wrote:
> 
> The attached will enable decode and basic encode of VP8 through
> libvpx.

Some general comments about your patches:

- use text/plain or text/x-diff as content type, if gmail sucks too
  much for this, get a decent mailer (or get gmail fixed)
- leave empty lines between your reply and quoted text
- use K&R style, 4 spaces indentation, no tabs, no trailing whitespace
  in the new files you add
- split your files logically, not file by file, the latter is a big
  annoyance
- try to get the decoder merged first, then submit the encoder

> 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

Given that you have a longish license + patent clause and given
that it makes no difference in practice I suggest you use
FFmpeg's standard LGPL header.

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

yes

> - 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

Sorry, but how lame is that?  It should not take you more than half
a day to fix if you are very inexperienced...


I have a question: What about releasing sources for the other On2 codecs
as well?  There is VP4, VP5, VP6, VP7 video and the On2 Audio for Video
audio codec waiting to be implemented in FFmpeg (VP6 is incomplete in
FFmpeg IIRC).

> --- /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 @@
> +
> +/*!\file
> +   \brief VP8 decoder support via libvpx
> +*/

The Javadoc-style Doxygen variant is preferred.

> +#include "avcodec.h"

system headers before local headers please

> +#ifndef HAVE_STDINT_H
> +# define HAVE_STDINT_H 1
> +#endif

WTH?

> +#define VPX_CODEC_DISABLE_COMPAT 1
> +#include <vpx_decoder.h>
> +#include <vp8dx.h>
> +
> +#include <assert.h>
> +
> +typedef struct VP8DecoderContext {
> +  vpx_codec_ctx_t decoder;
> +} vp8dctx_t;

The _t namespace is reserved by POSIX, don't invade it.

> +static av_cold int vp8_init(AVCodecContext* avctx)
> +{
> +  vp8dctx_t* const ctx = avctx->priv_data;
> +  vpx_codec_iface_t* const iface = &vpx_codec_vp8_dx_algo;
> +  vpx_codec_dec_cfg_t deccfg = { /*token partitions+1 would be a decent choice*/
> +                                 .threads= FFMIN(avctx->thread_count,16) };
> +  vp8_postproc_cfg_t ppcfg;
> +#if (defined(CONFIG_LIBPOSTPROC) && CONFIG_LIBPOSTPROC) ||\
> +    (defined(CONFIG_POSTPROC) && CONFIG_POSTPROC)

I dunno who came up with this redundancy, but it's nonsense.

> +  av_log(avctx,AV_LOG_INFO,"%s\n",vpx_codec_version_str());
> +  av_log(avctx,AV_LOG_VERBOSE,"%s\n",vpx_codec_build_config());

K&R style: spaces after commas

> +  if(vpx_codec_dec_init(&ctx->decoder,iface,&deccfg,flags)!=VPX_CODEC_OK) {

K&R style: spaces after keywords and around operators

> +  if( (img= vpx_codec_get_frame(&ctx->decoder,&iter)) ) {

no spaces inside ()

> +    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];
> +    *data_size = sizeof(AVPicture);

The = could be vertically aligned.

> --- configure	(revision 23157)
> +++ configure	(working copy)
> @@ -183,6 +183,7 @@ External library support:
> +  --enable-libvpx-vp8      enable VP8 support via libvpx [no]

Does the name of the lib really contain "vp8"?  If not, then drop the
silly "-vp8" postfix.

> @@ -2617,6 +2621,9 @@ enabled libschroedinger && add_cflags $(
> +enabled libvpx_vp8 && add_cflags -DHAVE_STDINT_H &&

What's the weird preprocessor flag for?  Fix that in your library.
You can safely assume that stdint.h is available on a system that
can decode decode VP8.  The 80s are well behind us.

> --- /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 @@

same comments as above apply

> +typedef struct FrameListData {

What is it that people have with typedeffing structs?
I'd drop the typedef...

> +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);

These are unacceptable, just reorder your functions properly.

> +  enccfg.g_w = avctx->width;
> +  enccfg.g_h = avctx->height;
> +
> +  enccfg.g_timebase.num = avctx->time_base.num;
> +  enccfg.g_timebase.den = avctx->time_base.den;

align the =

> +  enccfg.rc_min_quantizer = ((avctx->qmin*5+1)>>2) - 1;
> +  enccfg.rc_max_quantizer = ((avctx->qmax*5+1)>>2) - 1;

This looks rather cramped, give the poor operators a few spaces

> +  if(enccfg.g_pass==VPX_RC_FIRST_PASS) enccfg.g_lag_in_frames= 0;

statements on the next line please

> +static int queue_frames(AVCodecContext *avctx, uint8_t* buf, int buf_size, AVFrame* coded_frame)

Break overly long lines where easily possible.
K&R places the * with the variable, not with the type.

> --- doc/general.texi	(revision 23153)
> +++ doc/general.texi	(working copy)
> @@ -481,6 +481,8 @@ following image formats are supported:
>  @item V210 Quicktime Uncompressed 4:2:2 10-bit     @tab  X  @tab  X
>  @item VMware Screen Codec / VMware Video  @tab     @tab  X
>      @tab Codec used in videos captured by VMware.
> + at item VP8                    @tab  E  @tab  E
> +    @tab fourcc: VP80, encoding/decoding supported through external library libvpx

Place it together with the other On2 codecs.

Diego