[FFmpeg-devel] [PATCH] Add libx265 encoder
Clément Bœsch
u at pkh.me
Tue Feb 11 20:38:39 CET 2014
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>
More information about the ffmpeg-devel
mailing list