[FFmpeg-devel,WIP] libcodec2 wrapper + de/muxer in FFmpeg

Hi

Posting this to both [ffmpeg-devel] and [Freetel-codec2] in hopes of
maximum feedback.

I've been spending the last few days getting codec2 (http://freedv.org/
) hooked up in libavcodec, and a set of muxers and demuxers for both
raw codec2 streams and the recently created .c2 format. codec2 is very
low bitrate (3200 bit/s down to 700 bit/s) speech codec developed for
digital voice in amateur radio, but is now finding use in other
applications where compressing large amounts of human speech is useful
(audiobooks or podcasts for example)

Sample: http://www.nivex.net/images/tmp/report2074.c2

With both the raw demuxer and the encoder you need to specify the
desired mode, like this:

  # Transcode .wav to .c2
  ffmpeg -i foo.wav -mode 700C out.c2

  # Transcode raw codec2 stream to .wav
  ffmpeg -mode 700C -i bar.raw out.wav

Decoding a .c2 file is straightforward however, thanks to the header:

  ffmpeg -i report2074.c2 out.wav

There's one issue currently which I suspect is due to timestamping,
where the demuxer seems to lose track of the block_align grid:

  $ ./ffmpeg -i report2074.c2 /tmp/out.wav
  ...
  Output #0, wav, to '/tmp/out.wav':
    Metadata:
      ISFT            : Lavf57.77.100
      Stream #0:0: Audio: pcm_s16le ([1][0][0][0] / 0x0001), 8000 Hz,
mono, s16, 128 kb/s
      Metadata:
        encoder         : Lavc57.103.100 pcm_s16le
  [libcodec2 @ 0x561d7bc4d1e0] Multiple frames in a packet
  [libcodec2 @ 0x561d7bc4d1e0] get_buffer() failed
  Error while decoding stream #0:0: Invalid argument
  [libcodec2 @ 0x561d7bc4d1e0] get_buffer() failed
  Error while decoding stream #0:0: Invalid argument
  [libcodec2 @ 0x7f59c0015840] Multiple frames in a packet.
  [libcodec2 @ 0x7f59c0015840] get_buffer() failed

I plan on looking into this once I've had some sleep.

Some remarks:

* I had to make the ffmpeg CLI not complain about the 700 modes, since
it thinks encoding at below 1 kbps is a user error

* I have duplicated some minor functions in libcodec2 in
libavcodec/codec2utils.*. This avoid having to link libcodec2 just for
remuxing, and in case someone writes a native decoder for libavcodec

* Not sure if codec2utils should go in libavutil, libavcodec felt good
enough

* The demuxer tries to read up to 0.1 seconds worth of frames to speed
things up a little without making seeking too broken. 3 frames = 12
bytes for the 700 bit/s modes, which decode to 960 samples

* The decoder is able to deal with multiple frames at a time, the
encoder does not need to

* The .c2 format is still fairly experimental, but the version bytes
should be enough to deal with any potential future variants 

Feel free to bikeshed around whether extradata should be the entire .c2
header or just the mode byte. It really only matters if we go with RIFF
or ISOM mappings (.wav .avi .mov and friends), which I decided to leave
out for now.

/Tomas

2017-08-03 0:40 GMT+02:00 Tomas Härdin <tjoppen@acc.umu.se>:

> Decoding a .c2 file is straightforward however, thanks to the header:
>
>   ffmpeg -i report2074.c2 out.wav

The probe score is too high.
I suspect you should also check for the major version,
the score should be MAX/2 if the first 32bit are ok,
significantly less for 24bit.

Carl Eugen

On Thu, 2017-08-03 at 01:07 +0200, Carl Eugen Hoyos wrote:
> 2017-08-03 0:40 GMT+02:00 Tomas Härdin <tjoppen@acc.umu.se>:
> 
> > 
> > Decoding a .c2 file is straightforward however, thanks to the
> > header:
> > 
> >   ffmpeg -i report2074.c2 out.wav
> The probe score is too high.
> I suspect you should also check for the major version,
> the score should be MAX/2 if the first 32bit are ok,
> significantly less for 24bit.

OK, didn't know that. Major being bumped would imply a new API, and
we're not likely to link two versions of the same library. I suspect it
would primarily impact the decoder, not the demuxer. But checking major
== 0 makes sense for now. libcodec2 does not yet expose version in its
headers, but should soon since I pointed that out to them :)

/Tomas

Le sextidi 16 thermidor, an CCXXV, Tomas Härdin a écrit :
> Posting this to both [ffmpeg-devel] and [Freetel-codec2] in hopes of
> maximum feedback.
> 
> I've been spending the last few days getting codec2 (http://freedv.org/
> ) hooked up in libavcodec, and a set of muxers and demuxers for both
> raw codec2 streams and the recently created .c2 format. codec2 is very
> low bitrate (3200 bit/s down to 700 bit/s) speech codec developed for
> digital voice in amateur radio, but is now finding use in other
> applications where compressing large amounts of human speech is useful
> (audiobooks or podcasts for example)
> 
> Sample: http://www.nivex.net/images/tmp/report2074.c2
> 
> With both the raw demuxer and the encoder you need to specify the
> desired mode, like this:

The encoder could default to one the two.

> Some remarks:
> 
> * I had to make the ffmpeg CLI not complain about the 700 modes, since
> it thinks encoding at below 1 kbps is a user error

It is just a warning. I am not really a fan of a special case like that,
and it would be better split into a separate patch I think.

> * I have duplicated some minor functions in libcodec2 in
> libavcodec/codec2utils.*. This avoid having to link libcodec2 just for
> remuxing, and in case someone writes a native decoder for libavcodec

The license allows it, but you neglected to give copyright attribution.

> * Not sure if codec2utils should go in libavutil, libavcodec felt good
> enough

Definitely libavcodec.

> * The demuxer tries to read up to 0.1 seconds worth of frames to speed
> things up a little without making seeking too broken. 3 frames = 12
> bytes for the 700 bit/s modes, which decode to 960 samples

I do not like the sound of that, but I will see the code.

> * The decoder is able to deal with multiple frames at a time, the
> encoder does not need to

???

> Feel free to bikeshed around whether extradata should be the entire .c2
> header or just the mode byte. It really only matters if we go with RIFF
> or ISOM mappings (.wav .avi .mov and friends), which I decided to leave
> out for now.

