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

[Michael Niedermayer]

On Sun, May 08, 2011 at 07:12:36AM +0200, Reimar Döffinger wrote:
> On 8 May 2011, at 00:27, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Sat, May 07, 2011 at 09:27:00PM +0200, Reimar Döffinger wrote:
> >> 
> >> 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);
> >                     }
> 
> What about make fate for this? Doesn't this for MPEG-ES result in placing the index entries at the wrong place?

> Or did you mean to select between these two at run-time?

thats a simple possibility.


> What condition would you use? Checking for COMPLETE_FRAME will break things for parsers which do not support it correctly, which I think is most.

its a per demuxer property so a new per demuxer flag could be used


> That is why this approach, please don't take it as offence, feels like hacking around to try to avoid fixing parser output to have some sensible values.

Iam not offended by that but i think our disagreement comes from this:
I feel that frame_offset is a parser internal variable and its already
a hack to use it like is done. And now modifying this parser internal
variable to make this use work feels risky to me. Maybe it would work
out but maybe it would break some parser in some corner case.
I very much prefer to maintain 1 if() statment
than trying to change a varible that is intended to be a offset inside
a stream into a file offset and hope that nothing of the existing code
that uses it as a stream offset will break
messing with frame_offset once on parser open or after seek+flush is
safe because that just adds a offset.
when there are no demuxer packets then file offset = stream offset
and it works out but when there are demuxer packets then it doesnt and
parsers dont care about file offset at all they need stream offsets
to split packets and associate timestamps and all the other things
changing their internal variables into file offsets does not seem
safe to me

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There seems to be only one solution to NIH syndrom, ... a shooting squad
-------------- 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/baccb25d/attachment.asc>