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

[Swaraj Hota]

On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote:
> 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?

By "special functions" do you mean those "read_seek" functions that
are present in many demuxers(Cuz I have not implemented that)? 
If yes then am I mistaken that FFmpeg can also handle seeking
automatically? (Carl suggesting something like that, iirc)

Keeping the corruption case aside, how do you suggest I implement this?
Is it not required to skip bytes till the next frame boundary in the
read_packet function (as done in the current patch) ?
If not then I guess I can go with a binary search, or better yet
remember the last position.

Thank you.