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

[Tomas Härdin]

On Wed, 2010-10-06 at 13:31 +0200, 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. Thats the least if you dont
> replace the buffering code by the working code from mpeg-ps

It isn't silent. The code lets execution reach line 681, which has a
check for that ("dts < pcr, TS is invalid"). Having it error out or
abort there is unrelated to this patch though, so I sort of ignored it.

I haven't looked at the mpeg-ps code much yet. Perhaps by sharing and
reworking it a bit, it can be used to solve the PCR interval problem I
posted a patch for just a few minutes ago? Maybe I should have checked
out the mpeg-ps muxer earlier..

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101006/0409f7ac/attachment.pgp>