[FFmpeg-devel] [PATCH] [soc: dvbmuxer] Improve PCR accuracy

Tomer Barletz barletz
Wed Dec 31 00:39:55 CET 2008


Hi Baptiste,

On Tue, Dec 30, 2008 at 9:15 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> Hi Tomer,
>
> Tomer Barletz wrote:
>> On Mon, Dec 29, 2008 at 10:06 AM, Tomer Barletz <barletz at gmail.com> wrote:
>>> Hi,
>>>
>>> On Thu, Dec 25, 2008 at 7:38 PM, Baptiste Coudurier
>>> <baptiste.coudurier at gmail.com> wrote:
>>>> Tomer Barletz wrote:
>>>>> On Wed, Dec 24, 2008 at 12:44 PM, Baptiste Coudurier
>>>>> <baptiste.coudurier at gmail.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Barletz wrote:
>>>>>>> Hi Baptiste, and thanks for your comments!
>>>>>>>
>>>>>>> On Tue, Dec 23, 2008 at 9:44 PM, Baptiste Coudurier
>>>>>>> <baptiste.coudurier at gmail.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Barletz wrote:
>>>>>>>>> On Tue, Dec 23, 2008 at 1:34 PM, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>> Barletz <barletz <at> gmail.com> writes:
>>>>>>>>>>
>>>>>>>>>>> This patch mainly improves PCR accuracy in the current mpegtsenc.c
>>>>>>>>>>> implementation.
>>>>>>>>>> Please use svn diff (or diff -u) to produce the patches.
>>>>>>>>>>
>>>>>>>>>> We can't read other formats;-)
>>>>>>>>>>
>>>>>>>> First, I'd like to thank you for taking the time to work on the muxer,
>>>>>>>> this is really appreciated.
>>>>>>>>
>>>>>>> No problem, the pleasure is mine ;)
>>>>>>>
>>>>>>>>  > [...]
>>>>>>>>  >
>>>>>>>>> +
>>>>>>>>> +    return num_packets_written;
>>>>>>>> Instead of modifying all function to return packets written, would it
>>>>>>>> simpler to add a fiel in the global context ?
>>>>>>>>
>>>>>>>> You can add packet_count to MpegTSWrite, and access it using
>>>>>>>> ->opaque->priv_data in mpegts_write_section.
>>>>>>>>
>>>>>>>> Also please split all modifications that can be splitted. This makes
>>>>>>>> reviewing easier. Thanks.
>>>>>>>>
>>>>>>> I was thinking that this will enhance these methods, since the number
>>>>>>> of packets which were actually flushed to the stream may be important
>>>>>>> in other uses, and per table (perhaps I just want to know how many
>>>>>>> packets the SDT used).
>>>>>> Well, you can check the value before calling the func and after, this is
>>>>>> the same IMHO. I really feel the code would be simpler.
>>>>>>
>>>>> The problem is that I will not know whether any of the mpets_write_XXX
>>>>> has failed. Currently -1 return is used in order to signal for error,
>>>>> but most of the write methods will not delegate this error, since they
>>>>> have no return value.
>>>> Changing functions to return -1 on error and propagating this error is
>>>> probably be ok, but this should be in a separate patch.
>>>>
>>>>> I also must disagree on the simpler part - I believe that my way is
>>>>> simpler, I guess It's a matter of taste :D
>>>> Only 2 function actually write ts packets: mpegts_write_section1 and
>>>> mpegts_write_pes, so technically only 2 ("ts->packet_count++") are
>>>> needed, then you add the ("ts->cur_pcr = ...") line I pasted and the
>>>> field declaration in the struct and you should be done.
>>> Sorry for misunderstanding. I've attached another patch - please tell
>>> me if this is what you meant. This code does not compile - please tell
>>> me what I'm doing wrong.
>
> You need to cast ->opaque to the right struct.
> MpegTSWrite *ts = ((AVFormatContext*)s->opaque)->priv_data;

I've tried that, but it is impossible since MpegTSWrite is declared
after mpegts_write_section. I though you have some kind of trick to
work that out by using the opaque...
I tried to forward-declare MpegTSWrite - didn't work. Please advise.

>
> Also what I meant was:
>
> packet_count; ///< total number of TS packets written
>
> Is that more clear ? Sorry for the confusion.

I got it the first time - am I doing something wrong?

