[FFmpeg-devel] [PATCH] Generic part of frame multithreading

[Michael Niedermayer]

On Thu, Aug 21, 2008 at 02:22:00PM +0300, Uoti Urpala wrote:
> On Thu, 2008-08-21 at 12:35 +0200, Michael Niedermayer wrote:
> > On Thu, Aug 21, 2008 at 09:58:33AM +0300, Uoti Urpala wrote:
> > > On Wed, 2008-08-20 at 23:37 +0200, Michael Niedermayer wrote:
> > > > On Wed, Aug 20, 2008 at 10:23:31PM +0300, Uoti Urpala wrote:
> > > > > On Wed, 2008-08-20 at 21:00 +0200, Michael Niedermayer wrote:
> > > > > > > mplayer counts frame delay using avctx->has_b_frames, which works for  
> > > > > > > h264 and doesn't work with this. ffplay counts delay by adding pts to  
> > > > > > > frames in get_buffer, which works with this but might not work with  
> > > > > > > h264, because of the frame num gap code. I thought about making avctx- 
> > > > > > >  >delay accurate for decoding too, which should solve all this.
> > > > > > 
> > > > > > mplayer is violating lavc API ...
> > > > > > IIRC ive rejected the use of has_b_frames back then when uoti suggested it
> > > > > 
> > > > > You remember things backwards. The preliminary version of -correct-pts
> > > > > used buffer callbacks to count the number of buffered frames, and it was
> > > > > you who suggested using has_b_frames instead.
> > > > 
> > > > I do remember rejecting the even more broken 
> > > > "callbacks to count the number of buffered frames" and i suspect i told you
> > > > back then that the timestamps should be given to the decoder to reorder them.
> > > > Maybe you rejected this and i then suggested to use has_b_frames, but if my
> > > 
> > > Your first suggestion was using has_b_frames instead, 
> > 
> > do you happen to have a link to the mail? I am just curious what the exact
> > question/discussion was to which i suggested has_b_frames ...
> 
> http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2006-May/042906.html

thanks


> 
> > > and lavc of course
> > > did not support reordering in the decoder.
> > 
> > lavc supported reordering in the decoder since it supported direct rendering
> > that predates any discussion we have had
> 
> Direct rendering allowed the buffer callback hacks. Is that what you
> mean? 

I mean that lavc supported reordering timestamps. Maybe it needed 5 more lines
of code than now but it was supported.


[...]

> 
> > > You also implemented reordering with buffer callbacks in ffplay.c
> > > yourself later, and even suggested that buffer callbacks should be a
> > > documented way of implementing such functionality with lavc:
> > > http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-March/043661.html
> > 
> > Are you complaining that i improved the API or are you jealous because i
> > fixed it when mans complaind while i didnt when you did?
> 
> What I'm "complaining" about are your claims that what I did with the
> available API was particularly hacky or ugly, when in fact you could do
> no better yourself and even defended similar usage earlier.

What i meant was that your attempts to count buffers is hacky as such
not how this buffer counting is done.


> 
> > > > guess (and this is only a guess) is true it would really be more a
> > > > "least broken solution within the constraints" not a "working solution"
> > > 
> > > You finally added some reordering support to lavc, but I think the
> > > current API is still lacking.
> > > 
> > > Using int64_t as the type of an opaque user-set field is unnecessarily
> > > limiting. MPlayer would use doubles, and void * should generally be
> > > available for storing arbitrary data in opaque user fields. I think an
> > > union of int64_t, double and void * would be a practical choice.
> > 
> > this reminds me of the word overkill, void * is a recipe for problems, like
> 
> Sure void * does have negative sides, which is why having the other
> types as alternatives is a good thing. However it is IMO not safe to
> assume that the other types will always be enough for everyone, and when
> they're not supporting void * directly is less ugly than the
> alternatives.

Its not my goal to make things enough for everyone, theres alway an odd
guy with a CRAY-1 in his basement who might have different needs.
My goal is to find a compromise between simplicity and features. And make
useage easy and straightforward for the majority. void * in this specific
case is going to cause bugs when the user application is not carefull ...

using void* successfully requires the user to understand the internal buffer
handling so he doesnt loose his pointers, these problems are not worth the
merely hypothetical gain of being more generic.


> 
> > memory leaks and a 64bit double surely can be stored in a opaque int64_t.
> 
> You can use pointer type casts but that's pretty ugly.
> 

> > of course if you do want a union of double and int64_t then iam not opposed
> > to approve a patch
> 
> What would you need the patch for? Seems like a trivial change.

Iam not planing to implement your feature request for you, if you want it
send a patch that passes review.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080821/cbe9060c/attachment.pgp>