[FFmpeg-devel] [PATCH] Add libx265 encoder

[Clément Bœsch]

On Tue, Feb 11, 2014 at 07:22:48PM +0000, Derek Buitenhuis wrote:
> 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.
> 

That was recently added for vid.stab, and it's kind of handy.

> >> +/*
> >> + * libx265 encoder
> >> + *
> > 
> > probably more appropriate as a /** @file ... */
> 
> ... in contrast to literally every other header in every other decoder?
> 

I guess I'm confused with how it's done in other libraries.

[...]
> >> +    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_...).
> 

EINVAL          Invalid argument (POSIX.1)

AVERROR(...) is for existing errno, AVERROR_ is for what's never defined
(I don't have "EOF" in my errno man here for instance).

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

libavutil/error.c:    { ERROR_TAG(EXTERNAL),           "Generic error in an external library"           },


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

The only reason this call would fail is because of an random header_size
like a negative or 0 one.

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

Yes but it's old; we came up with that convention maybe 2-3 years ago,
Stefano knows better I suppose.

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140211/7aa2902f/attachment.asc>