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

[Baptiste Coudurier]

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.

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

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

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

> +        }
> +        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).
Also the ternary is unnecessary, ">" returns either 0 or 1 in C.

>          }
>  
>          /* 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.

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