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

[Baptiste Coudurier]

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.

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

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

[...]

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