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

[Michael Niedermayer]

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

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/6d4c4d41/attachment.pgp>