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

Fixes ticket #2956.

Signed-off-by: Swaraj Hota <swarajhota353@gmail.com>
---
Revised patch. Made some minor changes based on original player:
- Removed incorrect reading of frame_rate, instead frame rate
is kept fixed at 25 (seems like this value is always same).
- Added reading of frame width and height from input file.

---
 Changelog                |   1 +
 libavformat/Makefile     |   1 +
 libavformat/allformats.c |   1 +
 libavformat/ifv.c        | 335 +++++++++++++++++++++++++++++++++++++++
 libavformat/version.h    |   4 +-
 5 files changed, 340 insertions(+), 2 deletions(-)
 create mode 100644 libavformat/ifv.c

On 5/4/19, Swaraj Hota <swarajhota353@gmail.com> wrote:
> Fixes ticket #2956.
>
> Signed-off-by: Swaraj Hota <swarajhota353@gmail.com>
> ---
> Revised patch. Made some minor changes based on original player:
> - Removed incorrect reading of frame_rate, instead frame rate
> is kept fixed at 25 (seems like this value is always same).
> - Added reading of frame width and height from input file.
>

Looks good, I gonna apply it in next few days.

Okay. Thanks!

On 5/4/19, Swaraj Hota <swarajhota353@gmail.com> wrote:
> Okay. Thanks!
>

Sorry, but I found some issues while testing your patch.

Why are there no timestamps?

Seeking with mpv/ffplay caused hard crash.

You do not handle EOF, so ffmpeg with 1111.ivf never exits.

Some more or less important issues found:

