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