[FFmpeg-devel] [PATCH] Add libx265 encoder

[Derek Buitenhuis]

On 2/11/2014 7:12 PM, Clément Bœsch wrote:
>> +enabled libx265           && require_pkg_config x265 x265.h x265_encoder_encode &&
>> +                             { check_cpp_condition x265.h "X265_BUILD >= 5" ||
>> +                               die "ERROR: libx265 version must be >= 5."; }
> 
> maybe require_pkg_config "x265 >= 5" x265.h x265_encoder_encode

Ah. Wasn't sure if there was a way to do this or not.

>> +/*
>> + * libx265 encoder
>> + *
> 
> probably more appropriate as a /** @file ... */

... in contrast to literally every other header in every other decoder?

> 
>> + * Copyright (c) 2013-2014 Derek Buitenhuis
>> + *
> 
>> + * This file is part of Libav.
> 
> We tend to use another project name here.

Sorry. git am fail on my part. Fixed locally.

>> +static av_cold int libx265_encode_close(AVCodecContext *avctx)
>> +{
>> +    libx265Context *ctx = avctx->priv_data;
>> +
>> +    av_freep(&avctx->coded_frame);
> 
> av_frame_free()?

Fixed locally. I wrote the original patch before this existed.

>> +    if (ctx->params)
>> +        x265_param_free(ctx->params);
>> +
>> +    if (ctx->encoder)
>> +        x265_encoder_close(ctx->encoder);
>> +
> 
> I guess it's pointless to ask if those function are smart enough to deal
> with NULL pointers.

Better safe than sorry IMO.

>> +    if (avctx->width % ctx->params->maxCUSize) {
>> +        av_log(avctx, AV_LOG_ERROR,
>> +               "libx265 requires a width that is a multiple of %d.\n",
>> +               ctx->params->maxCUSize);
>> +        libx265_encode_close(avctx);
> 
>> +        return AVERROR_INVALIDDATA;
> 
> invalid data doesn't refer to invalid user input data, but invalid data
> stream. You probably want to use AVERROR(EINVAL). Same below.

I'm always confused by this (and the mix of AVERROR(...) and AVERROR_...).

>> +    ctx->encoder = x265_encoder_open(ctx->params);
>> +    if (!ctx->encoder) {
>> +        av_log(avctx, AV_LOG_ERROR, "Cannot open libx265 encoder.\n");
>> +        libx265_encode_close(avctx);
>> +        return AVERROR_INVALIDDATA;
> 
> I think you want AVERROR_EXTERNAL.

Ditto.

>> +    ctx->header = av_malloc(ctx->header_size);
>> +    if (!ctx->header) {
>> +        av_log(avctx, AV_LOG_ERROR, "Cannot allocate HEVC header.\n");
> 
> Since you're being pedantic about printing error message, you probably
> want to print the size here.

I'm not sure it's useful info, but OK.

>> +    memset(&x265pic_out, 0, sizeof(x265pic_out));
> 
> {0} raises random warnings?

Yes, it didn't like it when I tried.

>> +#define OFFSET(x) offsetof(libx265Context, x)
>> +#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
>> +static const AVOption options[] = {
>> +    { "preset",      "Set the x265 preset.",                                                        OFFSET(preset),    AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
>> +    { "tune",        "Set teh x265 tune parameter.",                                                OFFSET(tune),      AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
>> +    { "x265-params", "Set the x265 configuration using a :-separated list of key=value parameters", OFFSET(x265_opts), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
> 
> undo the initial cap and remove trailing '.' for current style.

libx264.c uses both caps and periods.

I fixed 'teh' btw.

>> +    { NULL },
> 
> trailing comma uneeded.

Other decoders use this style, so I did as well.

>> +    .long_name        = NULL_IF_CONFIG_SMALL("libx265 H.265 / HEVC"),
> 
> nit: probably belongs below .name; I think that's the current convention.

OK.

> Changelog entry welcome.

It's there already. It's the first change in the patch.

- Derek