On 5/4/19, Swaraj Hota <swarajhota353@gmail.com> wrote:
> Fixes ticket #2956.
>
> Signed-off-by: Swaraj Hota <swarajhota353@gmail.com>
> ---
> Revised patch. Made some minor changes based on original player:
> - Removed incorrect reading of frame_rate, instead frame rate
> is kept fixed at 25 (seems like this value is always same).
> - Added reading of frame width and height from input file.
>
> ---
>  Changelog                |   1 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/ifv.c        | 335 +++++++++++++++++++++++++++++++++++++++
>  libavformat/version.h    |   4 +-
>  5 files changed, 340 insertions(+), 2 deletions(-)
>  create mode 100644 libavformat/ifv.c
>
> diff --git a/Changelog b/Changelog
> index a3fa0c14a2..03d2a9db01 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -26,6 +26,7 @@ version <next>:
>  - lscr decoder
>  - lagfun filter
>  - asoftclip filter
> +- IFV demuxer
>
>
>  version 4.1:
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 99be60d184..f68d41e4a5 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -231,6 +231,7 @@ OBJS-$(CONFIG_ICO_MUXER)                 += icoenc.o
>  OBJS-$(CONFIG_IDCIN_DEMUXER)             += idcin.o
>  OBJS-$(CONFIG_IDF_DEMUXER)               += bintext.o sauce.o
>  OBJS-$(CONFIG_IFF_DEMUXER)               += iff.o
> +OBJS-$(CONFIG_IFV_DEMUXER)               += ifv.o
>  OBJS-$(CONFIG_ILBC_DEMUXER)              += ilbc.o
>  OBJS-$(CONFIG_ILBC_MUXER)                += ilbc.o
>  OBJS-$(CONFIG_IMAGE2_DEMUXER)            += img2dec.o img2.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index d316a0529a..cd00834807 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -188,6 +188,7 @@ extern AVOutputFormat ff_ico_muxer;
>  extern AVInputFormat  ff_idcin_demuxer;
>  extern AVInputFormat  ff_idf_demuxer;
>  extern AVInputFormat  ff_iff_demuxer;
> +extern AVInputFormat  ff_ifv_demuxer;
>  extern AVInputFormat  ff_ilbc_demuxer;
>  extern AVOutputFormat ff_ilbc_muxer;
>  extern AVInputFormat  ff_image2_demuxer;
> diff --git a/libavformat/ifv.c b/libavformat/ifv.c
> new file mode 100644
> index 0000000000..f2d12b4fe7
> --- /dev/null
> +++ b/libavformat/ifv.c
> @@ -0,0 +1,335 @@
> +/*
> + * IFV demuxer
> + *
> + * Copyright (c) 2019 Swaraj Hota
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +#include "avformat.h"
> +#include "internal.h"
> +#include "avio_internal.h"
> +
> +#define IFV_MAGIC "\x11\xd2\xd3\xab\xba\xa9\xcf\x11\
> +\x8e\xe6\x00\xc0\x0c\x20\x53\x65\x44"
> +
> +typedef struct IFVIndexEntry {
> +    uint32_t frame_offset;
> +    uint32_t frame_size;
> +    enum AVMediaType frame_type;
> +} IFVIndexEntry;
> +
> +typedef struct IFVContext {
> +    IFVIndexEntry *vid_index;
> +    IFVIndexEntry *aud_index;
> +    uint32_t index_pos;
> +    uint32_t total_vframes;
> +    uint32_t total_aframes;
> +
> +    int width, height;
> +    int is_audio_present;
> +    int sample_rate;
> +
> +    int video_stream_index;
> +    int audio_stream_index;
> +} IFVContext;
> +
> +static int ifv_probe(const AVProbeData *p)
> +{
> +    if (!memcmp(p->buf, IFV_MAGIC, 17))
> +        return AVPROBE_SCORE_MAX;
> +    return 0;
> +}
> +
> +static int read_next_index_entry(AVFormatContext *s, enum AVMediaType
> frame_type)
> +{
> +    IFVContext *ifv = s->priv_data;
> +    IFVIndexEntry *e;
> +
> +    if (frame_type == AVMEDIA_TYPE_VIDEO) {
> +        if (ifv->index_pos >= ifv->total_vframes)
> +            return 0;
> +
> +        e = &ifv->vid_index[ifv->index_pos++];
> +
> +        e->frame_offset = avio_rl32(s->pb);
> +        e->frame_size = avio_rl32(s->pb);
> +        e->frame_type = frame_type;
> +
> +        avio_skip(s->pb, 20);

Are PTS stored here?

> +
> +    } else if (frame_type == AVMEDIA_TYPE_AUDIO) {
> +        if (ifv->index_pos >= ifv->total_aframes)
> +            return 0;
> +
> +        e = &ifv->aud_index[ifv->index_pos++];
> +
> +        e->frame_offset = avio_rl32(s->pb);
> +        e->frame_size = avio_rl32(s->pb);
> +        e->frame_type = frame_type;
> +
> +        avio_skip(s->pb, 16);


Are PTS stored here?

> +    }
> +
> +    return 1;
> +}
> +
> +static int read_index(AVFormatContext *s, enum AVMediaType frame_type)
> +{
> +    IFVContext *ifv = s->priv_data;
> +    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;
> +    }
> +
> +    while (read_next_index_entry(s, frame_type))
> +        ;
> +
> +    return 0;
> +}
> +
> +static int parse_header(AVFormatContext *s)
> +{
> +    IFVContext *ifv = s->priv_data;
> +    uint32_t aud_magic;
> +    uint32_t vid_magic;
> +
> +    avio_skip(s->pb, 0x5c);
> +    ifv->width = avio_rl16(s->pb);
> +    ifv->height = avio_rl16(s->pb);
> +
> +    avio_skip(s->pb, 0x8);
> +    vid_magic = avio_rl32(s->pb);
> +
> +    if (vid_magic != MKTAG('H','2','6','4'))
> +        avpriv_request_sample(s, "Unknown video codec %x\n", vid_magic);
> +
> +    avio_skip(s->pb, 0x2c);
> +    ifv->sample_rate = avio_rl32(s->pb);
> +    aud_magic = avio_rl32(s->pb);
> +
> +    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);
> +    }
> +
> +    avio_skip(s->pb, 0x44);
> +    ifv->total_vframes = avio_rl32(s->pb);
> +    ifv->total_aframes = avio_rl32(s->pb);
> +
> +    return 0;
> +}
> +
> +static int ifv_read_header(AVFormatContext *s)
> +{
> +    IFVContext *ifv = s->priv_data;
> +    AVStream *st;
> +    int ret;
> +
> +    ret = parse_header(s);
> +    if (ret < 0)
> +        return ret;
> +
> +    ifv->vid_index = NULL;
> +    ifv->aud_index = NULL;
> +
> +    /*read video index*/
> +    avio_seek(s->pb, 0xf8, SEEK_SET);
> +
> +    ifv->index_pos = 0;
> +    ret = read_index(s, AVMEDIA_TYPE_VIDEO);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (ifv->is_audio_present) {
> +        /*read audio index*/
> +        avio_seek(s->pb, 0x14918, SEEK_SET);
> +
> +        ifv->index_pos = 0;
> +        ret = read_index(s, AVMEDIA_TYPE_AUDIO);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +
> +    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;
> +
> +    avpriv_set_pts_info(st, 32, 1, 25);
> +
> +    if (ifv->is_audio_present) {
> +        st = avformat_new_stream(s, NULL);
> +
> +        if (!st)
> +            return AVERROR(ENOMEM);
> +
> +        st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> +        st->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE;
> +        st->codecpar->channels = 1;
> +        st->codecpar->channel_layout = AV_CH_LAYOUT_MONO;
> +        st->codecpar->sample_rate = ifv->sample_rate;
> +        ifv->audio_stream_index = st->index;
> +    }
> +
> +    avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb));
> +
> +    return 0;
> +}
> +
> +static int ifv_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    IFVContext *ifv = s->priv_data;
> +    IFVIndexEntry *e;
> +    uint32_t pos, nb_new_vframes, nb_new_aframes, new_index_start;
> +    int ret, i, j, vdiff, adiff;
> +
> +    vdiff = adiff = INT_MAX;
> +
> +    pos = avio_tell(s->pb);
> +
> +    for (i = 0; i < ifv->total_vframes; i++) {
> +        e = &ifv->vid_index[i];
> +        if (e->frame_offset >= pos)
> +            break;
> +    }
> +
> +    vdiff = e->frame_offset - pos;
> +
> +    if (ifv->is_audio_present && vdiff) {
> +        for (j = 0; j < ifv->total_aframes; j++) {
> +            e = &ifv->aud_index[j];
> +            if (e->frame_offset >= pos)
> +                break;
> +        }
> +
> +        adiff = e->frame_offset - pos;
> +    }
> +
> +    if (vdiff >= 0 && adiff >= 0) {
> +        /*skip to the boundary of next frame if needed*/
> +        avio_skip(s->pb, FFMIN(vdiff, adiff));
> +        if (vdiff && adiff)
> +            return 0;
> +    }
> +
> +    if (i == ifv->total_vframes) {
> +        if (ifv->is_audio_present && j == ifv->total_aframes) {
> +            /*read new video and audio indexes*/
> +
> +            avio_skip(s->pb, 0x1c);
> +            nb_new_vframes = avio_rl32(s->pb);
> +            nb_new_aframes = avio_rl32(s->pb);
> +            avio_skip(s->pb, 0xc);
> +
> +            if (avio_feof(s->pb))
> +                return AVERROR_EOF;
> +
> +            ifv->index_pos = new_index_start = ifv->total_vframes;
> +            ifv->total_vframes += nb_new_vframes;
> +
> +            ret = read_index(s, AVMEDIA_TYPE_VIDEO);
> +            if (ret < 0)
> +                return ret;
> +
> +            ifv->index_pos = ifv->total_aframes;
> +            ifv->total_aframes += nb_new_aframes;
> +
> +            ret = read_index(s, AVMEDIA_TYPE_AUDIO);
> +            if (ret < 0)
> +                return ret;
> +
> +            pos = avio_tell(s->pb);
> +            avio_skip(s->pb, ifv->vid_index[new_index_start].frame_offset -
> pos);
> +
> +            return 0;
> +
> +        } else if (!ifv->is_audio_present) {
> +            /*read new video index*/
> +
> +            avio_skip(s->pb, 0x1c);
> +            nb_new_vframes = avio_rl32(s->pb);
> +            avio_skip(s->pb, 0x10);
> +
> +            if (avio_feof(s->pb))
> +                return AVERROR_EOF;
> +
> +            ifv->index_pos = new_index_start = ifv->total_vframes;
> +            ifv->total_vframes += nb_new_vframes;
> +
> +            ret = read_index(s, AVMEDIA_TYPE_VIDEO);
> +            if (ret < 0)
> +                return ret;
> +
> +            pos = avio_tell(s->pb);
> +            avio_skip(s->pb, ifv->vid_index[new_index_start].frame_offset -
> pos);
> +
> +            return 0;
> +        }
> +    }
> +
> +    ret = av_get_packet(s->pb, pkt, e->frame_size);
> +    if (ret < 0)
> +        return ret;
> +
> +    pkt->stream_index = e->frame_type == AVMEDIA_TYPE_VIDEO?
> +                        ifv->video_stream_index: ifv->audio_stream_index;
> +
> +    return ret;
> +}
> +
> +static int ifv_read_close(AVFormatContext *s)
> +{
> +    IFVContext *ifv = s->priv_data;
> +
> +    av_freep(&ifv->vid_index);
> +    if (ifv->is_audio_present)
> +        av_freep(&ifv->aud_index);
> +
> +    return 0;
> +}
> +
> +AVInputFormat ff_ifv_demuxer = {
> +    .name           = "ifv",
> +    .long_name      = NULL_IF_CONFIG_SMALL("IFV CCTV DVR"),
> +    .priv_data_size = sizeof(IFVContext),
> +    .extensions     = "ifv",
> +    .read_probe     = ifv_probe,
> +    .read_header    = ifv_read_header,
> +    .read_packet    = ifv_read_packet,
> +    .read_close     = ifv_read_close,
> +    .flags          = AVFMT_NOTIMESTAMPS,
> +};
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 150a72e27d..52dd95f5c6 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,8 +32,8 @@
>  // Major bumping may affect Ticket5467, 5421, 5451(compatibility with
> Chromium)
>  // Also please add any ticket numbers that you believe might be affected
> here
>  #define LIBAVFORMAT_VERSION_MAJOR  58
> -#define LIBAVFORMAT_VERSION_MINOR  27
> -#define LIBAVFORMAT_VERSION_MICRO 103
> +#define LIBAVFORMAT_VERSION_MINOR  28
> +#define LIBAVFORMAT_VERSION_MICRO 100
>
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>                                                 LIBAVFORMAT_VERSION_MINOR, \
> --
> 2.21.0
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


