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

[Baptiste Coudurier]

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.

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

Please stop claiming otherwise, and stop throwing that PS muxer in, 
which has already make one SOC project fail. The PS code, is:
- hard to reuse
- messy and obfuscated.

The TS muxer _works_ in VBR mode. CBR is another beast.

So let the people working figure out how to do it. If people manage to 
reuse the PS code that is great, but this is clearly not the priority.
I prefer someone writing code that he understand and that works, than 
doing gymnastic to accommodate some other piece of code.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org