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

[Baptiste Coudurier]

On 10/06/2010 06:04 PM, Michael Niedermayer wrote:
> On Thu, Oct 07, 2010 at 02:33:27AM +0200, Michael Niedermayer wrote:
>> On Wed, Oct 06, 2010 at 04:09:30PM -0700, Baptiste Coudurier wrote:
>>> On 10/06/2010 03:48 PM, Michael Niedermayer wrote:
>>>> 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
>>>
>>> Agree but sometimes I just don't care myself, I'm sure I'm not the only one.
>>>
>>>>>
>>>>>> 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
>>>
>>> It appears when stream copying
>>>
>>> ffmpeg -i<file>  -vcodec copy -f vob test.mpg
>>
>> i found and fixed one issue, there possibly are more.
>> especially the VBV buffer size used could plain be too small
>
> and interrestingly the mpeg ts muxer doesnt even read the vbv buf size

Of course it should, but the TS muxer in CBR mode is only activated by 
the user specifying a muxrate, so it's up to the user to specify a 
correct one :)

> either way, if you still have a failing file (with mpegps), i need a sample
> as my 2 that i tried work but they are lower bitrate.

You can create a file that fails using -vb 50Mb -minrate 50Mb -maxrate 
50Mb -bufsize <pick optimal value here, btw ffmpeg could do that for you> :)

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