[MPlayer-dev-eng] [PATCH] changes in vf semantics

[D Richard Felker III]

On Wed, Dec 01, 2004 at 11:13:12PM +0200, Ivan Kalvachev wrote:
> Well well well.
> So, If I have understood what the already applied patch is supposed to
> do, is the following:
> At mpcodecs_config_vo() time it stores the visible dimensions, that are
> passed to it and later
> forces them to the mpi structures.
> This way get_image() are free to request aligned dimensions.
> 
> Well, I see sense in this modification, but I would have rejected it, as
> it also
> involves change in many places. And basically changing the philosophy of
> the video
> system.
> What I would have done is to align the height together with width when
> ALIGNED flag
> is set in mpi. This is nearly 2 line fix that doesn't require massive
> changes here and there.

the issue isn't alignment, it's needing border space for crap to get
written to. this is a different issue, imo, and the alignment system
isn't general enough to handle it.

> I have attached the patch I have done (IT IS NOT TESTED AT ALL)
> to see if it will fix all know issues, if you revert the current patch.
> There is even chance it to work correctly
> for all codecs (except svq1)
> The added bonus of mine patch is that it already works with all
> filters.)
> Well, it could have some side effects as some filters use chroma_height
> in loops. Probably we may need cw,ch similar to w,h in mpi ;) Anyway
> this is not problem specific to my patch ;)

these filters need to be fixed. the correct thing is to use w/h
shifted by horizontal or vertical chroma shift...

> > > Because of this, we should pass the aligned dimensions to vo
> > > config(). But this may lead to problems as we also pass visible
> > > dimensions (aspect) at the same time.(e.g. off by few lines creates
> > > horrible scaling artifacts on cheap old video cards, like the one I
> > > used to have). For fixed size vo it may lead to black bars, but this
> > > is not really issue.
> > 
> > This is totally broken and unacceptable. There's a reason it's not
> > implemented that way.
> 
> How nice you don't give reason why it is so.

the output is blatently wrong. it's not ok for mplayer to display
wrong output!

> > > Probably it would be good idea to add an alignment parameters to the
> > > mpcodecs_config_vo() and remove them from get_image() (e.g. making
> > > flags informative, not settable). This seams to be also the less
> > > intrusive way. Indeed, it will need changing of all vo-s.
> > 
> > More total nonsense. There's no reason special alignment/border needs
> > should be restricted to codecs! Filters might need it too!!!
> 
> You insensitive clod, you don't even know what mpcodecs_config_vo()
> does!
> JFYI it also calls vf->config chain, not only decoder control and vo one
> ...

my point is that even tho the codec might not need alignment/borders,
some filter might. thus the interface to requesting borders MUST be in
the vf layer, not the vd/vf interface.

> > What you're talking about is a whole new vf api, so go write the whole
> > thing in a non-broken way and call us in 5 years when you're done.
> > Meanwhile we're trying to FIX MPLAYER G1 WITH MINIMAL CHANGES TO VF SO
> > THAT IT NEVER CRASHES ON STRANGE-SIZED MOVIES!! And the proposed fix
> > does exactly that.
> 
> Well, no. The already applied patch changes the whole video filter
> system dimension
> handling, that need reworking of quite many places to handle the new
> semantics.

what need reworking/fix? imo nothing.

> > > I insist to discuss this here in maillist as it give more time to
> > > think about issues, more time to check the code and test things.
> > 
> > OK, we'll give you the 5 years to write g3 vf... ;)
> > But each week it takes adds 10l cola to your tab. :))))
> 
> How nice, that you started coding your video system before me.
> This way you have an head start of cola...

yes, i agree. :))
msg me on irc for my postal address and you can ship me some crates of
coke. just not pepsi... :))

rich