[FFmpeg-devel] [PATCH] [soc: dvbmuxer] Improve PCR accuracy
Tomer Barletz
barletz
Sun Jan 4 11:55:25 CET 2009
On Wed, Dec 31, 2008 at 1:39 AM, Tomer Barletz <barletz at gmail.com> wrote:
> 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
>
Baptiste,
Can you update regarding this patch?
Is the SOC dvbmuxer project going to be integrated into the main
branch any time soon?
More information about the ffmpeg-devel
mailing list