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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Tue, Feb 24, 2009 at 08:20:53PM +0100, Ivan Schreter wrote:
>   
> [...]
>> 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 ...
>
>   
It is for the user. See, I want to convert a H.264 interlaced video to 
some other format. So I'd call ffmpeg -i h264.mts -vcodec 
mpeg2video-acodec mp2 -f vob h264.vob. What will this do? It will create 
a vob with 2*fps and each picture duplicated, which is definitely not 
what we want... True, the user could specify -r 25 to force frame rate 
to 25fps, but it's not logical. So IMHO we need this. Agreed?

>>> 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
>
>   
Ah, OK.

> [...]
>
>   
>> @@ -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)
>   
Not really. If used, the two others are _always_ available (even if it 
wouldn't be required, there could be and there is a file where they 
are). dts_ref_dts_delta contains delta to reference DTS. Reference DTS 
changes at the beginning of buffering period. So the only way would be 
to detect new buffering period, if we take one frame in the future and 
look at dts_ref_dts_delta there. If it's less than current 
dts_ref_dts_delta, then current frame is start of new buffering period.

Since we don't have a crystal ball to look into the future :-), it has 
to be done differently, therefore the flag to indicate start of new 
buffering period. Note that delta for first frame of new buffering 
period refers to the _old_ buffering period (i.e., old reference DTS).

Additionally, on and around buffering period change, we potentially have 
no DTS from the container. So we cannot use something you suggested 
below as well.

Clear?

>
>> 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
>   
Darn... True.

>> +            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.
>
>   
True, but then I'd have to convert a scalar value to an AVRational, 
which involves copy into memory and more complex/slower code. And I 
doubt it would really simplify it optically in this particular case. So 
I decided that for this particular case, it's better without.

>> +                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;
>   
That won't work, due to necessity of sync point, see above.

Imagine following sequence (dts, sync flag, dts_ref_dts_delta): (0, 1, 
0); (-, 0, 1); (-, 0, 2), (-, 1, 3), (-, 0, 1), (5, 0, 2); (-, 1, 3); ...

At (-, 1, 3), we need to generate DTS 3. This would still work. But at 
(-, 0, 1) we need to generate DTS 4 (with reference to 3). Since 
reference is still at 0 without reference flag, it would generate DTS 1, 
which is wrong. With reference flag, we can set the reference DTS to 3 
when processing (-, 1, 3), so it will work.

Attached is the corrected patch (regarding 64-bit values) and H.264 
timestamp patch for reference.

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: avcodec_timestamp.patch
Type: text/x-patch
Size: 5480 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090224/8019096d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_timestamp.patch
Type: text/x-patch
Size: 739 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090224/8019096d/attachment-0001.bin>