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

[Swaraj Hota]

On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote:
> 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.
> 

That will be another way to do it. I'll change it then.

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

Okay that could be done.

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

No particular reason. Essentially all are just skips. There is no
backward seek. I left two seeks becuase they seemed more readable.
Someone could know at a glance at what offset the first video and audio
index are assumed/found to be. Should I change them to skips as well?

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

Yeah it is indeed inefficient. But it also seems like the "correct" one.
Because in case of seeking we might not be at the boundary of a frame
and hence might need to skip to the boundary of next frame we can find.
I guess this rules out binary search, and maybe also saving the last
read. 

Regarding the frame_offset corruption, well that rules out binary search
as well because then the order of the index will be disturbed.

Or maybe I misunderstood? Please do mention if this can be done more
efficiently by some method. I really need some ideas on this if it can
be done.

> > +    av_freep(&ifv->vid_index);
> > +    if (ifv->is_audio_present)
> > +        av_freep(&ifv->aud_index);
> 
> Shouldn't the if () be unnecessary?
> 

Yeah I guess it is unnecessary. I'll change it.


Thanks for the review!