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

[Tomas Härdin]

On 2017-08-15 21:15, Moritz Barsnick wrote:

On Tue, Aug 08, 2017 at 23:49:45 +0200, Tomas Härdin wrote:

Feel free to comment

Don't forget to mention #1959 in the commit message.


Huh, I didn't know there was a ticket for it. That audiobook guy popped 
up on [Freetel-codec2] too, and thus the container discussion was born. 
It's also part of the reason why a raw demuxer is needed. Guess I'll 
have to send him some e-mail too, maybe I can nip some format headaches 
in the bud


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

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

[Moritz Barsnick]

On Tue, Aug 08, 2017 at 23:49:45 +0200, Tomas Härdin wrote:
> Feel free to comment

Don't forget to mention #1959 in the commit message.

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

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

[Tomas]

 Michael Niedermayer skrev 

> On Tue, Aug 08, 2017 at 11:49:45PM +0200, Tomas Härdin wrote:
>  [...]
> 
> >  ffmpeg.c |3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 5bd20883fdc12aefa609fc803fe5709069b3e9a0  
> > 0003-Don-t-complain-about-codec2-s-700-bit-s-modes-in-ffm.patch
> > From b693b6175289e6ad0c643462d8f69f6830086099 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
> > Date: Thu, 3 Aug 2017 17:33:04 +0200
> > Subject: [PATCH 3/3] Don't complain about codec2's 700 bit/s modes in 
> > ffmpeg.c
> > 
> > ---
> >  ffmpeg.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > 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");
> >  
> 
> LGTM
> 
> alternatively you could add a minimum bitrate parameter to AVCodec
> or AVCodecDescriptor

That seems a bit excessive for just one case. Bitrate merely gets set as a side 
effect of setting mode.

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

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

[Paul B Mahol]

On 8/9/17, Michael Niedermayer  wrote:
> On Tue, Aug 08, 2017 at 11:49:45PM +0200, Tomas Härdin wrote:
>  [...]
>
>>  ffmpeg.c |3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 5bd20883fdc12aefa609fc803fe5709069b3e9a0
>> 0003-Don-t-complain-about-codec2-s-700-bit-s-modes-in-ffm.patch
>> From b693b6175289e6ad0c643462d8f69f6830086099 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
>> Date: Thu, 3 Aug 2017 17:33:04 +0200
>> Subject: [PATCH 3/3] Don't complain about codec2's 700 bit/s modes in
>> ffmpeg.c
>>
>> ---
>>  ffmpeg.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> 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");
>>
>
> LGTM
>
> alternatively you could add a minimum bitrate parameter to AVCodec
> or AVCodecDescriptor

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

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

[Michael Niedermayer]

On Tue, Aug 08, 2017 at 11:49:45PM +0200, Tomas Härdin wrote:
 [...]

>  ffmpeg.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 5bd20883fdc12aefa609fc803fe5709069b3e9a0  
> 0003-Don-t-complain-about-codec2-s-700-bit-s-modes-in-ffm.patch
> From b693b6175289e6ad0c643462d8f69f6830086099 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
> Date: Thu, 3 Aug 2017 17:33:04 +0200
> Subject: [PATCH 3/3] Don't complain about codec2's 700 bit/s modes in ffmpeg.c
> 
> ---
>  ffmpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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");
>  

LGTM

alternatively you could add a minimum bitrate parameter to AVCodec
or AVCodecDescriptor

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tomas Härdin]

On Fri, 2017-08-04 at 19:20 +0200, Tomas Härdin wrote:
> TODO:
> 
> * have -mode be an integer and use the CONST system Nicolas mentioned
> * option for demuxing multiple frames at a time
> * sort the extradata aliasing thing
> * address API issues in libcodec2, possibly modify the format (there
> should still be time)

Had some time today and went over and addressed all TODOs but the API
issue in libcodec2. For that I need to figure out why reCAPTCHA is
broken on sourceforge.net, to be able to fix my ML registration >:|

I split the patchset into one for lavc and a second for lavf, plus a
third for the small ffmpeg CLI fix. Should make review simpler.

I also removed the 0xC0DEC2 magic from extradata, mostly as part of
reworking codec2utils

Finally I made codec2_probe only award AVPROBE_SCORE_MAX if the file
extension is .c2

Feel free to comment

/Tomas
From b693b6175289e6ad0c643462d8f69f6830086099 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
Date: Thu, 3 Aug 2017 17:33:04 +0200
Subject: [PATCH 3/3] Don't complain about codec2's 700 bit/s modes in ffmpeg.c

---
 ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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");
 
-- 
2.13.3

From 057b941e82aeb8ccc835fb6a11baac4cb245a531 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
Date: Tue, 8 Aug 2017 15:28:06 +0200
Subject: [PATCH 2/3] Add muxer/demuxer for raw codec2 and .c2 files

---
 Changelog|   1 +
 doc/general.texi |   2 +
 libavformat/Makefile |   4 +
 libavformat/allformats.c |   2 +
 libavformat/codec2.c | 313 +++
 libavformat/rawenc.c |  13 ++
 libavformat/utils.c  |   1 +
 libavformat/version.h|   2 +-
 8 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 libavformat/codec2.c