You solely use index for getting packets?

avformat have code to store index too you should try using those.

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

Am So., 5. Mai 2019 um 21:59 Uhr schrieb Reimar Döffinger
<Reimar.Doeffinger@gmx.de>:

> > +    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.

Iirc, all values we understood are used now.

> > +    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"?

We have several files with "PCMU" and several files with "GRAW".
All files with GRAW contain audio data, none of the files with PCMU
contain audio data.
I originally thought that the two bytes at 0x40/0x41 (iirc) show number
of streams but a test with the proprietary player confirmed that this
is not correct.
Another test would be to overwrite GRAW with PCMU to find out if
audio is still played...

Thank you for your comments, Carl Eugen

On Sun, May 05, 2019 at 10:05:17PM +0200, Carl Eugen Hoyos wrote:
> Am So., 5. Mai 2019 um 21:59 Uhr schrieb Reimar Döffinger
> <Reimar.Doeffinger@gmx.de>:
> > > +    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"?
>
> We have several files with "PCMU" and several files with "GRAW".
> All files with GRAW contain audio data, none of the files with PCMU
> contain audio data.
> I originally thought that the two bytes at 0x40/0x41 (iirc) show number
> of streams but a test with the proprietary player confirmed that this
> is not correct.
> Another test would be to overwrite GRAW with PCMU to find out if
> audio is still played...

