Re: [FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Matteo Naccari]

> > I still have an issue with this patch, while it would be nice to have 
> > another
> HEVC encoder available in FFmpeg for comparisons and whatnot, I feel that
> turingcodec isn't a mature enough encoder compared to what is already in
> the project.
> >
> > Not to mention that development seems to have stalled (with only very
> minor fixes for several months). In FFmpeg it is very difficult to remove
> 'features', so if this patch were to be merged and then turingcodec to
> actually go stale we'd be left with dead code for longer than is ideal 
> (forever).
> >
> > My opinion is that you should just maintain this separately as an out-of-
> tree patch as it doesn't benefit us.
> >
> 
> Hi,
> 
> After taking a look at the commit history of the last year+ or so, issue 
> tracker
> and pull request list over at https://github.com/bbc/turingcodec I must say I
> find it somewhat hard to disagree on a general level.
> 
> Creating an encoder is great work and I applaud people for that, but
> unfortunately it - at the current point of time - looks like neither from the
> licensing, performance or project activity point of view merging a wrapper for
> this library is a good idea.

If stability of the project was your concern, you should have told us from the 
very beginning and save everyone's time. The project and its development is 
still active. We pushed additional fixes in November and we plan to add new 
features in the near future. We acknowledge that there is still a lot to do to 
get the encoder to the same level of maturity of  others both from the 
performance and the licensing/collaboration point of view. At the same time, we 
also trust you appreciate that being a small team of researchers (3) working 
part time on this project we are slow in replying to requests and progressing 
with the development. Indeed we thought having the codec interfaced to FFmpeg 
would have been a good opportunity to share our work and know-how on content 
distribution for broadcasting applications using HEVC and provide an 
alternative codec to ffmpeg. Clearly we were wrong.
We'll distribute the patch onto our website for those who are interested in 
having Turing in FFmpeg and stop pestering you with new requests.

---
Matteo Naccari, Ph.D.
Senior Technologist, BBC R
Block D, Centre House
56 Wood lane, W12 7SB, London (UK)
Phone: +44 (0)303 0409640
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Jan Ekström]

On Thu, Dec 14, 2017 at 12:39 AM, Josh de Kock  wrote:
> On Thu,  7 Dec 2017 11:08:47 +
> Matteo Naccari  wrote:
>
>> - This patch contains the changes required to interface the Turing codec
>> [...]
>
> I still have an issue with this patch, while it would be nice to have another 
> HEVC encoder available in FFmpeg for comparisons and whatnot, I feel that 
> turingcodec isn't a mature enough encoder compared to what is already in the 
> project.
>
> Not to mention that development seems to have stalled (with only very minor 
> fixes for several months). In FFmpeg it is very difficult to remove 
> 'features', so if this patch were to be merged and then turingcodec to 
> actually go stale we'd be left with dead code for longer than is ideal 
> (forever).
>
> My opinion is that you should just maintain this separately as an out-of-tree 
> patch as it doesn't benefit us.
>

Hi,

After taking a look at the commit history of the last year+ or so,
issue tracker and pull request list over at
https://github.com/bbc/turingcodec I must say I find it somewhat hard
to disagree on a general level.

Creating an encoder is great work and I applaud people for that, but
unfortunately it - at the current point of time - looks like neither
from the licensing, performance or project activity point of view
merging a wrapper for this library is a good idea.


Best regards,
Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Josh de Kock]

On Thu,  7 Dec 2017 11:08:47 +
Matteo Naccari  wrote:

> - This patch contains the changes required to interface the Turing codec
> [...]

I still have an issue with this patch, while it would be nice to have another 
HEVC encoder available in FFmpeg for comparisons and whatnot, I feel that 
turingcodec isn't a mature enough encoder compared to what is already in the 
project.

Not to mention that development seems to have stalled (with only very minor 
fixes for several months). In FFmpeg it is very difficult to remove 'features', 
so if this patch were to be merged and then turingcodec to actually go stale 
we'd be left with dead code for longer than is ideal (forever).

My opinion is that you should just maintain this separately as an out-of-tree 
patch as it doesn't benefit us.

-- 
Josh de Kock 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Matteo Naccari]

