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

[Barletz]

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).

>  > [...]
>  >
>> +        if (0 == pcr) // first pcr value in stream
>> +        {
>> +            ts->last_pcr = 0;
>
> I don't think this is needed, it is zeroed at context init.

I will check it again.

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

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

> Also the ternary is unnecessary, ">" returns either 0 or 1 in C.
>

Yes, my bad.

>>          }
>>
>>          /* prepare packet header */
>> @@ -536,17 +593,19 @@
>>          *q++ = 0x10 | ts_st->cc | (write_pcr ? 0x20 : 0);
>>          ts_st->cc = (ts_st->cc + 1) & 0xf;
>>          if (write_pcr) {
>> +            mpeg_pcr.base = ts->cur_pcr / 300;
>> +            mpeg_pcr.ext  = ts->cur_pcr % 300;
>>              /* add header and pcr bytes to pcr according to specs */
>> -            pcr = ts->cur_pcr + (4+7)*8*90000LL / ts->mux_rate;
>>              *q++ = 7; /* AFC length */
>>              *q++ = 0x10; /* flags: PCR present */
>> -            *q++ = pcr >> 25;
>> -            *q++ = pcr >> 17;
>> -            *q++ = pcr >> 9;
>> -            *q++ = pcr >> 1;
>> -            *q++ = (pcr & 1) << 7;
>> -            *q++ = 0;
>> -            ts->last_pcr = pcr;
>> +            *q++ = mpeg_pcr.base >> 25;
>> +            *q++ = mpeg_pcr.base >> 17;
>> +            *q++ = mpeg_pcr.base >> 9;
>> +            *q++ = mpeg_pcr.base >> 1;
>> +            *q = (mpeg_pcr.base & 1) << 7;
>> +            *q++ = (*q | 0x3E) | (mpeg_pcr.ext >> 8);
>> +            *q++ = mpeg_pcr.ext;
>
> I think this hunk, which add precision should be in a separate patch.
>

Will do.

> --
> 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
>