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

[Uoti Urpala]

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

> > 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? If so, how would that have been an improvement? The only possible
difference I see would be using buffer callback hacks to count the
number of buffers vs using buffer callback hacks to associate data with
a particular buffer.

> > 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.

> > > 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.

> 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.

> > There should be some way for the user to know that a particular opaque
> > field is not going to ever be returned. This would be needed to avoid
> > memory leaks if storage was allocated for data used with the void *, and
> 
> Sounds like a argument against your suggestion.

It's an argument for using direct storage in the struct when possible.
However it is not an argument proving that would always be possible. You
can have more than 64 bits of data associated with a frame.