It matters for inclusion in any format: Matroska, Nut, etc. Is anybody
considering normalization?

> From 569a252536ea224bcd44f55f0f5102ce1aa4ec77 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
> Date: Wed, 2 Aug 2017 22:17:19 +0200
> Subject: [PATCH] Add codec2 muxers, demuxers and en/decoder via libcodec2
> 
> ---
>  Changelog                |   2 +
>  configure                |  12 +++
>  doc/general.texi         |  13 +++
>  ffmpeg.c                 |   3 +-
>  libavcodec/Makefile      |   3 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/avcodec.h     |   1 +
>  libavcodec/codec2utils.c | 118 +++++++++++++++++++++++
>  libavcodec/codec2utils.h |  58 +++++++++++
>  libavcodec/codec_desc.c  |   7 ++
>  libavcodec/libcodec2.c   | 190 ++++++++++++++++++++++++++++++++++++
>  libavcodec/version.h     |   2 +-
>  libavformat/Makefile     |   4 +
>  libavformat/allformats.c |   2 +
>  libavformat/codec2.c     | 244 +++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/rawenc.c     |  14 +++
>  libavformat/utils.c      |   1 +
>  libavformat/version.h    |   2 +-
>  18 files changed, 674 insertions(+), 3 deletions(-)
>  create mode 100644 libavcodec/codec2utils.c
>  create mode 100644 libavcodec/codec2utils.h
>  create mode 100644 libavcodec/libcodec2.c
>  create mode 100644 libavformat/codec2.c
> 
> diff --git a/Changelog b/Changelog
> index 187ae7950a..e28da7dcc4 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -29,6 +29,8 @@ version <next>:
>  - limiter video filter
>  - libvmaf video filter
>  - Dolby E decoder and SMPTE 337M demuxer
> +- codec2 en/decoding via libcodec2
> +- muxer/demuxer for raw codec2 files and .c2 files
>  
>  version 3.3:
>  - CrystalHD decoder moved to new decode API
> diff --git a/configure b/configure
> index 66c7b948e4..05af25cb22 100755
> --- a/configure
> +++ b/configure
> @@ -220,6 +220,7 @@ External library support:
>    --enable-libcaca         enable textual display using libcaca [no]
>    --enable-libcelt         enable CELT decoding via libcelt [no]
>    --enable-libcdio         enable audio CD grabbing with libcdio [no]
> +  --enable-libcodec2       enable codec2 en/decoding using libcodec2 [no]
>    --enable-libdc1394       enable IIDC-1394 grabbing using libdc1394
>                             and libraw1394 [no]
>    --enable-libfdk-aac      enable AAC de/encoding via libfdk-aac [no]
> @@ -1540,6 +1541,7 @@ EXTERNAL_LIBRARY_LIST="
>      libbs2b
>      libcaca
>      libcelt
> +    libcodec2
>      libdc1394
>      libflite
>      libfontconfig
> @@ -2087,6 +2089,7 @@ CONFIG_EXTRA="
>      blockdsp
>      bswapdsp
>      cabac
> +    codec2utils
>      dirac_parse
>      dvprofile
>      exif
> @@ -2863,6 +2866,10 @@ pcm_mulaw_at_encoder_select="audio_frame_queue"
>  chromaprint_muxer_deps="chromaprint"
>  h264_videotoolbox_encoder_deps="videotoolbox_encoder pthreads"
>  libcelt_decoder_deps="libcelt"

> +libcodec2_decoder_deps="libcodec2"
> +libcodec2_decoder_select="codec2utils"
> +libcodec2_encoder_deps="libcodec2"
> +libcodec2_encoder_select="codec2utils"

This and the similar hunks are not necessary, see below the comments
about the Makefile.

