On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote:
> On 07.05.2019, at 12:00, Swaraj Hota <swarajhota...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
