[FFmpeg-devel] mpegtsenc: Improve PCR generation and output

[Michael Niedermayer]

On Wed, Oct 06, 2010 at 03:19:25PM -0700, Baptiste Coudurier wrote:
> On 10/06/2010 04:31 AM, Michael Niedermayer wrote:
>> On Wed, Oct 06, 2010 at 11:07:05AM +0200, Tomas H?rdin wrote:
>>> On Mon, 2010-10-04 at 16:48 +0200, Tomas H?rdin wrote:
>>>> On Thu, 2010-09-30 at 11:33 -0700, Baptiste Coudurier wrote:
>>>>> Hi Tomas,
>>>>>
>>>>> On 9/30/10 7:07 AM, Tomas H?rdin wrote:
>>>>>> Hi
>>>>>>
>>>>>> While working some more on remuxing dvbsub in mpegts I noticed that the
>>>>>> longer the sample I used was, the higher the muxdelay I needed in order
>>>>>> to avoid the "dts<   pcr, TS is invalid" warning.
>>>>>>
>>>>>> This is due to how the current muxer calculates PCR. It simply
>>>>>> accumulates TS_PACKET_SIZE*8*90000LL/ts->mux_rate in cur_pcr. Unless
>>>>>> your mux_rate evenly divides 135360000 (TS_PACKET_SIZE*8*90000) this
>>>>>> will cause rounding errors which quickly lead to unacceptable PCR drift.
>>>>>>
>>>>>> A second problem is that it only uses 90 kHz precision, when it should
>>>>>> use 27 MHz. 90 kHz is insufficient to stay within the allowed +- 500 ns
>>>>>> jitter.
>>>>>>
>>>>>> The attached patch calculates PCR based on max_delay and the current
>>>>>> size of the output, in 27 MHz. This method should eliminate any PCR
>>>>>> drift caused by the rounding errors in the previous solution. It also
>>>>>> outputs the full PCR.
>>>>>>
>>>>>> The patch passes the regtests, but that seems to be because there are
>>>>>> none for mpegts. Maybe some should be added?
>>>>>
>>>>> The test is present, by default the muxer uses VBR, you activate CBR
>>>>> muxing by specifying a muxrate. Did you test specifying a muxrate ? :)
>>>>>
>>>>> Patch looks good, thanks for the work, I'll test it against some tools.
>>>>
>>>> Actually, I made a mistake. The six reserved bits are between
>>>> program_clock_reference_base and program_clock_reference_extension, not
>>>> after. So the patch writes the program_clock_reference_extension bits in
>>>> the wrong place. See table 2-7 on page 44 in ISO/IEC 13818-1.
>>>>
>>>> Updated patch attached.
>>>>
>>>> As for testing, I failed to find a file in tests/ that contains "mpegts"
>>>> or whose name includes "mpegts", so how would I go about testing this?
>>>>
>>>> /Tomas
>>>
>>> I discovered another bug yesterday. If dts ever becomes less than pcr
>>
>> if dts become less than pcr then you should do
>> av_log("internal error in mpeg ts muxer")
>> av_abort() or return -1 at appropriate point to end muxing
>>
>> why?
>>
>> because you create a broken file that isnot going to playback on at least some
>> players and doing this silently is VERY wrong.
>
> Well, many players just don't care about the PCR, namely FFmpeg,  
> mplayer, even Quicktime player.

yes, still its better to generate compliant files that play on all players


>
>> Thats the least if you dont replace the buffering code by the
> > working code from mpeg-ps
>
> Ok, now tell me how the ps muxer works with very high bitrates ? 50mb/s  
> and higher. It doesn't.

elaborate please, i was not aware of this problem


> Now PS and TS buffering model is _different_, and by different I mean as
> much as having a different model in the specs (T-STD).

could you summarize the differences?
Having things desribed by 2 totally different pieces of text and still be
completely identical is not uncommon for ISO specs.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101007/82f68ad6/attachment.pgp>