[FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files

[Reimar Döffinger]

Hello!
Nothing major, but a few comments on things that might make
sense to polish below.

On Sat, May 04, 2019 at 06:42:40PM +0530, Swaraj Hota wrote:
> +#define IFV_MAGIC "\x11\xd2\xd3\xab\xba\xa9\xcf\x11\
> +\x8e\xe6\x00\xc0\x0c\x20\x53\x65\x44"

> +    if (!memcmp(p->buf, IFV_MAGIC, 17))

Using an array and sizeof() instead of the hard-coded 17 might be a bit
nicer.

> +    int ret;
> +
> +    if (frame_type == AVMEDIA_TYPE_VIDEO) {
> +        ret = av_reallocp_array(&ifv->vid_index, ifv->total_vframes, sizeof(IFVIndexEntry));
> +        if (ret < 0)
> +            return ret;
> +
> +    } else if (frame_type == AVMEDIA_TYPE_AUDIO) {
> +        ret = av_reallocp_array(&ifv->aud_index, ifv->total_aframes, sizeof(IFVIndexEntry));
> +        if (ret < 0)
> +            return ret;
> +    }

Could just declare "ret" right where it is assigned.

> +    avio_skip(s->pb, 0x5c);

If for any of these skips you have any idea what data they
contain, it would be nice to document it.

> +    if (aud_magic == MKTAG('G','R','A','W')) {
> +        ifv->is_audio_present = 1;
> +    } else if (aud_magic == MKTAG('P','C','M','U')) {
> +        ifv->is_audio_present = 0;
> +    } else {
> +        avpriv_request_sample(s, "Unknown audio codec %x\n", aud_magic);
> +    }

Why does PCMU mean "no audio"? Could you add a comment?
Also, wouldn't it be more consistent to explicitly set
is_audio_present in the "else" case?

> +    /*read video index*/
> +    avio_seek(s->pb, 0xf8, SEEK_SET);
[...]
> +    avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb));

Why use avio_seek in one place and avio_skip in the other?

> +    pos = avio_tell(s->pb);
> +
> +    for (i = 0; i < ifv->total_vframes; i++) {
> +        e = &ifv->vid_index[i];
> +        if (e->frame_offset >= pos)
> +            break;
> +    }

This looks rather inefficient.
Wouldn't it make more sense to either
use a binary search or at least to
remember the position from the last read?
This also does not seem very robust either,
if a single frame_offset gets corrupted
to a very large value, this code will
never be able to find the "correct" position.
It seems to assume the frame_offset
is ordered increasingly (as would be needed for
binary search), but that property isn't
really checked/enforced.

> +    av_freep(&ifv->vid_index);
> +    if (ifv->is_audio_present)
> +        av_freep(&ifv->aud_index);

Shouldn't the if () be unnecessary?

Best regards,
Reimar Döffinger