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.
> + 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.
> + avio_skip(s->pb, 0x5c);
If for any of these skips you have any idea what data they
contain, it would be nice to document it.
> + if (aud_magic == MKTAG('G','R','A','W')) {
> + ifv->is_audio_present = 1;
> + } else if (aud_magic == MKTAG('P','C','M','U')) {
> + ifv->is_audio_present = 0;
> + } else {
> + avpriv_request_sample(s, "Unknown audio codec %x\n", aud_magic);
> + }
Why does PCMU mean "no audio"? Could you add a comment?
Also, wouldn't it be more consistent to explicitly set
is_audio_present in the "else" case?
> + /*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?
> + 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.
> + av_freep(&ifv->vid_index);
> + if (ifv->is_audio_present)
> + av_freep(&ifv->aud_index);
Shouldn't the if () be unnecessary?
Best regards,
Reimar Döffinger
_______________________________________________
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".