Ok, makes sense.
I think it's good (and good enough) to have a comment that this
is just heuristics/guesswork based on existing samples and
might be quite wrong.

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!

On 07.05.2019, at 12:00, Swaraj Hota <swarajhota353@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?

Am Mi., 8. Mai 2019 um 00:52 Uhr schrieb Reimar Döffinger
<Reimar.Doeffinger@gmx.de>:
>
> On 07.05.2019, at 12:00, Swaraj Hota <swarajhota353@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.

I originally suggested using avio_skip() instead of avio_seek() to clarify
that no back-seeking is involved.
I realize it may not have been the best approach...

Thank you for your comments, Carl Eugen

On 08.05.2019, at 08:01, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Mi., 8. Mai 2019 um 00:52 Uhr schrieb Reimar Döffinger
> <Reimar.Doeffinger@gmx.de>:
>> 
>> On 07.05.2019, at 12:00, Swaraj Hota <swarajhota353@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.
> 
> I originally suggested using avio_skip() instead of avio_seek() to clarify
> that no back-seeking is involved.
> I realize it may not have been the best approach...

Well, it is good advice in principle, but in this case it seems to lead to ugly code (and it was only done in some cases).
That kind of thing tends to be hard to know before making the change.

On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote:
> On 07.05.2019, at 12:00, Swaraj Hota <swarajhota353@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.