>
>>>>>>  >>> [...]
>>>>>>  >>>
>>>>>>>>> +            ts->cur_pcr =
>>>>>>>>> +                // the time passed while SI tables written
>>>>>>>>> +                MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate)
>>>>>>>>> +                // the time passed for packets up until the first pcr bit, as
>>>>>>>>> +                // required in the standard
>>>>>>>>> +                + BYTES_TIME(11, ts->mux_rate);
>>>>>>>>> +            write_pcr = 1;
>>>>>>>>> +            ++pcr; // done with first time pcr
>>>>>>>> I need to double check this, but why is it simply incremented ?
>>>>>>>>
>>>>>>> I use this variable only to avoid declaring another flag which will
>>>>>>> indicate whether the first pcr is already written - the pcr variable
>>>>>>> will be ignored from now on, and only ts->cur_pcr will be used.
>>>>>> Humm, then can't ts->cur_pcr be checked ? It should be 0 at init.
>>>>>>
>>>>> I've tried that, but the results are different. I figured that some
>>>>> other code is using that value somewhere in between, but I couldn't
>>>>> find it - could you point me to it? Or maybe we could just commit that
>>>>> solution, then later improve it.
>>>> I need to double check, this was just an idea, maybe last_pcr could be
>>>> used too.
>>> Any news?
>>>
>>>>>>>>> +        }
>>>>>>>>> +        else
>>>>>>>>> +        {
>>>>>>>>> +            ts->cur_pcr = increment_pcr(ts->cur_pcr,
>>>>>>>>> +                    // the time passed from last packet
>>>>>>>>> +                    SINGLE_PACKET_TIME(ts->mux_rate)
>>>>>>>>> +                    // the time passed while SI tables written
>>>>>>>>> +                    + MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate));
>>>>>>>>> +            // write pcr only if pcr pid and if repetition interval is right
>>>>>>>>> +            write_pcr = (ts_st->pid == ts_st->service->pcr_pid) ?
>>>>>>>>> +                    ((ts->cur_pcr - ts->last_pcr) > PCR_REPETITION_INTERVAL ? 1 : 0) : 0;
>>>>>>>> I think header and pcr bytes must be added to pcr value here (11 bytes).
>>>>>>> I added those bytes only when I first encountered a new pcr (pcr==0).
>>>>>>> From then on, I already have this offset, and I just want to increment
>>>>>>> by a single transport packet.
>>>>>>> I've tested the output ts with Tektronics' StreamAnalyzer, and the PCR
>>>>>>> interval and accuracy looks right.
>>>>>> Yes, I see, I think something like this would be more clear to
>>>>>> understand the code:
>>>>>>
>>>>>> ts->cur_pcr = (ts->packet_count*188+11)*8*time_base/ts->mux_rate.
>>>>>>
>>>>>> What do you think ?
>>>>> I think that this is exactly what you see in the current
>>>>> implementation, only in a macro. Do you think that the macro's name is
>>>>> not suitable? Or perhaps I misunderstand you...
>>>> No, you are right, This is just what the macro does, only one macro
>>>> could be used though (like PACKET_COUNT_TO_PCR(ts->packet_count)), but
>>>> ideally the +11 should be explained in comment, maybe the macro is not
>>>> needed and the code itself is easier to understand.
>>> I don't mind - as long as we use that macro only once.
>
>  > [...]
>>
>> --- dvbmuxer/mpegtsenc.c      2008-09-24 09:58:45.000000000 +0300
>> +++ ffmpeg/libavformat/mpegtsenc.c    2008-12-29 10:07:42.000000000 +0200
>> @@ -25,6 +25,18 @@
>>  #include "libavcodec/bytestream.h"
>>  #include "mpegpes.h"
>>
>> +#define MAX_PCR_VALUE (0x1FFFFFFFF*300+0x12C)
>
> 33bits int *300 + ?

This is the maximum extension value (max base + max ext = max pcr).
Have I missed something?

>
>> +#define PCR_TIME_BASE 27000000LL
>> +#define PCR_REPETITION_INTERVAL (PCR_TIME_BASE / 25)
>
> Can you please explain the 25 ?

40ms pcr interval as defined in the dvb standard, (100ms in mpeg, if
I'm not mistaken), presented in 27MHz clock.

>
>> +#define BYTES_TIME(num_bytes, mux_rate) ((num_bytes)*8*PCR_TIME_BASE / (mux_rate))
>> +#define SINGLE_PACKET_TIME(mux_rate) BYTES_TIME(TS_PACKET_SIZE, mux_rate)
>> +#define MULTI_PACKETS_TIME(num_packets, mux_rate) ((num_packets)*SINGLE_PACKET_TIME(mux_rate))
>
> I think only one macro is needed, TS_PACKET_COUNT_TO_PCR.

Agreed.

>
>  > [...]
>  >
>> @@ -78,6 +90,7 @@
>>              memset(q, 0xff, left);
>>
>>          s->write_packet(s, packet);
>> +        ++s->opaque->priv_data->packet_count;
>
> You need to cast.

See above.

>
>>
>>          buf_ptr += len1;
>>          len -= len1;
>> @@ -173,6 +186,7 @@
>>      int64_t last_pcr; ///< last program clock reference */
>>      int64_t cur_pcr;  ///< last program clock reference */
>>      int mux_rate;
>> +    int packet_count;
>>  } MpegTSWrite;
>
> packet_count; ///< total number of TS packets written

