[mythtv] Ticket #7759

[Taylor Ralph]

--- On Sun, 12/20/09, Davin McCall <davmac at davmac.org> wrote:

> From: Davin McCall <davmac at davmac.org>
> Subject: Re: [mythtv] Ticket #7759
> To: mythtv-dev at mythtv.org
> Date: Sunday, December 20, 2009, 4:57 PM
> On 21/12/09 02:44, Daniel
> Kristjansson wrote:
> >       repeat_delay =
> frame_interval / 2 * buffer->repeat_pict;
> >    I'm revoking your programmers license :)
> You should never ever use a
> > division when a multiply will do. Maybe>>1 is
> what you meant to
> > say? It avoids the conversion from int to double that
> 0.5 forces.
> > 
> >    
> 
> To any even remotely capable compiler, ">>1" and "/2"
> are pretty much the same operation, but the intent of "/2"
> is more clear. (Yes, avoiding the conversion to double was
> one of the aims - that's an optimization the compiler
> generally won't, and probably isn't allowed to, perform).
> 
> I'm still not sure why the division by 2 (no matter how
> it's done) is necessary at all in that particular line of
> code, however... or at least, I'm not certain that it's
> always correct.
> 
> Davin
> 

I didn't create that formula out of thin air. The formula for the additional frame delay is taken directly from what "was" defined by the FFmpeg library. It is also what ffplay currently uses to calculate frame duration. Also, the patch implements it per frame which I believe to be correct and it should not matter if m_double_rate is set.

However, the last FFmpeg sync included some changes to repeat_pict for mpegvideo_parser and h264_parser. It also redefined how to calculate frame duration. However, for MPEG-2 it still uses repeat_pict from mpeg12.c which was never adapted for the new formula. So for MPEG-2 playback it should be correct and for H.264 it appears to half a frame interval to little. At any rate, it should be exact for MPEG-2 and improved for H.264. I guess we could determine the codec and apply two different formulas. I'd appreciate someone to correct me if I'm wrong. It would be nice if someone else did the research too.

With all this said, the patch uses what is currently implemented by ffplay which is maintained by FFmpeg. I've confirmed the formula to be correct by manually looking at video timestamps for broadcast 720p and 1080i. If someone has a sample which doesn't correlate then I'd like to see it.

Thanks for reviewing the patch. It should be interesting what others have to say.

--
Taylor