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

Tristan Matthews tmatth at videolan.org
Tue Feb 9 03:37:36 CET 2016


On Mon, Feb 8, 2016 at 6:29 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
> 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?

This is how I interpret the standard. From 6.7.8 Initialization:
"If there are fewer initializers in a brace-enclosed list than there
are elements or members of an aggregate, or fewer characters in a
string literal used to initialize an array of known size than there
are elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage
duration."

>
>> +    enccfg.g_threads = __MIN(vlc_GetCPUCount(), 16);
>
> 16 is a bit much, no?

It's what the decoder uses as an upper limit as well, but I could
bring it down to 4.

>
>> +/****************************************************************************
>> + * 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?

The alternative would be to use vpx_img_wrap (instead of
vpx_img_alloc) which allows you to wrap an existing buffer, but it
doesn't allow for independent strides per plane. See Decode() which
does the same copy  (but in the reverse direction).

>
>> +    /* 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list