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

[Michael Niedermayer]

On Sat, May 07, 2011 at 09:27:00PM +0200, Reimar Döffinger wrote:
> 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."

the ea case is fixed by my suggestion of
index 4a4141e..c60d27f 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1103,7 +1103,7 @@ static int av_read_frame_internal(AVFormatContext *s, AVPacket *pkt)

                     if((s->iformat->flags & AVFMT_GENERIC_INDEX) && pkt->flags & AV_PKT_FLAG_KEY){
                         ff_reduce_index(s, st->index);
-                        av_add_index_entry(st, st->parser->frame_offset, pkt->dts,
+                        av_add_index_entry(st, pkt->pos, pkt->dts,
                                            0, 0, AVINDEX_KEYFRAME);
                     }

----------------
there are 2 case
1, where there are packets at the (de)muxer level, in which case
   seeking has to happen to these packets, the output of the parsers
   could be used to skip each stream to the next keyframe. But it
   cant be used as position to seek to at the underlaying file io level
   example MPEG-PS & ea
2. where there are no such packets and just 1 stream.
   This can be handled like 1. Or its possible to peek in the parser
   and directly do a file io seek to the keyframe.
   example MPEG-ES

the current code does not support 1.
my suggested change above is part of what would be needed to support 1.




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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20110508/dcd1b948/attachment.asc>