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

[Swaraj Hota]

On Tue, May 28, 2019 at 11:30:13PM +0200, Reimar Döffinger wrote:
> Hi!
> Did you intentionally not send to the list?

Okay I just realised I have been replying personally to all comments
whereas I should have sent them to the list ':D

> 
> On 28.05.2019, at 17:32, Swaraj Hota <swarajhota353 at gmail.com> wrote:
> 
> > On Sun, May 26, 2019 at 09:44:35PM +0200, Reimar Döffinger wrote:
> >> On Sun, May 26, 2019 at 01:46:32AM +0530, Swaraj Hota wrote:
> >>> +    st = avformat_new_stream(s, NULL);
> >>> +    if (!st)
> >>> +        return AVERROR(ENOMEM);
> >>> +
> >>> +    st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> >>> +    st->codecpar->codec_id = AV_CODEC_ID_H264;
> >>> +    st->codecpar->width = ifv->width;
> >>> +    st->codecpar->height = ifv->height;
> >>> +    st->start_time = 0;
> >>> +    ifv->video_stream_index = st->index;
> >> 
> >> I suspect that it would have been ok to
> >> just assume/assert this will always be 0.
> >> 
> > 
> > Okay, but I didn't get what exact change are you suggesting? Should 
> > I replace st->index with 0 here? Or should I get rid of
> > ifv->video/audio_stream_index variables and instead use 0 and 1
> > everywhere?
> 
> I think it's fine, now that you already wrote the code.
> Just saying that you probably didn't really have to implement quite as much.
> 

Will keep that in mind :)

> >>> +static int ifv_read_packet(AVFormatContext *s, AVPacket *pkt)
> >> 
> >> As far as I can tell, you just choose between audio
> >> and video by the closest timestamp.
> > 
> > Yes.
> > 
> >> While this might be ok considering the limited importance
> >> and uses of the format, it is rather simplistic.
> >> I do not know if it follows the latest best practices,
> >> but mov_find_next_sample is an example of a more
> >> sophisticated way.
> >> It takes into account whether the underlying transport
> >> has issues seeking (like http, or even worse piped input)
> >> and in that case prefers file position over timestamp
> >> position.
> >> It also avoids wasting time reading streams marked as
> >> AVDISCARD_ALL (in mov_read_packet is that check).
> >> Maybe others following the project more closely
> >> can give additional/better best practice tips.
> > 
> > Okay, I get that, but as you said considering the limited importance and
> > uses of the format, we must also consider if it is worth all the effort,
> > cuz it can also get more messy ':D Besides I have been working on this
> > format since over a month or two now, and understanding, writing and debugging 
> > this change might take quite some time ':D
> > 
> > I have been trying to comply with all good practices but I think a working 
> > demuxer is quite enough for a minority format like this (e.g. even the
> > dhav format is quite simplistic), but again I can't say that, 
> > if others agree that it is a necessity I will work on it.
> 
> Entirely depends on the purpose.
> If the aim is to have a working demuxer, I think it is acceptable from what I looked at.
> I guess the only concern might be that not supporting piped input would be a bit of a regression from the first patch.

Is there another simpler way to support piped input? Can you explain a
bit?

> If the aim is to learn and possibly tackle more involved formats it might be useful. But even then maybe rather as a separate patch later...
> 

Actually another reason is that I now have to focus on my gsoc project,
so yeah, a separate patch later seems like a good idea to me :)

Thank you.

Swaraj