[vlc-devel] [PATCH 2/2] vpx: add vp8 and vp9 encoder

[Jean-Baptiste Kempf]

On 08 Feb, Tristan Matthews wrote :
> diff --git a/NEWS b/NEWS
> diff --git a/configure.ac b/configure.ac
> diff --git a/modules/MODULES_LIST b/modules/MODULES_LIST

All OK.

> +++ b/modules/codec/vpx.c
> @@ -34,11 +34,21 @@
>  #include <vpx/vpx_decoder.h>
>  #include <vpx/vp8dx.h>
>  
> +#ifdef ENABLE_SOUT
> +#include <vpx/vpx_encoder.h>
> +#include <vpx/vp8cx.h>
> +#endif

I prefer indenting in those ifdefs.

>  /****************************************************************************
>   * Local prototypes
>   ****************************************************************************/
> -static int Open(vlc_object_t *);
> -static void Close(vlc_object_t *);
> +static int OpenDecoder(vlc_object_t *);
> +static void CloseDecoder(vlc_object_t *);

You should have done the renaming in a different patch.

> -    msg_Dbg(p_this, "VP%d: using libvpx version %s (build options %s)", 
> +    msg_Dbg(p_this, "VP%d: using libvpx version %s (build options %s)",

Ynrelated to the encoder?

> +#ifdef ENABLE_SOUT
> +
> +/*****************************************************************************
> + * encoder_sys_t: libvpx encoder descriptor
> + *****************************************************************************/
> +struct encoder_sys_t
> +{
> +    struct vpx_codec_ctx ctx;
> +};
> +
> +/*****************************************************************************
> + * OpenEncoder: probe the encoder
> + *****************************************************************************/
> +static int OpenEncoder(vlc_object_t *p_this)
> +{
> +    encoder_t *p_enc = (encoder_t *)p_this;
> +    encoder_sys_t *p_sys;
> +
> +    const struct vpx_codec_iface *iface;
> +    int vp_version;
> +
> +    p_enc->pf_encode_video = Encode;
> +    p_enc->fmt_in.i_codec = VLC_CODEC_I420;

I argue you need to set those after the easy fail cases.

> +    switch (p_enc->fmt_out.i_codec)
> +    {
> +#ifdef ENABLE_VP8_ENCODER
> +    case VLC_CODEC_VP8:
> +        iface = &vpx_codec_vp8_cx_algo;
> +        vp_version = 8;
> +        break;
> +#endif
> +#ifdef ENABLE_VP9_DECODER
> +    case VLC_CODEC_VP9:
> +        iface = &vpx_codec_vp9_cx_algo;
> +        vp_version = 9;
> +        break;
> +#endif
> +    default:
> +        return VLC_EGENERIC;
> +    }
> +
> +    p_sys = malloc(sizeof(*p_sys));
> +    if (p_sys == NULL)
> +        return VLC_ENOMEM;
> +    p_enc->p_sys = p_sys;
> +
> +    struct vpx_codec_enc_cfg enccfg = {};

Are you sure this is the correct way and this is portable?

> +    enccfg.g_threads = __MIN(vlc_GetCPUCount(), 16);

16 is a bit much, no?

> +/****************************************************************************
> + * Encode: the whole thing
> + ****************************************************************************/
> +static block_t *Encode(encoder_t *p_enc, picture_t *p_pict)
> +{
> +    encoder_sys_t *p_sys = p_enc->p_sys;
> +    struct vpx_codec_ctx *ctx = &p_sys->ctx;
> +    vlc_object_t *p_this = (vlc_object_t *)p_enc;
> +
> +    if (!p_pict) return NULL;
> +
> +    vpx_image_t img = {};

idem

> +    unsigned i_w = p_enc->fmt_in.video.i_visible_width;
> +    unsigned i_h = p_enc->fmt_in.video.i_visible_height;
> +
> +    /* Create and initialize the vpx_image */
> +    if (!vpx_img_alloc(&img, VPX_IMG_FMT_I420, i_w, i_h, 1)) {
> +        vpx_err_msg(p_this, ctx, "Failed to allocate image: %s (%s)");
> +        return NULL;
> +    }
> +
> +    for (int plane = 0; plane < p_pict->i_planes; plane++) {
> +        uint8_t *src = p_pict->p[plane].p_pixels;
> +        uint8_t *dst = img.planes[plane];
> +        int src_stride = p_pict->p[plane].i_pitch;
> +        int dst_stride = img.stride[plane];
> +
> +        int size = __MIN(src_stride, dst_stride);
> +        for (int line = 0; line < p_pict->p[plane].i_visible_lines; line++)
> +        {
> +            memcpy(dst, src, size);
> +            src += src_stride;
> +            dst += dst_stride;
> +        }
> +    }

Why do you need to copy here?

> +    /* TODO: make quality configurable */

That shouldn't be hard to had, tbh.

> +    int flags = 0;
> +    vpx_codec_err_t res = vpx_codec_encode(ctx, &img, p_pict->date, 1,
> +     flags, VPX_DL_BEST_QUALITY);
> +    if (res != VPX_CODEC_OK) {
> +        vpx_err_msg(p_this, ctx, "Failed to encode frame: %s (%s)");
> +        vpx_img_free(&img);
> +        return NULL;
> +    }
> +
> +    const vpx_codec_cx_pkt_t *pkt = NULL;
> +    vpx_codec_iter_t iter = NULL;
> +    block_t *p_out = NULL;
> +    while ((pkt = vpx_codec_get_cx_data(ctx, &iter)) != NULL)
> +    {
> +        if (pkt->kind == VPX_CODEC_CX_FRAME_PKT)
> +        {
> +            int keyframe = pkt->data.frame.flags & VPX_FRAME_IS_KEY;
> +            block_t *p_block = block_Alloc(pkt->data.frame.sz);
> +
> +            memcpy(p_block->p_buffer, pkt->data.frame.buf, pkt->data.frame.sz);
> +            p_block->i_dts = p_block->i_pts = pkt->data.frame.pts;
> +            if (keyframe)
> +                p_block->i_flags |= BLOCK_FLAG_TYPE_I;
> +            block_ChainAppend(&p_out, p_block);
> +        }
> +    }
> +    vpx_img_free(&img);
> +    return p_out;
> +}
> +
> +/*****************************************************************************
> + * CloseEncoder: encoder destruction
> + *****************************************************************************/
> +static void CloseEncoder(vlc_object_t *p_this)
> +{
> +    encoder_t *p_enc = (encoder_t *)p_this;
> +    encoder_sys_t *p_sys = p_enc->p_sys;
> +    if (vpx_codec_destroy(&p_sys->ctx))
> +        vpx_err_msg(p_this, &p_sys->ctx, "Failed to destroy codec: %s (%s)");
> +    free(p_sys);
> +}


Looks very cool, all in all, and a great addition!


With my kindest regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device