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!
_______________________________________________
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".