diff --git a/Changelog b/Changelog
index a3a16f0073..c95ffa8d16 100644
--- a/Changelog
+++ b/Changelog
@@ -33,6 +33,7 @@ version :
 - tlut2 video filter
 - floodfill video filter
 - 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/doc/general.texi b/doc/general.texi
index fd8d657e4e..4bcc2b2d91 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -299,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
diff --git a/libavformat/Makefile b/libavformat/Makefile
index b0ef82cdd4..9aa3466161 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)+= ../libavcodec/codec2utils.o codec2.o rawdec.o pcm.o
+OBJS-$(CONFIG_CODEC2_MUXER)  += ../libavcodec/codec2utils.o codec2.o rawenc.o
+OBJS-$(CONFIG_CODEC2RAW_DEMUXER) += ../libavcodec/codec2utils.o codec2.o rawdec.o pcm.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,

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

[Tomas Härdin]

On Thu, 2017-08-03 at 22:24 +0200, Reimar Döffinger wrote:
> 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.

Aliasing violation is a good point, but it also doesn't matter here.
extradata isn't used anywhere else in the function.

The struct is mostly a handy way of getting names for all the bytes, so
a enum with offsets could work just as well:

enum avpriv_codec2_header_bytes {
  AVPRIV_CODEC2_MAGIC0 = 0,
  AVPRIV_CODEC2_MAGIC1,
  AVPRIV_CODEC2_MAGIC2,
  AVPRIV_CODEC2_VERSION_MAJOR,
  ...
};

Endianness is also not an issue since it's all bytes. But: one nice way
to do this is perhaps a pair of functions for parsing / creating the
struct out of the byte array and vice versa.

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

This has been fixed in the updated patchset

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

Some kind of static assert for sizeof(short) == sizeof(int16_t) would
be nicest I think. If someone's on a platform with non-16-bit shorts
and they want --enable-libcodec2 to work then they can probably write
and submit a patch.

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

Sure.

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

No const in the API, but adding it is of course easy enough. That won't
get into Debian for some time however, so users would have to build
their own libcodec2.

Version #defines in  could help with this, currently
there are none so that's one way to detect the non-const API. I'll have
to run that past the other guys.

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

Fair enough. I use this style in a few places when stacking a bunch of
inits and checks for ENOMEM and such. doc/developer.html doesn't seem
to have a preference, and I've seen the style used elsewhere in FFmpeg.
I went with splitting the assignments and testing in all the ifs in the
updated patchset

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

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

[Reimar Döffinger]

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Thu, Aug 3, 2017 at 5:45 PM, Tomas Härdin  wrote:
>> > +} 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?
>
> Good catch, fixed. Should AVERROR_INVALIDDATA be preferred over
> AVERROR(EINVAL) perhaps? I've seen both used almost interchangably.
>

Generally, AVERROR_INVALIDDATA is for invalid bitstreams (which
extradata would be a part of), AVERROR(EINVAL) for invalid
user-provided options/settings.
Not all code follows that quite strictly, but thats the usual
definition we use around here.

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

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

[Tomas Härdin]

On Thu, 2017-08-03 at 13:00 +0200, Nicolas George wrote:
> Le sextidi 16 thermidor, an CCXXV, Tomas Härdin a écrit :
> > 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.

Default on-the-air is 1300 (aka FreeDV 1600), so that seems like a
reasonable default

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

There's other codec-specific special cases in ffmpeg.c, like the one
for AV_CODEC_ID_VP9 which doesn't even have an explanation. Separate
patch is a good idea however.

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

Alright, I can fix that. Although the only thing lifted straight are
the mode #defines

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

Cool.

> > 
> > * 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
> ???

The encoder is given frame_size number of samples each time. Are there
times where this is not the case? I haven't set
AV_CODEC_CAP_SMALL_LAST_FRAME.

> > 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?

I raised the possibility on [Freetel-codec2], either by coding mode as
part of the TwoCC (0xC208 for mode 8 for examples) or via extradata. I
imagine version + mode + flags in extradata would not be unreasonable.

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

I presume you mean codec2utils and not the libcodec2 deps.

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

Fixed.

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

Guess I'll nick the comment from the relevant files in libcodec2.


> > 
> > +int avpriv_codec2_mode_from_str(void *logctx, const char *modestr)
> > {
> Please normalize the coding style. Same below.

I presume you mean putting the function opening braces on their own
lines like the rest of FFmpeg. Fixed.

> > 
> > +//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()?

Sure. Could go in avutil.h or something. Maybe even inside a static
function with a fake name like:

#define FF_ASSERT_STATIC(x, name) static void name(){switch(0){...}}

Didn't do that in this patchset however.

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

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

[Nicolas George]

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?= 
> 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 :
>  - 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"
>  

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

[Tomas Härdin]

On Thu, 2017-08-03 at 01:07 +0200, Carl Eugen Hoyos wrote:
> 2017-08-03 0:40 GMT+02:00 Tomas Härdin :
> 
> > 
> > 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



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-08-03 0:40 GMT+02:00 Tomas Härdin :

> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tomas Härdin]

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.

/TomasFrom 569a252536ea224bcd44f55f0f5102ce1aa4ec77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 
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 :
 - 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