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

[Shivam Goyal]

On 09-05-2019 01:15, Reimar Döffinger wrote:

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

Okay, I would change it on next version, Yeah it is '--fbdr\r\n' The
file from Arecont h264 contains http header starting from this, many
times. 

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

Yeah, this would be a great idea. I will change this. 

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

I saw the raw video demuxer, there it was using avio_read_partial. 
Because it is allowed to read less data then specified.  

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

Okay, would add comments to this also. 
I also have another idea to solve this findind and removing the http
header, but it would require to change RAW_PACKET_SIZE. So, just want to
know, can i change this? if yes, then how much can i change this? 

Thanks for the review, 
Shivam Goyal