- This patch contains the changes required to interface the Turing codec
  (http://turingcodec.org/) to ffmpeg
---
 Changelog  |   1 +
 LICENSE.md |   1 +
 configure  |   6 +
 libavcodec/Makefile|   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/libturing.c | 338 +
 6 files changed, 348 insertions(+)
 create mode 100644 libavcodec/libturing.c

diff --git a/Changelog b/Changelog
index a1a7557..9ed18b5 100644
--- a/Changelog
+++ b/Changelog
@@ -25,6 +25,7 @@ version :
 - AMD AMF H.264 and HEVC encoders
 - video fillborders filter
 - video setrange filter
+- Added support to interface with the Turing codec
 
 
 version 3.4:
diff --git a/LICENSE.md b/LICENSE.md
index ba65b05..03787c0 100644
--- a/LICENSE.md
+++ b/LICENSE.md
@@ -84,6 +84,7 @@ The following libraries are under GPL:
 - frei0r
 - libcdio
 - librubberband
+- libturing
 - libvidstab
 - libx264
 - libx265
diff --git a/configure b/configure
index d053886..9be1fcb 100755
--- a/configure
+++ b/configure
@@ -260,6 +260,7 @@ External library support:
   --enable-libssh  enable SFTP protocol via libssh [no]
   --enable-libtesseractenable Tesseract, needed for ocr filter [no]
   --enable-libtheora   enable Theora encoding via libtheora [no]
+  --enable-libturing   enable H.265/HEVC encoding via libturing [no]
   --enable-libtwolame  enable MP2 encoding via libtwolame [no]
   --enable-libv4l2 enable libv4l2/v4l-utils [no]
   --enable-libvidstab  enable video stabilization using vid.stab [no]
@@ -1551,6 +1552,7 @@ EXTERNAL_LIBRARY_GPL_LIST="
 frei0r
 libcdio
 librubberband
+libturing
 libvidstab
 libx264
 libx265
@@ -2963,6 +2965,7 @@ libspeex_decoder_deps="libspeex"
 libspeex_encoder_deps="libspeex"
 libspeex_encoder_select="audio_frame_queue"
 libtheora_encoder_deps="libtheora"
+libturing_encoder_deps="libturing"
 libtwolame_encoder_deps="libtwolame"
 libvo_amrwbenc_encoder_deps="libvo_amrwbenc"
 libvorbis_decoder_deps="libvorbis"
@@ -5889,6 +5892,9 @@ enabled libssh&& require_pkg_config libssh 
libssh libssh/sftp.h sftp
 enabled libspeex  && require_pkg_config libspeex speex speex/speex.h 
speex_decoder_init
 enabled libtesseract  && require_pkg_config libtesseract tesseract 
tesseract/capi.h TessBaseAPICreate
 enabled libtheora && require libtheora theora/theoraenc.h th_info_init 
-ltheoraenc -ltheoradec -logg
+enabled libturing && require_pkg_config libturing libturing turing.h 
turing_version &&
+ { check_cpp_condition turing.h 
"TURING_API_VERSION > 1" ||
+   die "ERROR: libturing requires turing api 
version 2 or greater."; }
 enabled libtwolame&& require libtwolame twolame.h twolame_init 
-ltwolame &&
  { check_lib libtwolame twolame.h 
twolame_encode_buffer_float32_interleaved -ltwolame ||
die "ERROR: libtwolame must be installed and 
version must be >= 0.3.10"; }
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index ab7893f..1c8d945 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -952,6 +952,7 @@ OBJS-$(CONFIG_LIBSHINE_ENCODER)   += libshine.o
 OBJS-$(CONFIG_LIBSPEEX_DECODER)   += libspeexdec.o
 OBJS-$(CONFIG_LIBSPEEX_ENCODER)   += libspeexenc.o
 OBJS-$(CONFIG_LIBTHEORA_ENCODER)  += libtheoraenc.o
+OBJS-$(CONFIG_LIBTURING_ENCODER)  += libturing.o
 OBJS-$(CONFIG_LIBTWOLAME_ENCODER) += libtwolame.o
 OBJS-$(CONFIG_LIBVO_AMRWBENC_ENCODER) += libvo-amrwbenc.o
 OBJS-$(CONFIG_LIBVORBIS_DECODER)  += libvorbisdec.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index ed1e7ab..b42630d 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -551,6 +551,7 @@ static void register_all(void)
 REGISTER_ENCODER(LIBSHINE,  libshine);
 REGISTER_ENCDEC (LIBSPEEX,  libspeex);
 REGISTER_ENCODER(LIBTHEORA, libtheora);
+REGISTER_ENCODER(LIBTURING, libturing);
 REGISTER_ENCODER(LIBTWOLAME,libtwolame);
 REGISTER_ENCODER(LIBVO_AMRWBENC,libvo_amrwbenc);
 REGISTER_ENCDEC (LIBVORBIS, libvorbis);
diff --git a/libavcodec/libturing.c b/libavcodec/libturing.c
new file mode 100644
index 000..ac64797
--- /dev/null
+++ b/libavcodec/libturing.c
@@ -0,0 +1,338 @@
+/*
+ * libturing encoder
+ *
+ * Copyright (c) 2017 Turing Codec contributors
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or 

Re: [FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Derek Buitenhuis]

On 11/20/2017 10:35 AM, Matteo Naccari wrote:
> +enabled libturing && require_pkg_config libturing libturing turing.h 
> turing_version &&
> + { check_cpp_condition turing.h 
> "TURING_API_VERSION > 1" ||
> +   die "ERROR: libturing requires turing api 
> version 2 or greater."; }

Why exactly is libturing's API version not correlated with its soversion?

> +#define MAX_OPTION_LENGTH 256

I probably asked this in a previous iteration, but why is there an
arbitrary limit?

> +if (!(option_ctx->options)) {
> +option_ctx->options = av_malloc(option_length + 1);
> +if (!(option_ctx->options)) {
> +return AVERROR(ENOMEM);
> +}
> +} else {
> +temp_ptr = av_realloc(option_ctx->options, 
> option_ctx->options_buffer_size + option_length + 1);
> +if (!(temp_ptr)) {
> +return AVERROR(ENOMEM);
> +}
The if/else is redundant, since av_realloc can take a NULL pointer.

> +
> +snprintf(option_string, MAX_OPTION_LENGTH, "--input-res=%dx%d", 
> avctx->width, avctx->height);
> +if (error_code = add_option(option_string, _options)) {
> +goto fail;
> +}

Since the return value for snprintf is not checked, won't this lead to mangled 
arguments if
its truncated? Same for below
> +
> +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);

Shouldn't calls to turing_destroy_encoder (or preferably 
libturing_encode_close) be
handled at the fail label rather than all over the place?

> +av_freep(_options.argv);
> +av_freep(_options.options);
> +return 0;
> +
> +fail:
> +av_log(avctx, AV_LOG_ERROR, "Error while initialising the Turing 
> codec.\n");

Redundant generic log.

> +ret = ff_alloc_packet2(avctx, pkt, output->bitstream.size, 0);
> +if (ret < 0) {
> +av_log(avctx, AV_LOG_ERROR, "Error getting output packet.\n");
> +return ret;
> +}
> +
> +memcpy(pkt->data, output->bitstream.p, output->bitstream.size);

Is there no way to support AV_CODEC_FLAG_GLOBAL_HEADER directly (see libx265.c)?

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Moritz Barsnick]

On Mon, Nov 20, 2017 at 10:35:39 +, Matteo Naccari wrote:

I don't recall all of the previous review remarks, but these may be
new:

>  LICENSE.md |   1 +
>  configure  |   6 +

Please add a changelog entry.

>  librubberband
> + libturing
>  libvidstab

You used a tab instead spaces.

> + #if defined(_MSC_VER)
> +#define TURING_API_IMPORTS 1
> +#endif

Stray whitespace in front of "#if".

> +int error_code = 0;
> +int i = 0;

I believe these initializations are never used.

> +if (error_code = add_option("turing", _options)) {
> +goto fail;
> +}

This assignment in an if() clause will give a compiler warning if you
don't add an extra set of brackets around the assingment.

Preferred method is actually:
   error_code = add_option("turing", _options);
   if (error_code) {
   goto fail;
   }

> +if (error_code = add_option("--frames=0", _options)) {
> +goto fail;
> +}

Same here, and further occurences below.

> +error_code =  AVERROR(ENOMEM);

Stray whitespace.

> +int ret = 0;

Unused assignment.

> +av_strlcpy(option_ctx->s, current_option, (option_length + 1));
> +option_ctx->s += 1 + option_length;
> +option_ctx->options_added++;
> +option_ctx->buffer_filled += option_length + 1;

It's a bit confusing to read, if you use "option_length + 1" twice, and
"1 + option_length" the third time.

> +static const AVOption options[] = {
> +{ "turing-params", "configure additional turing encoder parameters", 
> offsetof(libturingEncodeContext, options), AV_OPT_TYPE_STRING,{ .str = NULL 
> }, 0, 0, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },

IMHO you can drop the word "configure ".

Cheers,
Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] Added Turing codec to ffmpeg

[Matteo Naccari]

This patch contains the changes required to interface the Turing codec
(http://turingcodec.org/) to ffmpeg
---
 LICENSE.md |   1 +
 configure  |   6 +
 libavcodec/Makefile|   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/libturing.c | 318 +
 5 files changed, 327 insertions(+)
 create mode 100644 libavcodec/libturing.c

diff --git a/LICENSE.md b/LICENSE.md
index ba65b05..03787c0 100644
--- a/LICENSE.md
+++ b/LICENSE.md
@@ -84,6 +84,7 @@ The following libraries are under GPL:
 - frei0r
 - libcdio
 - librubberband
+- libturing
 - libvidstab
 - libx264
 - libx265
diff --git a/configure b/configure
index 8262358..566fa6e 100755
--- a/configure
+++ b/configure
@@ -260,6 +260,7 @@ External library support:
   --enable-libssh  enable SFTP protocol via libssh [no]
   --enable-libtesseractenable Tesseract, needed for ocr filter [no]
   --enable-libtheora   enable Theora encoding via libtheora [no]
+  --enable-libturing   enable H.265/HEVC encoding via libturing [no]
   --enable-libtwolame  enable MP2 encoding via libtwolame [no]
   --enable-libv4l2 enable libv4l2/v4l-utils [no]
   --enable-libvidstab  enable video stabilization using vid.stab [no]
@@ -1548,6 +1549,7 @@ EXTERNAL_LIBRARY_GPL_LIST="
 frei0r
 libcdio
 librubberband
+   libturing
 libvidstab
 libx264
 libx265
@@ -2965,6 +2967,7 @@ libspeex_decoder_deps="libspeex"
 libspeex_encoder_deps="libspeex"
 libspeex_encoder_select="audio_frame_queue"
 libtheora_encoder_deps="libtheora"
+libturing_encoder_deps="libturing"
 libtwolame_encoder_deps="libtwolame"
 libvo_amrwbenc_encoder_deps="libvo_amrwbenc"
 libvorbis_decoder_deps="libvorbis"
@@ -5891,6 +5894,9 @@ enabled libssh&& require_pkg_config libssh 
libssh libssh/sftp.h sftp
 enabled libspeex  && require_pkg_config libspeex speex speex/speex.h 
speex_decoder_init
 enabled libtesseract  && require_pkg_config libtesseract tesseract 
tesseract/capi.h TessBaseAPICreate
 enabled libtheora && require libtheora theora/theoraenc.h th_info_init 
-ltheoraenc -ltheoradec -logg
+enabled libturing && require_pkg_config libturing libturing turing.h 
turing_version &&
+ { check_cpp_condition turing.h 
"TURING_API_VERSION > 1" ||
+   die "ERROR: libturing requires turing api 
version 2 or greater."; }
 enabled libtwolame&& require libtwolame twolame.h twolame_init 
-ltwolame &&
  { check_lib libtwolame twolame.h 
twolame_encode_buffer_float32_interleaved -ltwolame ||
die "ERROR: libtwolame must be installed and 
version must be >= 0.3.10"; }
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 494c76d..d94cce7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -945,6 +945,7 @@ OBJS-$(CONFIG_LIBSHINE_ENCODER)   += libshine.o
 OBJS-$(CONFIG_LIBSPEEX_DECODER)   += libspeexdec.o
 OBJS-$(CONFIG_LIBSPEEX_ENCODER)   += libspeexenc.o
 OBJS-$(CONFIG_LIBTHEORA_ENCODER)  += libtheoraenc.o
+OBJS-$(CONFIG_LIBTURING_ENCODER)  += libturing.o
 OBJS-$(CONFIG_LIBTWOLAME_ENCODER) += libtwolame.o
 OBJS-$(CONFIG_LIBVO_AMRWBENC_ENCODER) += libvo-amrwbenc.o
 OBJS-$(CONFIG_LIBVORBIS_DECODER)  += libvorbisdec.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index e0adb71..b7a3493 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -631,6 +631,7 @@ static void register_all(void)
 REGISTER_ENCODER(LIBSHINE,  libshine);
 REGISTER_ENCDEC (LIBSPEEX,  libspeex);
 REGISTER_ENCODER(LIBTHEORA, libtheora);
+REGISTER_ENCODER(LIBTURING, libturing);
 REGISTER_ENCODER(LIBTWOLAME,libtwolame);
 REGISTER_ENCODER(LIBVO_AMRWBENC,libvo_amrwbenc);
 REGISTER_ENCDEC (LIBVORBIS, libvorbis);
diff --git a/libavcodec/libturing.c b/libavcodec/libturing.c
new file mode 100644
index 000..77688ea
--- /dev/null
+++ b/libavcodec/libturing.c
@@ -0,0 +1,318 @@
+/*
+ * libturing encoder
+ *
+ * Copyright (c) 2017 Turing Codec contributors
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA