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

[Tomer Barletz]

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.
I also must disagree on the simpler part - I believe that my way is
simpler, I guess It's a matter of taste :D

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

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

>
> [...]
>
> --
> 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 again for your comments.

Tomer