[FFmpeg-devel] [PATCH] [soc: dvbmuxer] Improve PCR accuracy
Baptiste Coudurier
baptiste.coudurier
Tue Dec 30 20:15:21 CET 2008
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;
Also what I meant was:
packet_count; ///< total number of TS packets written
Is that more clear ? Sorry for the confusion.
>>>>> >>> [...]
>>>>> >>>
>>>>>>>> + 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 + ?
> +#define PCR_TIME_BASE 27000000LL
> +#define PCR_REPETITION_INTERVAL (PCR_TIME_BASE / 25)
Can you please explain the 25 ?
> +#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.
> [...]
>
> @@ -78,6 +90,7 @@
> memset(q, 0xff, left);
>
> s->write_packet(s, packet);
> + ++s->opaque->priv_data->packet_count;
You need to cast.
>
> 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
>
> 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 ?
> 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.
> +}
> +
> +/* 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.
> - 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.
> }
> + 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 ?
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.
[...]
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list