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

[Reimar Döffinger]

On Sat, May 07, 2011 at 09:12:22PM +0200, Michael Niedermayer wrote:
> On Sat, May 07, 2011 at 06:02:59PM +0200, Reimar Döffinger wrote:
> > 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.
> 
> Could you explain how to reproduce the problem these patches fix, so
> i can take a look?

Oh, of course, didn't realize I never explained that properly.
See the mail I just sent: "[PATCH] Add generic index support to ea demuxer."