[FFmpeg-devel] [PATCH] Implement guessed_pts in avcodec_decode_video2.

[Michael Niedermayer]

On Mon, Jan 31, 2011 at 11:54:26PM -0500, Alexander Strange wrote:
> 
> On Jan 30, 2011, at 2:28 PM, Nicolas George wrote:
> 
> > 
> > Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> > ---
> > cmdutils.c           |   27 ---------------------------
> > cmdutils.h           |   24 ------------------------
> > ffmpeg.c             |   14 ++++++--------
> > ffplay.c             |   10 +++-------
> > libavcodec/avcodec.h |   20 ++++++++++++++++++++
> > libavcodec/utils.c   |   43 ++++++++++++++++++++++++++++++++++++++++++-
> > 6 files changed, 71 insertions(+), 67 deletions(-)
> > 
> > This patch moves the guess_correct_pts function from cmdutils to libavcodec.
> > The feature is available through a new field in AVFrame, guessed_pts.
> > 
> > make test and ffplay still work, but I would like to have some more time to
> > read it carefully, but I have things to do in the next few days, and this
> > feature was discussed in another thread, so here the current version. It
> > will conflict with some patches that will certainly soon be applied, so I
> > will update and submit it again anyway.
> > 
> > Regards,
> > 
> > -- 
> >  Nicolas George
> > 
> > [..]
> > +\
> > +    /**\
> > +     * pts estimated using various heuristics, in stream time base\
> > +     * - encoding: unused\
> > +     * - decoding: set by libavcodec, read by user.\
> > +     */\
> > +    int64_t guessed_pts;\
> 
> It's not quite estimated, it's chosen from either pts or dts.

true
but we could in the future add the frame duration to it to get the next
timestamp if that has neither dts nor pts, then it would be guessed in some
cases like when the frame duration is guessed
maybe best_effort_timestamp would be an option


> I think the name should be reassuring - though I can't think of one, just 'timestamp'?
> Also, the comment should say that clients can rely on this as the time of the frame.
> 
> >  #define FF_QSCALE_TYPE_MPEG1 0
> > @@ -2817,6 +2824,19 @@ typedef struct AVCodecContext {
> >      * - encoding: unused
> >      */
> >     AVPacket *pkt;
> > +
> > +    /**
> > +     * Current statistics for PTS correction.
> > +     * - decoding: maintained and used by libavcodec
> > +     * - encoding: unused
> > +     */
> > +    struct av_pts_correction_context {
> > +        int64_t num_faulty_pts; /// Number of incorrect PTS values so far
> > +        int64_t num_faulty_dts; /// Number of incorrect DTS values so far
> > +        int64_t last_pts;       /// PTS of the last frame
> > +        int64_t last_dts;       /// DTS of the last frame
> > +    } pts_correction;
> > +
> > } AVCodecContext;
> 
> The trouble with this is that adding fields to the struct will break ABI compatibility, so if the algorithm changes new fields will have to be put at the end of AVCodecContext. Just put the fields into the struct directly and use @defgroup for documentation.

agree, and ill take over review from mans from here
ill post a review for the patch in a moment, after that once its ok ill commit
it to ffmpeg at videolan

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110201/a4a29988/attachment.pgp>