[FFmpeg-devel] [PATCH] make analyze_duration work for streams with incomplete timestamps (mp3)

[Michael Niedermayer]

On Mon, Sep 14, 2009 at 04:00:46PM +0200, Reimar D?ffinger wrote:
> On Mon, Sep 14, 2009 at 03:24:30PM +0200, Michael Niedermayer wrote:
> > On Mon, Sep 14, 2009 at 10:07:56AM +0200, Reimar D?ffinger wrote:
> > > On Mon, Sep 14, 2009 at 01:55:06AM +0200, Michael Niedermayer wrote:
> > > > the correct solution IMHO is to set duration, and actually i would naively
> > > > expect compute_frame_duration)()/get_audio_frame_size() to do so, but it seems
> > > > that its not otherwise this issue wouldnt exist ...
> > > 
> > > I think the correct solution is for the parser to set pts/dts correctly,
> > > but I don't know how to do that and I don't want to wait for someone to
> > > do it (because I have the impression it is expected behaviour for
> > > parsers to just _SUCK_ (yes, in uppercase)).
> > 
> > i have more the impression people call everything they dont understand to
> > be broken and to _SUCK_ granted its missing any and all documentation but
> > still ...
> 
> documentation is missing and very few people know how it is supposed to
> work, making it almost certain the behaviour is not even consistent.
> 
> > the next step these people do is fix that brokenness (1, see below)
> > (1) if you fix it as you suggest by setting dts in the parser, duration
> > still would not be set correctly and you would then require a parser for
> > pretty much every codec, let me be blunt but that _SUCKS_ more
> 
> I was biased by the fact that changing this would also reduce the issue
> with using AVParser in MPlayer. AFAICT you can't really use AVParser
> well unless combine it with a way to also compute additional PTS/DTS values.
> But unless you do that by decoding, the code for that is in libavformat
> and AVParser is in libavcodec. There is no question I am rather clueless
> about all this, but it just feels wrong and needlessly hard to use.

i think external use of just AVParsers without
libavformat was not considered back then ...,
that would require some code factoring. I do think this would
be welcome, seperating the parser related timestamp computation stuff out
surely would help readability of the whole ...


> 
> > back to the topic
> > compute_pkt_fields() sets missing dts/pts for codecs that dont use frame
> > reordering based on duration, not the parser. This is quite a bit simpler
> > than reimplementing dts/pts advancing in the parsers, especially as that
> > would require a parser for most codecs ...
> > now iam still wondering why duration is not set ?
> 
> I could maybe help answering that if you gave a hint why it should be
> set. I mean, it's not some kind of natural law that duration gets
> magically set - and AFAICT those packets are fresh out of the parser and
> just don't have all that stuff set, where should the duration have come
> from (should that one be set the parser? I couldn't find a way API-wise
> to do that, but I might be blind).

Ive not looked at the issue here but as said i would naively expect
compute_pkt_fields()/compute_frame_duration()/get_audio_frame_size()
to set the duration
compute_pkt_fields() is called after the parser in 
av_read_frame_internal() before the packets are given to the user ...
well its obvious i must be missing something ...
one way it could fail is if frame_size isnt set for some reason ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090915/0da56f91/attachment.pgp>