On Tue, Feb 21, 2017 at 05:15:59PM +0000, Saverio Blasi wrote: [...] > enabled libspeex && require_pkg_config speex speex/speex.h > speex_decoder_init -lspeex > enabled libtesseract && require_pkg_config tesseract tesseract/capi.h > TessBaseAPICreate > enabled libtheora && require libtheora theora/theoraenc.h > th_info_init -ltheoraenc -ltheoradec -logg
> +enabled libturing && require_pkg_config libturing turing.h
> turing_version
You probably want to specify a minimal version
[...]
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA */
> +
This looks mangled.
> +#include <turing.h>
Please add a line break after this
> +#include "libavutil/internal.h"
> +#include "libavutil/common.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixdesc.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +#define MAX_OPTION_LENGTH 256
> +
> +typedef struct libturingEncodeContext {
> + const AVClass *class;
> + turing_encoder *encoder;
> + const char *options;
> +} libturingEncodeContext;
> +
> +typedef struct optionContext {
> + char** argv;
> + char* options;
> + char* s;
Style here and in other places: * stick to the var
> + int options_buffer_size;
> + int buffer_filled;
> + int options_added;
> +} optionContext;
> +
> +static av_cold int libturing_encode_close(AVCodecContext *avctx) {
Style: here and in other places missing line break before "{" for
functions.
> + libturingEncodeContext *ctx = avctx->priv_data;
> +
> + if (ctx->encoder) {
> + turing_destroy_encoder(ctx->encoder);
> + }
Note: the NULL check should probably be part of the libturing API to
simplify code paths for the users (just like the stdlib free(3)
convention).
> + return 0;
> +}
> +
> +static av_cold int add_option(const char* current_option,
> +optionContext* option_ctx) {
This function should be replaced with AVBPrint API. It will be much
simpler (that function will basically disappear) and will allow the caller
to check for errors only once.
[...]
> +static av_cold int libturing_encode_init(AVCodecContext *avctx) {
> + libturingEncodeContext *ctx = avctx->priv_data;
> + const int bit_depth = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth;
> + int error_code = 0;
> +
> + optionContext encoder_options;
use "encoder_options = {0}" so you don't need to fill each and every field
later on, and risk to forget one in the future.
[...]
> + int const illegal_option =
> + !strcmp("input-res", en->key) ||
> + !strcmp("frame-rate", en->key) ||
> + !strcmp("f", en->key) ||
> + !strcmp("frames", en->key) ||
> + !strcmp("sar", en->key) ||
> + !strcmp("bit-depth", en->key) ||
> + !strcmp("internal-bit-depth", en->key);
you could use av_match_name(en->key, "input-res,frame-rate,f,...") here.
> + if (illegal_option) {
> + av_log(avctx, AV_LOG_WARNING, "%s=%s ignored - this
> parameter is inferred from ffmpeg.\n", en->key, en->value);
> + } else {
> + if (turing_check_binary_option(en->key)) {
> + snprintf(option_string, MAX_OPTION_LENGTH, "--%s",
> en->key);
> + } else {
> + snprintf(option_string, MAX_OPTION_LENGTH,
> "--%s=%s", en->key, en->value);
> + }
> + if ((error_code = add_option(option_string,
> &encoder_options)) > 0) {
> + goto fail;
leaking dict here.
> + }
> + }
> + }
> + av_dict_free(&dict);
> + }
> + }
> +
> + if ((error_code = add_option("dummy-input-filename", &encoder_options))
> > 0) {
> + goto fail;
> + }
> +
> + if ((error_code = finalise_options(&encoder_options)) > 0) {
> + goto fail;
> + }
> +
> + settings.argv = (char const**)encoder_options.argv;
> + settings.argc = encoder_options.options_added;
> +
> + for (int i=0; i<settings.argc; ++i) {
- int is not allowed here
- the style is broken (missing spaces)
- ++i is a c++ idiom and doesn't belong here
> + av_log(avctx, AV_LOG_VERBOSE, "arg %d: %s\n", i, settings.argv[i]);
> + }
> +
> + ctx->encoder = turing_create_encoder(settings);
> +
> + if (!ctx->encoder) {
> + av_log(avctx, AV_LOG_ERROR, "Failed to create libturing encoder.\n");
> + error_code = AVERROR_INVALIDDATA;
> + goto fail;
> + }
> +
> + if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> + turing_bitstream const *bitstream;
> + bitstream = turing_encode_headers(ctx->encoder);
> + if (bitstream->size <= 0) {
> + av_log(avctx, AV_LOG_ERROR, "Failed to encode headers.\n");
> + turing_destroy_encoder(ctx->encoder);
> + error_code = AVERROR_INVALIDDATA;
> + goto fail;
> + }
> +
> + avctx->extradata_size = bitstream->size;
> +
> + avctx->extradata = av_mallocz(avctx->extradata_size +
> AV_INPUT_BUFFER_PADDING_SIZE);
> + if (!avctx->extradata) {
> + av_log(avctx, AV_LOG_ERROR, "Failed to allocate HEVC extradata
> %d bytes\n", avctx->extradata_size);
> + turing_destroy_encoder(ctx->encoder);
> + error_code = AVERROR(ENOMEM);
> + goto fail;
> + }
> +
> + memcpy(avctx->extradata, bitstream->p, bitstream->size);
> + }
> +
> + av_freep(&encoder_options.argv);
> + av_freep(&encoder_options.options);
> + return 0;
> +
> +fail:
> + av_log(avctx, AV_LOG_ERROR, "Error while initialising the Turing
> codec.\n");
This message is useless as it won't provide any additional information.
Dropping that log allows you to replace the "fail" label with an "out"
label and drop the 3 lines above your current "fail" label.
> + av_freep(&encoder_options.argv);
> + av_freep(&encoder_options.options);
> + return error_code;
> +}
> +
[...]
> + ret = ff_alloc_packet2(avctx, pkt, output->bitstream.size, 0);
> + if (ret < 0) {
> + av_log(avctx, AV_LOG_ERROR, "Error getting output packet.\n");
Useless log, please drop it
> + return ret;
> + }
> +
> + memcpy(pkt->data, output->bitstream.p, output->bitstream.size);
> +
> + pkt->pts = output->pts;
> + pkt->dts = output->dts;
> + if (output->keyframe) {
> + pkt->flags |= AV_PKT_FLAG_KEY;
> + }
> +
> + *got_packet = 1;
> + return 0;
> +}
> +
> +static const AVOption options[] = {
> + { "turing-params", "configure additional turing encoder parameters",
> offsetof(libturingEncodeContext, options), AV_OPT_TYPE_STRING,{ 0 }, 0, 0,
> AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },
"{ 0 }, 0, 0" is ugly; drop them and use a named ".flags = AV_OPT..."
[...]
Regards,
--
Clément B.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
