[FFmpeg-devel] [PATCH] Only using st->parser->pos when doing repacking in the parser.

[Reimar Döffinger]

On Tue, Apr 26, 2011 at 06:52:11PM +0200, Michael Niedermayer wrote:
> On Mon, Apr 25, 2011 at 11:41:54PM +0200, Reimar Döffinger wrote:
> > On 25 Apr 2011, at 22:56, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > On Mon, Apr 25, 2011 at 12:34:12AM +0200, Reimar Döffinger wrote:
> > >> On Sun, Apr 24, 2011 at 11:50:15PM +0200, Reimar Döffinger wrote:
> > >>> On Sun, Apr 24, 2011 at 11:28:42PM +0200, Michael Niedermayer wrote:
> > >>>> On Sun, Apr 24, 2011 at 06:17:14PM +0200, Reimar Döffinger wrote:
> > >>>>> ---
> > >>>>> libavformat/utils.c |    6 +++++-
> > >>>>> 1 files changed, 5 insertions(+), 1 deletions(-)
> > >>>>> 
> > >>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
> > >>>>> index e7ce911..d2b8fc2 100644
> > >>>>> --- a/libavformat/utils.c
> > >>>>> +++ b/libavformat/utils.c
> > >>>>> @@ -1069,7 +1069,11 @@ static int av_read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> > >>>>>                     pkt->stream_index = st->index;
> > >>>>>                     pkt->pts = st->parser->pts;
> > >>>>>                     pkt->dts = st->parser->dts;
> > >>>>> -                    pkt->pos = st->parser->pos;
> > >>>>> +                    // When not repacking, using parser pos can at best break
> > >>>>> +                    // things since parsers are not designed to handle the
> > >>>>> +                    // case where current packet pos + size < next packet pos
> > >>>>> +                    if (st->needs_parsing == AVSTREAM_PARSE_FULL)
> > >>>>> +                        pkt->pos = st->parser->pos;
> > >>>> 
> > >>>> i think this should also check for AVSTREAM_PARSE_TIMESTAMPS
> > >>> 
> > >>> Hm, I think it might make most sense to check for
> > >>> st->parser->flags & PARSER_FLAG_COMPLETE_FRAMES
> > >> 
> > >> Doesn't work, causes the pos to be too far ahead for
> > >> DNXHD.
> > > 
> > > why dont you use st->parser->pos / pkt->pos in the
> > > av_add_index_entry() call ?
> > 
> > That will still leave pkt->pos as complete nonsense afterwards
> 
> > and for cases where the parser adds a delay (as the dnxhd case) it would put the frame _after_ the keyframe into the index.
> 
> are the timestamps correct for this case?
> because if they are also delayed by 1 and incorrect the problem might
> be elsewhere

I can't be bothered to try to come up with a way to actually verify it,
but if the parser delays all packets by one the timestamps would of
course also be wrong if we used the timestamps from the packets that
go _into_ the parser instead of using the timestamps that _come out_
of the parser.
But regardless of that, bypassing the parser for only half of the
data is just plain bad design IMO.
We can't bypass the parser completely since then we do not know
which frames are I-frames in some cases.
Thus I think there is no way around fixing the parsers to at
least not mangle the ->pos completely beyond recognition.