>  libfdk_aac_decoder_deps="libfdk_aac"
>  libfdk_aac_encoder_deps="libfdk_aac"
>  libfdk_aac_encoder_select="audio_frame_queue"
> @@ -2935,6 +2942,10 @@ avi_demuxer_select="iso_media riffdec exif"
>  avi_muxer_select="riffenc"
>  caf_demuxer_select="iso_media riffdec"
>  caf_muxer_select="iso_media"
> +codec2_demuxer_select="codec2utils"
> +codec2_muxer_select="codec2utils"
> +codec2raw_demuxer_select="codec2utils"
> +codec2raw_muxer_select="codec2utils"
>  dash_muxer_select="mp4_muxer"
>  dirac_demuxer_select="dirac_parser"
>  dts_demuxer_select="dca_parser"
> @@ -5837,6 +5848,7 @@ enabled libcelt           && require libcelt celt/celt.h celt_decode -lcelt0 &&
>                               { check_lib libcelt celt/celt.h celt_decoder_create_custom -lcelt0 ||
>                                 die "ERROR: libcelt must be installed and version must be >= 0.11.0."; }
>  enabled libcaca           && require_pkg_config caca caca.h caca_create_canvas
> +enabled libcodec2         && require libcodec2 codec2/codec2.h codec2_create -lcodec2
>  enabled libdc1394         && require_pkg_config libdc1394-2 dc1394/dc1394.h dc1394_new
>  enabled libfdk_aac        && { use_pkg_config fdk-aac "fdk-aac/aacenc_lib.h" aacEncOpen ||
>                                 { require libfdk_aac fdk-aac/aacenc_lib.h aacEncOpen -lfdk-aac &&
> diff --git a/doc/general.texi b/doc/general.texi
> index 036c8c25d4..4bcc2b2d91 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -85,6 +85,15 @@ Go to @url{http://www.twolame.org/} and follow the
>  instructions for installing the library.
>  Then pass @code{--enable-libtwolame} to configure to enable it.
>  
> +@section libcodec2
> +
> +FFmpeg can make use of libcodec2 to codec2 encoding and decoding.
> +There is currently no native decoder, so libcodec2 must be used for decoding.
> +
> +Go to @url{http://freedv.org/}, download "Codec 2 source archive".
> +Build and install using CMake. Debian users can install the libcodec2-dev package instead.
> +Once libcodec2 is installed you can pass @code{--enable-libcodec2} to configure to enable it.
> +
>  @section libvpx
>  
>  FFmpeg can make use of the libvpx library for VP8/VP9 encoding.
> @@ -290,6 +299,8 @@ library:
>  @item BRSTM                     @tab   @tab X
>      @tab Audio format used on the Nintendo Wii.
>  @item BWF                       @tab X @tab X
> +@item codec2 (raw)              @tab X @tab X
> +@item codec2 (.c2 files)        @tab X @tab X
>  @item CRI ADX                   @tab X @tab X
>      @tab Audio-only format used in console video games.
>  @item Discworld II BMV          @tab   @tab X
> @@ -994,6 +1005,8 @@ following image formats are supported:
>      @tab Used in Bink and Smacker files in many games.
>  @item CELT                   @tab     @tab  E
>      @tab decoding supported through external library libcelt
> +@item codec2                 @tab  E  @tab  E
> +    @tab en/decoding supported through external library libcodec2
>  @item Delphine Software International CIN audio  @tab     @tab  X
>      @tab Codec used in Delphine Software International games.
>  @item Digital Speech Standard - Standard Play mode (DSS SP) @tab     @tab  X
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 888d19a647..09a5b541c0 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -3480,7 +3480,8 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
>              av_buffersink_set_frame_size(ost->filter->filter,
>                                              ost->enc_ctx->frame_size);
>          assert_avoptions(ost->encoder_opts);
> -        if (ost->enc_ctx->bit_rate && ost->enc_ctx->bit_rate < 1000)
> +        if (ost->enc_ctx->bit_rate && ost->enc_ctx->bit_rate < 1000 &&
> +            ost->enc_ctx->codec_id != AV_CODEC_ID_CODEC2 /* don't complain about 700 bit/s modes */)
>              av_log(NULL, AV_LOG_WARNING, "The bitrate parameter is set too low."
>                                           " It takes bits/s as argument, not kbits/s\n");
>  
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 74de41ab0f..f5531ab3f1 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -59,6 +59,7 @@ OBJS-$(CONFIG_AUDIODSP)                += audiodsp.o
>  OBJS-$(CONFIG_BLOCKDSP)                += blockdsp.o
>  OBJS-$(CONFIG_BSWAPDSP)                += bswapdsp.o
>  OBJS-$(CONFIG_CABAC)                   += cabac.o

> +OBJS-$(CONFIG_CODEC2UTILS)             += codec2utils.o

You do not need a separate configuration option, you can just depend on
the actual visible option:

+OBJS-$(CONFIG_CODEC2_DEMUXER)           += codec2utils.o
+OBJS-$(CONFIG_CODEC2_MUXER)             += codec2utils.o

Having the same file several times will not cause it to be included
several times.

>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
>  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
> @@ -885,6 +886,8 @@ OBJS-$(CONFIG_ILBC_AT_ENCODER)            += audiotoolboxenc.o
>  OBJS-$(CONFIG_PCM_ALAW_AT_ENCODER)        += audiotoolboxenc.o
>  OBJS-$(CONFIG_PCM_MULAW_AT_ENCODER)       += audiotoolboxenc.o
>  OBJS-$(CONFIG_LIBCELT_DECODER)            += libcelt_dec.o

> +OBJS-$(CONFIG_LIBCODEC2_DECODER)          += libcodec2.o
> +OBJS-$(CONFIG_LIBCODEC2_ENCODER)          += libcodec2.o

+OBJS-$(CONFIG_LIBCODEC2_DECODER)          += libcodec2.o codec2utils.o
+OBJS-$(CONFIG_LIBCODEC2_ENCODER)          += libcodec2.o codec2utils.o

>  OBJS-$(CONFIG_LIBFDK_AAC_DECODER)         += libfdk-aacdec.o
>  OBJS-$(CONFIG_LIBFDK_AAC_ENCODER)         += libfdk-aacenc.o
>  OBJS-$(CONFIG_LIBGSM_DECODER)             += libgsmdec.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 4712592a5f..5296fac507 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -618,6 +618,7 @@ static void register_all(void)
>      REGISTER_DECODER(QDMC_AT,           qdmc_at);
>      REGISTER_DECODER(QDM2_AT,           qdm2_at);
>      REGISTER_DECODER(LIBCELT,           libcelt);
> +    REGISTER_ENCDEC (LIBCODEC2,         libcodec2);
>      REGISTER_ENCDEC (LIBFDK_AAC,        libfdk_aac);
>      REGISTER_ENCDEC (LIBGSM,            libgsm);
>      REGISTER_ENCDEC (LIBGSM_MS,         libgsm_ms);
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index c594993766..488eb8b1f5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -622,6 +622,7 @@ enum AVCodecID {
>      AV_CODEC_ID_PAF_AUDIO,
>      AV_CODEC_ID_ON2AVC,
>      AV_CODEC_ID_DSS_SP,
> +    AV_CODEC_ID_CODEC2,
>  
>      AV_CODEC_ID_FFWAVESYNTH = 0x15800,
>      AV_CODEC_ID_SONIC,
> diff --git a/libavcodec/codec2utils.c b/libavcodec/codec2utils.c
> new file mode 100644
> index 0000000000..8f5012f845
> --- /dev/null
> +++ b/libavcodec/codec2utils.c
> @@ -0,0 +1,118 @@
> +/*
> + * codec2 utility functions

> + * Copyright (c) 2017 Tomas Härdin

Missing copyright attribution if the imported code is non-trivial
(better err on the side of more attribution).

> + *
> + * 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 02110-1301 USA
> + */
> +
> +#include <string.h>
> +#include "internal.h"
> +#include "libavcodec/codec2utils.h"
> +
> +//from codec2.h, repeated here to avoid a dependency

> +#define CODEC2_MODE_3200 0
> +#define CODEC2_MODE_2400 1
> +#define CODEC2_MODE_1600 2
> +#define CODEC2_MODE_1400 3
> +#define CODEC2_MODE_1300 4
> +#define CODEC2_MODE_1200 5
> +#define CODEC2_MODE_700  6
> +#define CODEC2_MODE_700B 7
> +#define CODEC2_MODE_700C 8
> +

> +int avpriv_codec2_mode_from_str(void *logctx, const char *modestr) {

Please normalize the coding style. Same below.

> +    //statically assert the size of avpriv_codec2_header
> +    //putting it here because all codec2 things depend on codec2utils
> +    switch(0) {
> +    case 0:
> +    case sizeof(avpriv_codec2_header) == 7: //if false then the compiler will complain
> +        break;
> +    }

I see how it works. This is a nice trick. Want to make it an official
macro FF_ASSERT_STATIC()?

> +
> +    if (!modestr) {
> +        av_log(logctx, AV_LOG_ERROR, "raw codec2 streams need -mode set\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +#define MATCH(x) do { if (!strcmp(modestr, #x)) { return CODEC2_MODE_##x; } } while (0)
> +    MATCH(3200);
> +    MATCH(2400);
> +    MATCH(1600);
> +    MATCH(1400);
> +    MATCH(1300);
> +    MATCH(1200);
> +    MATCH(700);
> +    MATCH(700B);
> +    MATCH(700C);
> +
> +    av_log(logctx, AV_LOG_ERROR, "invalid codec2 mode: %s\n", modestr);
> +    return AVERROR(EINVAL);
> +}
> +
> +int avpriv_codec2_mode_bit_rate(void *logctx, int mode) {

> +    int ret = 8 * 8000 * avpriv_codec2_mode_block_align(logctx, mode) / avpriv_codec2_mode_frame_size(logctx, mode);
> +    if (ret <= 0) {
> +        av_log(logctx, AV_LOG_WARNING, "unknown codec2 mode %i, can't estimate bitrate\n", mode);

I bet you did not test the invalid case. Otherwise, I think you would
have found a division by 0.

> +    }
> +    return ret;
> +}
> +
> +int avpriv_codec2_mode_frame_size(void *logctx, int mode) {
> +    switch (mode) {

> +    case CODEC2_MODE_3200: return 160;
> +    case CODEC2_MODE_2400: return 160;
> +    case CODEC2_MODE_1600: return 320;
> +    case CODEC2_MODE_1400: return 320;
> +    case CODEC2_MODE_1300: return 320;
> +    case CODEC2_MODE_1200: return 320;
> +    case CODEC2_MODE_700:  return 320;
> +    case CODEC2_MODE_700B: return 320;
> +    case CODEC2_MODE_700C: return 320;

For all these mappings, I think a table would be more elegant.

> +    default:
> +        av_log(logctx, AV_LOG_ERROR, "unknown codec2 mode %i, can't find frame_size\n", mode);
> +        return 0;
> +    }
> +}
> +
> +int avpriv_codec2_mode_block_align(void *logctx, int mode) {
> +    switch (mode) {
> +    case CODEC2_MODE_3200: return 8;
> +    case CODEC2_MODE_2400: return 6;
> +    case CODEC2_MODE_1600: return 8;
> +    case CODEC2_MODE_1400: return 7;
> +    case CODEC2_MODE_1300: return 7;
> +    case CODEC2_MODE_1200: return 6;
> +    case CODEC2_MODE_700:  return 4;
> +    case CODEC2_MODE_700B: return 4;
> +    case CODEC2_MODE_700C: return 4;
> +    default:
> +        av_log(logctx, AV_LOG_ERROR, "unknown codec2 mode %i, can't find block_align\n", mode);
> +        return 0;
> +    }
> +}
> +

> +avpriv_codec2_header avpriv_codec2_make_header(int mode) {
> +    avpriv_codec2_header header = {
> +        .magic = {0xC0, 0xDE, 0xC2},
> +        //version 0.8 as of 2017-08-02 (r3345)
> +        .version_major = 0,
> +        .version_minor = 8,
> +        .mode = mode,
> +        .flags = 0,
> +    };
> +    return header;
> +}

This would be more efficient as static inline.

> diff --git a/libavcodec/codec2utils.h b/libavcodec/codec2utils.h
> new file mode 100644
> index 0000000000..0551e88a20
> --- /dev/null
> +++ b/libavcodec/codec2utils.h
> @@ -0,0 +1,58 @@
> +/*
> + * codec2 utility functions
> + * Copyright (c) 2017 Tomas Härdin
> + *
> + * 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 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_CODEC2UTILS_H
> +#define AVCODEC_CODEC2UTILS_H
> +

> +#include "internal.h"

Why?

> +
> +//Converts strings like "1400" -> 3 and so on
> +//logctx is used for av_log()
> +//Returns <0 if modestr is invalid
> +int avpriv_codec2_mode_from_str(void *logctx, const char *modestr);
> +
> +//The three following functions are here to avoid needing libavformat/codec2.c to depend on libcodec2
> +
> +//Computes bitrate from mode, with frames rounded up to the nearest octet.
> +//So 700 bit/s (28 bits/frame) becomes 800 bits/s (32 bits/frame).
> +//logctx is used for av_log()
> +//Returns <0 if mode is invalid
> +int avpriv_codec2_mode_bit_rate(void *logctx, int mode);
> +
> +//duplicates codec2_samples_per_frame()
> +int avpriv_codec2_mode_frame_size(void *logctx, int mode);
> +
> +//duplicates (codec2_bits_per_frame()+7)/8
> +int avpriv_codec2_mode_block_align(void *logctx, int mode);
> +
> +//Used as extradata

> +typedef struct {

Anonymous structures are discouraged.

> +    uint8_t magic[3];
> +    uint8_t version_major;
> +    uint8_t version_minor;
> +    uint8_t mode;
> +    uint8_t flags;

> +} avpriv_codec2_header;

Types and structures names do not need to be namespaced when they are
private. On the other hand, the case is not consistent with the rest of
the code.

> +
> +//Used in codec2raw demuxer and libcodec2 encoder to make up .c2 headers
> +avpriv_codec2_header avpriv_codec2_make_header(int mode);
> +
> +#endif /* AVCODEC_CODEC2UTILS_H */
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 6f43b68b83..09727d7de2 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -2657,6 +2657,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .props     = AV_CODEC_PROP_LOSSY,
>      },
>      {
> +        .id        = AV_CODEC_ID_CODEC2,
> +        .type      = AVMEDIA_TYPE_AUDIO,
> +        .name      = "codec2",

> +        .long_name = NULL_IF_CONFIG_SMALL("codec2"),

Not helpful.

> +        .props     = AV_CODEC_PROP_LOSSY,
> +    },
> +    {
>          .id        = AV_CODEC_ID_G723_1,
>          .type      = AVMEDIA_TYPE_AUDIO,
>          .name      = "g723_1",
> diff --git a/libavcodec/libcodec2.c b/libavcodec/libcodec2.c
> new file mode 100644
> index 0000000000..a7451cc881
> --- /dev/null
> +++ b/libavcodec/libcodec2.c
> @@ -0,0 +1,190 @@
> +/*
> + * codec2 encoder/decoder using libcodec2
> + * Copyright (c) 2017 Tomas Härdin
> + *
> + * 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 02110-1301 USA
> + */
> +
> +#include <codec2/codec2.h>
> +#include "avcodec.h"
> +#include "libavutil/opt.h"
> +#include "internal.h"
> +#include "codec2utils.h"
> +
> +typedef struct {
> +    const AVClass *class;
> +    struct CODEC2 *codec;
> +    char *mode;

> +} libcodec2_context;

Case not consistent with the rest of the code.

> +
> +static const AVOption options[] = {

> +    //not AV_OPT_FLAG_DECODING_PARAM since mode should come from the demuxer

??? If it comes from the raw demuxer, it is still a decoding param, is
it not?

> +    { "mode", "codec2 mode", offsetof(libcodec2_context, mode), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },

I think it would be better to make the mode option an integer and use
the CONST system to expose the valid values. Same below.

> +    { NULL },
> +};
> +

> +static const AVClass codec2_class = {
> +    .class_name = "libcodec2",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};

IIRC, you cannot use the same class for two components. Does "ffmpeg -h
full" still work?

> +
> +static av_cold int libcodec2_init_common(AVCodecContext *avctx)
> +{
> +    libcodec2_context *c2 = avctx->priv_data;
> +    int mode;
> +
> +    //take -mode if set
> +    if (c2->mode) {
> +        if ((mode = avpriv_codec2_mode_from_str(avctx, c2->mode)) < 0) {
> +            return mode;
> +        }
> +
> +        if (!(avctx->extradata = av_mallocz(sizeof(avpriv_codec2_header) + AV_INPUT_BUFFER_PADDING_SIZE))) {
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        avctx->extradata_size = sizeof(avpriv_codec2_header);
> +        *((avpriv_codec2_header*)avctx->extradata) = avpriv_codec2_make_header(mode);
> +    } else {
> +        if (avctx->extradata_size != sizeof(avpriv_codec2_header)) {
> +            av_log(avctx, AV_LOG_ERROR, "must have exactly %zu bytes of extradata (got %i)\n",
> +                   sizeof(avpriv_codec2_header), avctx->extradata_size);

return AVERROR_INVALIDDATA?

> +        }
> +
> +        mode = ((avpriv_codec2_header*)avctx->extradata)->mode;
> +    }
> +
> +    if (!(c2->codec = codec2_create(mode))) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    avctx->frame_size = codec2_samples_per_frame(c2->codec);
> +    avctx->block_align = (codec2_bits_per_frame(c2->codec) + 7) / 8;
> +    codec2_set_natural_or_gray(c2->codec, 1);
> +
> +    return 0;
> +}
> +
> +static av_cold int libcodec2_init_decoder(AVCodecContext *avctx)
> +{
> +    avctx->sample_rate      = 8000;
> +    avctx->channels         = 1;
> +    avctx->sample_fmt       = AV_SAMPLE_FMT_S16;
> +    avctx->channel_layout   = AV_CH_LAYOUT_MONO;
> +
> +    return libcodec2_init_common(avctx);
> +}
> +
> +static av_cold int libcodec2_init_encoder(AVCodecContext *avctx)
> +{
> +    //will need to be smarter once we get wideband support
> +    if (avctx->sample_rate != 8000 ||
> +        avctx->channels != 1 ||
> +        avctx->sample_fmt != AV_SAMPLE_FMT_S16) {
> +        av_log(avctx, AV_LOG_ERROR, "only 8 kHz 16-bit mono allowed\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    return libcodec2_init_common(avctx);
> +}
> +
> +static av_cold int libcodec2_close(AVCodecContext *avctx)
> +{
> +    libcodec2_context *c2 = avctx->priv_data;
> +
> +    codec2_destroy(c2->codec);
> +    return 0;
> +}
> +
> +static int libcodec2_decode(AVCodecContext *avctx, void *data,
> +                            int *got_frame_ptr, AVPacket *pkt)
> +{
> +    libcodec2_context *c2 = avctx->priv_data;
> +    AVFrame *frame = data;
> +    int ret, nframes, i;
> +    int16_t *output;
> +
> +    nframes = pkt->size / avctx->block_align;
> +    frame->nb_samples = avctx->frame_size * nframes;
> +
> +    if ((ret = ff_get_buffer(avctx, frame, 0)) < 0) {
> +        return ret;
> +    }
> +
> +    output = (int16_t *)frame->data[0];
> +
> +    for (i = 0; i < nframes; i++) {

> +        codec2_decode(c2->codec, &output[i*avctx->frame_size], &pkt->data[i*avctx->block_align]);

I suggest:

    input  += avctx->frame_size;
    output += avctx->frame_size;

and drop the multiplication.

> +    }
> +
> +    if (nframes > 0) {
> +        *got_frame_ptr = 1;
> +    }
> +
> +    return nframes * avctx->block_align;
> +}
> +
> +static int libcodec2_encode(AVCodecContext *avctx, AVPacket *avpkt,
> +                            const AVFrame *frame, int *got_packet_ptr)
> +{
> +    libcodec2_context *c2 = avctx->priv_data;
> +    int16_t *samples = (int16_t *)frame->data[0];
> +    int ret;
> +
> +    if ((ret = ff_alloc_packet2(avctx, avpkt, avctx->block_align, 0)) < 0) {
> +        return ret;
> +    }
> +
> +    codec2_encode(c2->codec, avpkt->data, samples);
> +    *got_packet_ptr = 1;
> +
> +    return 0;
> +}
> +
> +AVCodec ff_libcodec2_decoder = {
> +    .name                   = "libcodec2",

> +    .long_name              = NULL_IF_CONFIG_SMALL("codec2 encoder/decoder using libcodec2"),

This is not an encoder.

> +    .type                   = AVMEDIA_TYPE_AUDIO,
> +    .id                     = AV_CODEC_ID_CODEC2,
> +    .priv_data_size         = sizeof(libcodec2_context),
> +    .init                   = libcodec2_init_decoder,
> +    .close                  = libcodec2_close,
> +    .decode                 = libcodec2_decode,
> +    .capabilities           = 0,
> +    .supported_samplerates  = (const int[]){ 8000, 0 },
> +    .sample_fmts            = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_NONE },
> +    .channel_layouts        = (const uint64_t[]) { AV_CH_LAYOUT_MONO, 0 },
> +    .priv_class             = &codec2_class,
> +};
> +
> +AVCodec ff_libcodec2_encoder = {
> +    .name                   = "libcodec2",

> +    .long_name              = NULL_IF_CONFIG_SMALL("codec2 encoder/decoder using libcodec2"),

And this is not a decoder.

> +    .type                   = AVMEDIA_TYPE_AUDIO,
> +    .id                     = AV_CODEC_ID_CODEC2,
> +    .priv_data_size         = sizeof(libcodec2_context),
> +    .init                   = libcodec2_init_encoder,
> +    .close                  = libcodec2_close,
> +    .encode2                = libcodec2_encode,
> +    .capabilities           = 0,
> +    .supported_samplerates  = (const int[]){ 8000, 0 },
> +    .sample_fmts            = (const enum AVSampleFormat[]) { AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_NONE },
> +    .channel_layouts        = (const uint64_t[]) { AV_CH_LAYOUT_MONO, 0 },
> +    .priv_class             = &codec2_class,
> +};
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 02c4f41800..7473000579 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR  57
> -#define LIBAVCODEC_VERSION_MINOR 102
> +#define LIBAVCODEC_VERSION_MINOR 103
>  #define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index b0ef82cdd4..ff1a1412eb 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -128,6 +128,10 @@ OBJS-$(CONFIG_CAVSVIDEO_MUXER)           += rawenc.o
>  OBJS-$(CONFIG_CDG_DEMUXER)               += cdg.o
>  OBJS-$(CONFIG_CDXL_DEMUXER)              += cdxl.o
>  OBJS-$(CONFIG_CINE_DEMUXER)              += cinedec.o
> +OBJS-$(CONFIG_CODEC2_DEMUXER)            += codec2.o rawdec.o
> +OBJS-$(CONFIG_CODEC2_MUXER)              += codec2.o rawenc.o
> +OBJS-$(CONFIG_CODEC2RAW_DEMUXER)         += codec2.o rawdec.o
> +OBJS-$(CONFIG_CODEC2RAW_MUXER)           += rawenc.o
>  OBJS-$(CONFIG_CONCAT_DEMUXER)            += concatdec.o
>  OBJS-$(CONFIG_CRC_MUXER)                 += crcenc.o
>  OBJS-$(CONFIG_DATA_DEMUXER)              += rawdec.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index 1ebc14231c..26f0f1eccd 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -94,6 +94,8 @@ static void register_all(void)
>      REGISTER_DEMUXER (CDG,              cdg);
>      REGISTER_DEMUXER (CDXL,             cdxl);
>      REGISTER_DEMUXER (CINE,             cine);
> +    REGISTER_MUXDEMUX(CODEC2,           codec2);
> +    REGISTER_MUXDEMUX(CODEC2RAW,        codec2raw);
>      REGISTER_DEMUXER (CONCAT,           concat);
>      REGISTER_MUXER   (CRC,              crc);
>      REGISTER_MUXER   (DASH,             dash);
> diff --git a/libavformat/codec2.c b/libavformat/codec2.c
> new file mode 100644
> index 0000000000..ea9c22e501
> --- /dev/null
> +++ b/libavformat/codec2.c
> @@ -0,0 +1,244 @@
> +/*
> + * codec2 raw demuxer
> + * Copyright (c) 2017 Tomas Härdin
> + *
> + * 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 02110-1301 USA
> + */
> +
> +#include "libavcodec/codec2utils.h"
> +#include "avio_internal.h"
> +#include "avformat.h"
> +#include "internal.h"
> +#include "rawdec.h"
> +#include "rawenc.h"
> +#include "pcm.h"
> +
> +typedef struct {
> +    const AVClass *class;
> +    char *mode;
> +} codec2_context;
> +
> +static int codec2_probe(AVProbeData *p)
> +{
> +    if (p->buf_size < sizeof(avpriv_codec2_header)) {
> +        return 0;
> +    }
> +
> +    //file starts wih 0xC0DEC2

> +    if (p->buf[0] == 0xC0 && p->buf[1] == 0xDE && p->buf[2] == 0xC2) {
> +        return AVPROBE_SCORE_MAX;

As pointed by Carl Eugen, this is not reliable enough. The problem is
not misdetecting the version of files, the problem is detecting files
that are not codec2 as codec2. A lot of files can start with 192 222
194.

> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int codec2raw_read_header_common(AVFormatContext *s, AVStream *st)
> +{
> +    int mode = ((avpriv_codec2_header*)st->codecpar->extradata)->mode;
> +
> +    //let decoder determine as many properties as possible (channels, sample format, channel layout etc)
> +    //we need to set sample_rate and bit_rate for duration and seeking to work properly
> +    st->codecpar->codec_type        = AVMEDIA_TYPE_AUDIO;
> +    st->codecpar->codec_id          = AV_CODEC_ID_CODEC2;
> +    st->codecpar->sample_rate       = 8000;
> +    st->codecpar->channels          = 1;
> +    st->codecpar->format            = AV_SAMPLE_FMT_S16;
> +    st->codecpar->channel_layout    = AV_CH_LAYOUT_MONO;
> +    st->codecpar->bit_rate          = avpriv_codec2_mode_bit_rate(s, mode);
> +    st->codecpar->frame_size        = avpriv_codec2_mode_frame_size(s, mode);
> +    st->codecpar->block_align       = avpriv_codec2_mode_block_align(s, mode);
> +
> +    avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> +

> +    //replicating estimate_timings_from_bit_rate() in utils.c to avoid warnings

Please elaborate.

> +    if (s->pb && st->codecpar->bit_rate > 0) {
> +        int64_t filesize = avio_size(s->pb);
> +        if (filesize > s->internal->data_offset) {
> +            filesize -= s->internal->data_offset;
> +            st->duration = av_rescale(8 * filesize,
> +                                      st->time_base.den,
> +                                      st->codecpar->bit_rate * (int64_t) st->time_base.num);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static int codec2_read_header(AVFormatContext *s)
> +{
> +    AVStream *st;
> +
> +    if (!(st = avformat_new_stream(s, NULL)) ||
> +        ff_alloc_extradata(st->codecpar, sizeof(avpriv_codec2_header))) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    s->internal->data_offset = sizeof(avpriv_codec2_header);
> +    avio_read(s->pb, st->codecpar->extradata, sizeof(avpriv_codec2_header));
> +

> +    if (st->codecpar->extradata[0] != 0xC0 ||
> +        st->codecpar->extradata[1] != 0xDE ||
> +        st->codecpar->extradata[2] != 0xC2) {

Since you do it twice, you could make it a function.

> +        av_log(s, AV_LOG_ERROR, "not a .c2 file\n");
> +    }

I think setting the time base information is missing.

> +
> +    return codec2raw_read_header_common(s, st);
> +}
> +
> +//based off of ff_raw_read_partial_packet()
> +static int codec2_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    AVStream *st = s->streams[0];
> +    int ret, size, n, block_align, frame_size;
> +
> +    block_align = st->codecpar->block_align;
> +    frame_size  = st->codecpar->frame_size;
> +

> +    //Read roughly 0.1 seconds worth of data.
> +    n = st->codecpar->bit_rate / ((int)(8/0.1) * block_align) + 1;

I do not like that block. Maybe make it an option?

> +    size = n * block_align;
> +
> +    if (av_new_packet(pkt, size) < 0)
> +        return AVERROR(ENOMEM);
> +
> +    //try to read desired number of bytes, recompute n from to actual number of bytes read
> +    pkt->pos= avio_tell(s->pb);
> +    pkt->stream_index = 0;
> +    ret = ffio_read_partial(s->pb, pkt->data, size);
> +    if (ret < 0) {
> +        av_packet_unref(pkt);
> +        return ret;
> +    }
> +    av_shrink_packet(pkt, ret);

av_get_packet()?

> +    n = ret / block_align;
> +
> +    //only set duration - compute_pkt_fields() and ff_pcm_read_seek() takes care of everything else
> +    //tested by spamming the seek functionality in ffplay
> +    pkt->duration = n * frame_size;
> +
> +    return ret;
> +}
> +
> +static int codec2_write_header(AVFormatContext *s)
> +{
> +    AVStream *st;
> +
> +    if (s->nb_streams != 1 || s->streams[0]->codecpar->codec_id != AV_CODEC_ID_CODEC2) {
> +        av_log(s, AV_LOG_ERROR, ".c2 files must have exactly one codec2 stream\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    st = s->streams[0];
> +
> +    if (st->codecpar->extradata_size != sizeof(avpriv_codec2_header)) {

> +        av_log(s, AV_LOG_ERROR, ".c2 files require exactly %zu bytes of extradata (got %i)\n",

IIRC, the z modifier is broken on some proprietary system. I personally
do not care.

> +               sizeof(avpriv_codec2_header), st->codecpar->extradata_size);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    avio_write(s->pb, st->codecpar->extradata, sizeof(avpriv_codec2_header));
> +
> +    return 0;
> +}
> +
> +static int codec2raw_read_header(AVFormatContext *s)
> +{
> +    codec2_context *c2 = s->priv_data;
> +    AVStream *st;
> +    int mode;
> +
> +    if (!(st = avformat_new_stream(s, NULL)) ||
> +        ff_alloc_extradata(st->codecpar, sizeof(avpriv_codec2_header))) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    if ((mode = avpriv_codec2_mode_from_str(s, c2->mode)) < 0) {
> +        return mode;
> +    }
> +
> +    s->internal->data_offset = 0;
> +    *((avpriv_codec2_header*)st->codecpar->extradata) = avpriv_codec2_make_header(mode);
> +
> +    return codec2raw_read_header_common(s, st);
> +}
> +
> +static const AVOption options[] = {
> +    { "mode", "codec2 mode", offsetof(codec2_context, mode), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, AV_OPT_FLAG_DECODING_PARAM},
> +    { NULL },
> +};
> +
> +static const AVClass codec2_class = {
> +    .class_name = "codec2",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +    .category   = AV_CLASS_CATEGORY_DEMUXER,
> +};
> +
> +static const AVClass codec2raw_class = {
> +    .class_name = "codec2raw",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +    .category   = AV_CLASS_CATEGORY_DEMUXER,
> +};
> +
> +#if CONFIG_CODEC2_DEMUXER
> +AVInputFormat ff_codec2_demuxer = {
> +    .name           = "codec2",
> +    .long_name      = NULL_IF_CONFIG_SMALL("codec2 .c2 file"),
> +    .priv_data_size = sizeof(codec2_context),
> +    .extensions     = "c2",
> +    .read_probe     = codec2_probe,
> +    .read_header    = codec2_read_header,
> +    .read_packet    = codec2_read_packet,
> +    .read_seek      = ff_pcm_read_seek,
> +    .flags          = AVFMT_GENERIC_INDEX,
> +    .raw_codec_id   = AV_CODEC_ID_CODEC2,
> +    .priv_class     = &codec2_class,
> +};
> +#endif
> +
> +#if CONFIG_CODEC2_MUXER
> +AVOutputFormat ff_codec2_muxer = {
> +    .name           = "codec2",
> +    .long_name      = NULL_IF_CONFIG_SMALL("codec2 .c2 file"),
> +    .priv_data_size = sizeof(codec2_context),
> +    .extensions     = "c2",
> +    .audio_codec    = AV_CODEC_ID_CODEC2,
> +    .video_codec    = AV_CODEC_ID_NONE,
> +    .write_header   = codec2_write_header,
> +    .write_packet   = ff_raw_write_packet,
> +    .flags          = AVFMT_NOTIMESTAMPS,
> +    .priv_class     = &codec2_class,
> +};
> +#endif
> +
> +#if CONFIG_CODEC2RAW_DEMUXER
> +AVInputFormat ff_codec2raw_demuxer = {
> +    .name           = "codec2raw",
> +    .long_name      = NULL_IF_CONFIG_SMALL("raw codec2"),
> +    .priv_data_size = sizeof(codec2_context),
> +    .read_header    = codec2raw_read_header,
> +    .read_packet    = codec2_read_packet,
> +    .read_seek      = ff_pcm_read_seek,
> +    .flags          = AVFMT_GENERIC_INDEX,
> +    .raw_codec_id   = AV_CODEC_ID_CODEC2,
> +    .priv_class     = &codec2raw_class,
> +};
> +#endif
> diff --git a/libavformat/rawenc.c b/libavformat/rawenc.c
> index 26baa850e1..f622c02244 100644
> --- a/libavformat/rawenc.c
> +++ b/libavformat/rawenc.c
> @@ -104,6 +104,20 @@ AVOutputFormat ff_cavsvideo_muxer = {
>  };
>  #endif
>  
> +#if CONFIG_CODEC2RAW_MUXER
> +AVOutputFormat ff_codec2raw_muxer = {
> +    .name              = "codec2raw",
> +    .long_name         = NULL_IF_CONFIG_SMALL("raw codec2"),
> +    //.extensions        = "c2",
> +    .audio_codec       = AV_CODEC_ID_CODEC2,
> +    .video_codec       = AV_CODEC_ID_NONE,
> +    .write_header      = force_one_stream,
> +    .write_packet      = ff_raw_write_packet,
> +    .flags             = AVFMT_NOTIMESTAMPS,
> +};
> +#endif
> +
> +
>  #if CONFIG_DATA_MUXER
>  AVOutputFormat ff_data_muxer = {
>      .name              = "data",
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 38d247c6cd..3558e700ac 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -898,6 +898,7 @@ static int determinable_frame_size(AVCodecContext *avctx)
>      case AV_CODEC_ID_MP1:
>      case AV_CODEC_ID_MP2:
>      case AV_CODEC_ID_MP3:
> +    case AV_CODEC_ID_CODEC2:
>          return 1;
>      }
>  
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 48b81f2e48..a8cf4c158e 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
>  // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
>  // Also please add any ticket numbers that you believe might be affected here
>  #define LIBAVFORMAT_VERSION_MAJOR  57
> -#define LIBAVFORMAT_VERSION_MINOR  76
> +#define LIBAVFORMAT_VERSION_MINOR  77
>  #define LIBAVFORMAT_VERSION_MICRO 100
>  
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> -- 
> 2.13.3
> 




> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On Thu, Aug 03, 2017 at 12:40:04AM +0200, Tomas Härdin wrote:

> +    //statically assert the size of avpriv_codec2_header
> +    //putting it here because all codec2 things depend on codec2utils
> +    switch(0) {
> +    case 0:
> +    case sizeof(avpriv_codec2_header) == 7: //if false then the compiler will complain
> +        break;
> +    }

I was pretty sure we had some kind of static assert already,
based on negative array size in a struct (so it can work outside
functions)...
But doesn't matter due to next comments...

> +        *((avpriv_codec2_header*)avctx->extradata) = avpriv_codec2_make_header(mode);

I am pretty sure this is a strict aliasing violation.
Even ignoring the ugliness and performance issues with
returning structs and assigning them causing lots of
pointless copies.
Also, since all struct elements are bytes, it isn't
really any more readable than just assigning to the
byte array, with the names you not have in the struct
in comments instead.
And that is without going into all the ways someone could
change that struct (e.g. in case of new features) that
would completely break this kind of code due to endianness,
alignment, ...
Even just supporting both the current and a potential future version
that changes any of the fields would be hard with this kind of code.

> +        if (avctx->extradata_size != sizeof(avpriv_codec2_header)) {
> +            av_log(avctx, AV_LOG_ERROR, "must have exactly %zu bytes of extradata (got %i)\n",
> +                   sizeof(avpriv_codec2_header), avctx->extradata_size);
> +        }

I would think at least if it is less you wouldn't want to just
continue? Even if extradata is required to be padded (is it?),
blindly selecting 0 as mode doesn't seem very likely to be right.

> +    output = (int16_t *)frame->data[0];

The codec2 documentation seems pretty non-existant, so I
assume you are right that this is always in native endianness.
However from what I can see codec2 actually uses the "short" type,
not int16_t.
While it shouldn't make a difference in practice, if casting anyway
I'd suggest using the type matching the API.

> +    if (nframes > 0) {
> +        *got_frame_ptr = 1;
> +    }

Not just *got_frame_ptr = nframes > 0; ?

> +    int16_t *samples = (int16_t *)frame->data[0];

You are casting the const away.
Did they just forget to add the const to the API?
If so, can you suggest it to be added?
Otherwise if it's intentional you need to make a copy.

> +    int ret;
> +
> +    if ((ret = ff_alloc_packet2(avctx, avpkt, avctx->block_align, 0)) < 0) {

If you merge the declaration and assignment it you would get
for free not having the assignment inside the if, which I
still think is just horrible style. :)

> +    //file starts wih 0xC0DEC2
> +    if (p->buf[0] == 0xC0 && p->buf[1] == 0xDE && p->buf[2] == 0xC2) {
> +        return AVPROBE_SCORE_MAX;
> +    }

As mentioned, try to find a few more bits and reduce the score.
If only these 3 bytes match, I would suggest a rather low score.
(I doubt there are enough useful bits in this header to allow
reaching MAX score, it's a shame they didn't invest a byte or
2 more, especially considering that while 0xC0DEC2 might look
clever it doesn't exactly get maximum entropy out of those 3 bytes).

> +    AVStream *st;
> +
> +    if (!(st = avformat_new_stream(s, NULL)) ||

Can merge allocation and declaration again.

> +    //Read roughly 0.1 seconds worth of data.
> +    n = st->codecpar->bit_rate / ((int)(8/0.1) * block_align) + 1;

That doesn't seem overly readable to me...
// Read about 1/10th of a second worth of data
st->codecpar->bit_rate / 10 / 8 / block_align + 1

Seems not really worse and doesn't have doubles and casts...
Otherwise
blocks_per_second = st->codecpar->bit_rate / 8 / block_align;
n = blocks_per_second / 10;
might be even clearer.

> +    if (av_new_packet(pkt, size) < 0)
> +        return AVERROR(ENOMEM);
> +    //try to read desired number of bytes, recompute n from to actual number of bytes read
> +    pkt->pos= avio_tell(s->pb);
> +    pkt->stream_index = 0;
> +    ret = ffio_read_partial(s->pb, pkt->data, size);
> +    if (ret < 0) {
> +        av_packet_unref(pkt);
> +        return ret;
> +    }
> +    av_shrink_packet(pkt, ret);

Ouch, why are you not just using av_get_packet?

> +    avio_write(s->pb, st->codecpar->extradata, sizeof(avpriv_codec2_header));

Error check?

> +    if (!(st = avformat_new_stream(s, NULL)) ||
> +        ff_alloc_extradata(st->codecpar, sizeof(avpriv_codec2_header))) {
> +        return AVERROR(ENOMEM);

Here and in the other read_header, this would preferably preserve the error that
ff_alloc_extradata returned.