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

[Michael Niedermayer]

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?



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110507/d8796f5e/attachment.asc>