On Wed, May 08, 2019 at 03:06:37PM +0530, Swaraj Hota wrote:
> On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote:
> > 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)?

Yes.

> If yes then am I mistaken that FFmpeg can also handle seeking
> automatically? (Carl suggesting something like that, iirc)

It has some functionality, but I think in practice I don't think
you can get a proper fully working solution with it.

> 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.

It's a bit more complex (in some aspect, simpler in others), but
gxf.c might be an example.

I will describe what I think would be a correct full implementation,
but I will also note that maybe this is asking a bit much.
You would start with changing your index reading function to
use av_add_index_entry instead of storing your own index.
In read_packet you simply remember your position in that index and
use that information to know where to read from the next time.
This should work for linear playback.

Once that is done, read_seek would need implementing
(it seems read_seek2 is currently only implemented by subtitle
formats?).
There you would seek the index for the right position
(av_index_search_timestamp), and set the variables you
use in read_packet accordingly so you will read the proper
frames next.
If the desired position is not in the index, you
will also need to read the next index entries from
the file (to start with you could do that by calling
read_packet, even though it is a bit overkill).

Unfortunately this is all a bit complicated and a
lot of things that need to be implemented right.
The "quick fix" would be to implement a read_seek
function that only returns -1, with that you
continue to use the current seeking code, but
it would allow you to tell the read_packet
function to use the "search index position based
on file position" code, whereas for normal
playback you just continue reading whatever
you stored as "next index position to read".

I'm sure far from everything I wrote is clear,
but hopefully it gives you some starting points.

Best regards,
Reimar Döffinger

On Wed, May 08, 2019 at 09:28:33PM +0200, Reimar Döffinger wrote:
> On Wed, May 08, 2019 at 03:06:37PM +0530, Swaraj Hota wrote:
> > On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote:
> > > 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)?
> 
> Yes.
> 
> > If yes then am I mistaken that FFmpeg can also handle seeking
> > automatically? (Carl suggesting something like that, iirc)
> 
> It has some functionality, but I think in practice I don't think
> you can get a proper fully working solution with it.
> 
> > 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.
> 
> It's a bit more complex (in some aspect, simpler in others), but
> gxf.c might be an example.
> 
> I will describe what I think would be a correct full implementation,
> but I will also note that maybe this is asking a bit much.
> You would start with changing your index reading function to
> use av_add_index_entry instead of storing your own index.
> In read_packet you simply remember your position in that index and
> use that information to know where to read from the next time.
> This should work for linear playback.
> 
> Once that is done, read_seek would need implementing
> (it seems read_seek2 is currently only implemented by subtitle
> formats?).
> There you would seek the index for the right position
> (av_index_search_timestamp), and set the variables you
> use in read_packet accordingly so you will read the proper
> frames next.
> If the desired position is not in the index, you
> will also need to read the next index entries from
> the file (to start with you could do that by calling
> read_packet, even though it is a bit overkill).
> 
> Unfortunately this is all a bit complicated and a
> lot of things that need to be implemented right.
> The "quick fix" would be to implement a read_seek
> function that only returns -1, with that you
> continue to use the current seeking code, but
> it would allow you to tell the read_packet
> function to use the "search index position based
> on file position" code, whereas for normal
> playback you just continue reading whatever
> you stored as "next index position to read".
> 
> I'm sure far from everything I wrote is clear,
> but hopefully it gives you some starting points.
> 
> Best regards,
> Reimar Döffinger

That was really helpful! Thanks a lot!
It is more or less clear to me now. Okay then I will surely try the
"complicated" one first, and fallback to the "quick fix" otherwise.
Really sorry if I am asking a lot of questions here ':D

Swaraj