[FFmpeg-devel] [PATCH] Fix Ogg data_offset computation.

[Reimar Döffinger]

On Thu, Nov 18, 2010 at 03:04:45PM -0800, Aaron Colwell wrote:
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 5e52bb3..1b85eeb 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -373,9 +373,14 @@ ogg_packet (AVFormatContext * s, int *str, int *dstart, int *dsize, int64_t *fpo
>          if (!os->header){
>              os->segp = segp;
>              os->psize = psize;
> -            if (!ogg->headers)
> -                s->data_offset = os->sync_pos;
> -            ogg->headers = 1;
> +            if (!ogg->headers) {
> +                if (!s->data_offset || s->data_offset > os->sync_pos)
> +                    s->data_offset = os->sync_pos;
> +                ogg->headers = 1;
> +                for (i = 0; ogg->headers && i < ogg->nstreams; i++)
> +                    if (ogg->streams[i].header > 0)
> +                        ogg->headers = 0;

Ok, let me explain some things that confuse me so about this.
The demuxer assumes that the first non-header packet means no
more header packets will follow.
Thus if this !os->header condition triggers we should not read
further in read_headers because we already have all headers.
As far as I can tell, by resetting ogg->headers that read_headers
loop might go on for a long time, and if there is another
packet from this same stream the first one will just be discarded.
I think that the code instead should be (the "if (!ogg->headers)"
condititon should no longer be needed):

av_assert0(!ogg->headers);
ogg->headers = 1;
s->data_offset = os->sync_pos;
for (i = 0; i < ogg->nstreams; i++) {
    struct ogg_stream *cur_os = ogg->streams + i;
    // all header packets must be complete before the first non-
    // header one, so everything that follows must be non-header
    cur_os->header = 0;
    // if we have a partial non-header packet, its start is
    // obviously at or after the data start
    if (cur_os->incomplete) {
        av_assert0(cur_os->sync_pos >= 0);
        s->data_offset = FFMIN(s->data_offset, cur_os->sync_pos);
    }
}

Completely untested of course.