Will add the comment + see above.

>
>>
>>  static void mpegts_write_pat(AVFormatContext *s)
>> @@ -481,10 +495,11 @@
>>      return -1;
>>  }
>>
>> -/* send SDT, PAT and PMT tables regulary */
>> -static void retransmit_si_info(AVFormatContext *s)
>> +/* send SDT, PAT and PMT tables regulary if the time is right */
>> +static int maybe_retransmit_si_info(AVFormatContext *s)
>>  {
>>      MpegTSWrite *ts = s->priv_data;
>> +    ts->packet_count = 0;
>
> Why are you resetting it ?

Cuz I have a new iteration, and I only want to count the packets since
the last pcr write.
I must say that I think that my first approach was clearer...

>
>>      int i;
>>
>>      if (++ts->sdt_packet_count == ts->sdt_packet_freq) {
>> @@ -498,6 +513,13 @@
>>              mpegts_write_pmt(s, ts->services[i]);
>>          }
>>      }
>> +    return ts->packet_count;
>
> Changing proto and return value can be done in a separate patch, forget
> about it for now, let's focus on fixing PCR computation to ensure a 100%
> spec compliant TS.

Agreed.

>
>> +}
>> +
>> +/* increment pcr, handle wrap */
>> +static int64_t increment_pcr(int64_t cur_value, int64_t inc_value)
>> +{
>> +    return (cur_value + inc_value) % (MAX_PCR_VALUE + 1);
>>  }
>>
>>  /* NOTE: pes_data contains all the PES packet */
>> @@ -510,18 +532,37 @@
>>      uint8_t *q;
>>      int val, is_start, len, header_len, write_pcr;
>>      int afc_len, stuffing_len;
>> +    MpegPcr mpeg_pcr;
>> +    int num_si_packets_written;
>>
>>      is_start = 1;
>> -    ts->cur_pcr = pcr;
>> +
>>      while (payload_size > 0) {
>> -        retransmit_si_info(s);
>> +        num_si_packets_written = maybe_retransmit_si_info(s);
>>
>> -        write_pcr = !ts->cur_pcr;
>
> IIRC this was done to force pcr to be written on first packet, so I
> really think cur_pcr can be checked against 0.

Agreed.

>
>> -        if (ts_st->pid == ts_st->service->pcr_pid) {
>> -            pcr = ts->cur_pcr + (TS_PACKET_SIZE+4+7)*8*90000LL / ts->mux_rate;
>> -            if (pcr - ts->last_pcr > MAX_DELTA_PCR)
>> -                write_pcr = 1;
>> +        if (0 == pcr) // first pcr value in stream
>> +        {
>> +            ts->last_pcr = 0;
>> +            ts->cur_pcr =
>> +                // the time passed while SI tables written
>> +                MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate)
>> +                // the time passed for packets up until the first pcr bit, as
>> +                // required in the standard
>> +                + BYTES_TIME(11, ts->mux_rate);
>> +            write_pcr = 1;
>> +            ++pcr; // done with first time pcr
>
> See above, though please do "pcr = 1", ++ makes no sense here. Variable
> name needs to be changed to pcr_written after that.

Agreed*2.

>
>>          }
>> +        else
>> +        {
>> +            ts->cur_pcr = increment_pcr(ts->cur_pcr,
>> +                    // the time passed from last packet
>> +                    SINGLE_PACKET_TIME(ts->mux_rate)
>> +                    // the time passed while SI tables written
>> +                    + MULTI_PACKETS_TIME(num_si_packets_written, ts->mux_rate));
>> +            // write pcr only if pcr pid and if repetition interval is right
>> +            write_pcr = (ts_st->pid == ts_st->service->pcr_pid) ?
>> +                    ((ts->cur_pcr - ts->last_pcr) > PCR_REPETITION_INTERVAL ? 1 : 0) : 0;
>> +         }
>
> Did you ignore my comment ?

No, simply forgot to change the code...

>
> Also I believe the whole can be simplified:
>
> if (!ts->cur_pcr || (ts_st->pid == ts_st->service->pcr_pid &&
>     pcr - ts->last_pcr > MAX_DELTA_PCR))
>           write_pcr = 1;
>
> if (write_pcr) {
>    ts->cur_pcr = TS_PACKET_COUNT_TO_PCR(ts->packet_count);
>    ....
> }
>
> You can put the check for max pcr value in the macro.

Agreed.

>
> [...]
>
> --
> Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
> Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
> checking for life_signs in -lkenny... no
> FFmpeg maintainer                                  http://www.ffmpeg.org
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>

Thanks for the input,
Tomer




More information about the ffmpeg-devel mailing list