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

[Baptiste Coudurier]

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.

>>  >>> [...]
>>  >>>
>>>>> +            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.

>>>>> +        }
>>>>> +        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.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org