[MPlayer-dev-eng] Re: [PATCH] VFCTRL_PERIODIC_UPDATE

[Uoti Urpala]

On Fri, 2007-01-19 at 14:32 -0500, Jason Tackaberry wrote:
> On Fri, 2007-01-19 at 19:17 +0200, Uoti Urpala wrote:
> > I think the patch should not be included as is. The timing-related
> > changes try to work around limitations that could be removed instead. A
> > better way would be as follows:
> 
> There needs to be a line drawn somewhere.  Ultimately what I'm trying to
> do (allow external process to update a blended overlay that is both
> functional while paused and independent of the mplayer framerate) is
> going to graft hacks onto the existing architecture.  Short of
> redesigning mplayer myself, we will have to accept there will be some
> compromises in some places, and if that isn't acceptable, I'd surely
> appreciate knowing that sooner rather than later.

Adding workarounds to the central timing functions is quite hackish,
especially when it later turned out the added functionality doesn't even
quite match what the intended use requires. I think it would have been
better not to post VFCTRL_PERIODIC_UPDATE as a completely independent
patch without context. A bad implementation for directly needed
functionality is one thing and can perhaps be improved; the same for
functionality that is meant to be used as a part of a larger hack is
another thing.

> > If I've understood correctly what you intend to do with the overlay
> > filter then after those changes implementing it should be a lot more
> > straightforward. It could use ordinary slave commands which would remove
> > the need for both the input filter hacks mentioned earlier and
> > VFCTRL_PERIODIC_UPDATE (since the only mentioned use for the update was
> > basically to check input instead of any real filter functionality).
> 
> Actually there are no input filter hacks, AFAIC.  The input hacks you
> mentioned were your own invention, based on your guess as to how I'm
> implementing vf_overlay.  I later followed up and described how I was
> implementing it, which while unprecedented in the sense that no other
> filter (or anywhere else in mplayer for that matter) seems to use
> mp_input_add_cmd_filter(), I find it to be a quite reasonable approach

The use of mp_input_add_cmd_filter() is what I was referring to by
"filter hacks".

> to a filter handling a non-trivial slave command as opposed to stuffing
> all that logic into mplayer.c, which itself strikes me as fragile and
> busting at the seams.

You don't necessarily need to (and probably shouldn't) add all the logic
to mplayer.c; the slave command implementation there can just call the
filter. I doubt handling some commands in a different way using filters
would do anything to reduce fragility.