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

[Reimar Döffinger]

On 07.05.2019, at 12:00, Swaraj Hota <swarajhota353 at gmail.com> wrote:

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

Not quite sure how things work nowadays, but I'd suggest to use whichever
gives the most readable code.
Which would mean using avio_seek in this case.

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

First, seeking should be handled specially, by resetting the state.
You should not make the get_packet less efficient because of that.
That should enable the "remember last position and start from there".

As to the corruption case, well the question is what to do about that, and I don't have the answer.
But if the solution were to e.g. ensure the frame offset is monotonous then binary search could be used.
However there is also the possibility that the format does in fact allow a completely arbitrary order of frames in the file, maybe even re-using an earlier frame_offset if the same frame appears multiple times.
In that case this whole offset-based positioning code would simply be wrong, and you'd have to store the current index position in your demuxer instead of relying on avio_tell.
Maybe you chose this solution because you did not know that seeking should be implemented via special functions?