[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

Michael Niedermayer michaelni
Tue Feb 24 20:58:49 CET 2009


On Tue, Feb 24, 2009 at 08:20:53PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> I ve just added h264 to tb_unreliable()
>>   
> So, here the new patches:
>

> avcodec_timebase: change duration computation to use time_base instead of 
> TB/2.

ok


> h264_timebase: correct time_base of H.264 and repeat_pict.

ok


>
> Further, I suppose, MPEG timebase should be adjusted as well, see 
> mpeg_timebase (actually your patch), to handle repeat_pict correctly.

ok


>
> I also noticed that ffplay computes frame delay using old formula with 1/2 
> fps. So I suppose, this should be changed as well (including increasing 
> AVFrame.repeat_pict analogously to AVPacket.repeat_pict for H.264 and 
> MPEG). Correct? If so, I could make a patch for it as well.

yes i think so


>
> In general, I wonder if repeat_pict (both in AVFrame and AVPacket) should 
> not be renamed to frame_duration on next major version bump and increased 
> by 1 everywhere...

maybe


>
> Timestamp computation is now also adjusted to your proposed names:
>
> avcodec_timestamp: add timestamp computation, if values exported from 
> codec.
> h264_timestamp: timestamp parameter export from H.264.
>

> One problem remains, though: FPS detection doesn't take field pictures into 
> account. So if the format returns frames containing field pictures with 1/2 
> frame duration (or better to say, with DTS distance of 1/2 frame duration), 
> then it will detect 2*fps instead of correct fps.

yes


>
> I'd propose adding a flag field_picture_flag to AVPacket (and pass it from 
> the parser) to notify the user about field pictures, so it can correctly 
> compute frame rate. What do you think? Or do you have a better idea?

i dont know, it doesnt seem to be such a big issue to me to have 2*fps ...


>
> > just added AVFMT_VARIABLE_FPS
>
> Shouldn't then mpegts have variable FPS as well? See mpegts_varframe patch.

no, mpeg-ps/ts do not support arbitrary variable fps also the flag is for
muxers


[...]

> @@ -3150,6 +3147,47 @@
>       * subtitles are correctly displayed after seeking.
>       */
>      int64_t convergence_duration;
> +
> +    // Timestamp generation support:
> +    /**
> +     * Synchronization point for start of timestamp generation.
> +     *
> +     * Set to >0 for sync point, 0 for no sync point and <0 for undefined
> +     * (default).
> +     *
> +     * For example, this corresponds to presence of H.264 buffering period
> +     * SEI message.
> +     */
> +    int dts_sync_point;

unneeded, the other 2 can just be set to AV_NOPTS_VALUE when unavailable
(they need to be 64bit then though)


> +
> +    /**
> +     * Offset of the current timestamp against last timestamp sync point in
> +     * units of AVCodecContext.time_base.
> +     *
> +     * Set to INT_MIN when dts_sync_point unused. Otherwise, it must
> +     * contain a valid timestamp offset.
> +     *
> +     * Note that the timestamp of sync point has usually a nonzero
> +     * dts_ref_dts_delta, which refers to the previous sync point. Offset of
> +     * the next frame after timestamp sync point will be usually 1.
> +     *
> +     * For example, this corresponds to H.264 cpb_removal_delay.
> +     */
> +    int dts_ref_dts_delta;
> +
> +    /**
> +     * Presentation delay of current frame in units of AVCodecContext.time_base.
> +     *
> +     * Set to INT_MIN when dts_sync_point unused. Otherwise, it must
> +     * contain valid non-negative timestamp delta (presentation time of a frame
> +     * must not lie in the past).
> +     *
> +     * This delay represents the difference between decoding and presentation
> +     * time of the frame.
> +     *
> +     * For example, this corresponds to H.264 dpb_output_delay.
> +     */
> +    int pts_dts_delta;
>  } AVCodecParserContext;
> 
>  typedef struct AVCodecParser {
> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 17520)
> +++ libavformat/avformat.h	(working copy)
> @@ -22,8 +22,8 @@
>  #define AVFORMAT_AVFORMAT_H
>  
>  #define LIBAVFORMAT_VERSION_MAJOR 52
> -#define LIBAVFORMAT_VERSION_MINOR 29
> -#define LIBAVFORMAT_VERSION_MICRO  2
> +#define LIBAVFORMAT_VERSION_MINOR 30
> +#define LIBAVFORMAT_VERSION_MICRO  0
>  
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>                                                 LIBAVFORMAT_VERSION_MINOR, \
> @@ -490,6 +490,16 @@
>      const uint8_t *cur_ptr;
>      int cur_len;
>      AVPacket cur_pkt;
> +
> +    // Timestamp generation support:
> +    /**
> +     * Timestamp corresponding to the last dts sync point.
> +     *
> +     * Initialized when AVCodecParserContext.dts_sync_point >= 0 and
> +     * a DTS is received from the underlying container. Otherwise set to
> +     * AV_NOPTS_VALUE by default.
> +     */
> +    int64_t reference_dts;




>  } AVStream;
>  
>  #define AV_PROGRAM_RUNNING 1

> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c	(revision 17520)
> +++ libavformat/utils.c	(working copy)
> @@ -837,6 +834,25 @@
>              pkt->dts += offset;
>      }
> 
> +    if (pc && pc->dts_sync_point >= 0) {
> +        // we have synchronization info from the parser
> +        int64_t den = st->codec->time_base.den * st->time_base.num;

you need a int64_t cast in there to prevent overflow


> +        if (den > 0) {
> +            int64_t num = st->codec->time_base.num * st->time_base.den;

same


> +            if (pkt->dts != AV_NOPTS_VALUE) {
> +                // got DTS from the stream, update reference timestamp
> +                st->reference_dts = pkt->dts - pc->dts_ref_dts_delta * num / den;

av_rescale_q() should simplify this and similar code.


> +                pkt->pts = pkt->dts + pc->pts_dts_delta * num / den;
> +            } else if (st->reference_dts != AV_NOPTS_VALUE) {
> +                // compute DTS based on reference timestamp
> +                pkt->dts = st->reference_dts + pc->dts_ref_dts_delta * num / den;
> +                pkt->pts = pkt->dts + pc->pts_dts_delta * num / den;
> +            }
> +            if (pc->dts_sync_point > 0)
> +                st->reference_dts = pkt->dts; // new reference
> +        }

a suggestion for simplification:

if (pkt->dts != AV_NOPTS_VALUE) {
    st->prev_dts = pkt->dts;
} else if (st->prev_dts != AV_NOPTS_VALUE && dts_ref_dts_delta != AV_NOPTS_VALUE) {
    st->prev_dts+= dts_ref_dts_delta * num / den;
    pkt->dts = st->prev_dts;
}

if(pkt->pts == AV_NOPTS_VALUE && pkt->dts != AV_NOPTS_VALUE && pts_dts_delta != AV_NOPTS_VALUE);
    pkt->pts = pkt->dts + pts_dts_delta * num / den;



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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20090224/86272bc7/attachment.pgp>



More information about the ffmpeg-devel mailing list