[FFmpeg-devel] [PATCH v3] lavf/h264: add support for h264 video from Arecont camera, fixes ticket #5154

[Reimar Döffinger]

On Tue, May 07, 2019 at 10:05:12AM +0530, Shivam Goyal wrote:
> +static int arecont_h264_probe(const AVProbeData *p)
> +{
> +    int i, j, k, o = 0;
> +    int ret = h264_probe(p);
> +    const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72, 0x0D, 0x0A};

Should be "static const" instead of just "const".
Also this seems to be just "--fbdr\r\n"?

> +        if (p->buf[i] == id[0] && !memcmp(id, p->buf + i, 8))
> +            o++;

Is that optimization really helping much?
If you want to speed it up, I suspect it would be more
useful to either go for AV_RL64 or memchr() or
a combination.
Also, sizeof() instead of hard-coding the 8.

> +    if (o >= 1)
> +        return ret + 1;

Since you only check for >= 1 you could have aborted the
scanning loop in the first match...

> +static int read_raw_arecont_h264_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    int ret, size, start, end, new_size = 0, i, j, k, w = 0;
> +    const uint8_t id[] = {0x2D, 0x2D, 0x66, 0x62, 0x64, 0x72};

"static", however this seems the same data (though 2 shorter).
I'd suggest defining the signature just once.

> +    uint8_t *data;
> +    int64_t pos;
> +
> +    //Extra to find the http header
> +    size = 2 * ARECONT_H264_MIME_SIZE + RAW_PACKET_SIZE;
> +    data = av_malloc(size);
> +
> +    if (av_new_packet(pkt, size) < 0)
> +        return AVERROR(ENOMEM);
> +
> +    pkt->pos = avio_tell(s->pb);
> +    pkt->stream_index = 0;
> +    pos = avio_tell(s->pb);
> +    ret = avio_read_partial(s->pb, data, size);
> +    if (ret < 0) {
> +        av_packet_unref(pkt);
> +        return ret;
> +    }
> +    if (pos <= ARECONT_H264_MIME_SIZE) {
> +        avio_seek(s->pb, 0, SEEK_SET);
> +        start = pos;
> +    } else {
> +        avio_seek(s->pb, pos - ARECONT_H264_MIME_SIZE, SEEK_SET);
> +        start = ARECONT_H264_MIME_SIZE;
> +    }
> +    ret = avio_read_partial(s->pb, data, size);
> +    if (ret < 0 || start >= ret) {
> +        av_packet_unref(pkt);
> +        return ret;
> +    }

You need to document more what you are doing here.
And even more importantly why you are using avio_read_partial.
And why you allocate both a packet and a separate "data"
with the same size.
And why not use av_get_packet.

> +            if (i >= start && j + 1 > start && j + 1 <= end) {
> +                memcpy(pkt->data + new_size, data + start, i - start);
> +                new_size += i - start;
> +                memcpy(pkt->data + new_size, data + j + 2, end - j - 1);
> +                new_size += end - j - 1;
> +                w = 1;
> +                break;
> +            } else if (i < start && j + 1 >= start && j + 1 < end) {
> +                memcpy(pkt->data + new_size, data + j + 2, end - j -1);
> +                new_size += end - j - 1;
> +                w = 1;
> +                break;
> +            } else if (j + 1 > end && i > start && i <= end) {
> +                memcpy(pkt->data + new_size, data + start, i - start);
> +                new_size += i - start;
> +                w = 1;
> +                break;
> +            }

With some comments I might be able to review without
spending a lot of time reverse-engineering this...

Best regards,
Reimar Döffinger