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

[Ivan Kalvachev]

On Wed, 1 Dec 2004 18:01:23 -0500
D Richard Felker III <dalias at aerifal.cx> wrote:

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

Different words for same thing. What do you think alignment is for? Storing cola in it?
It's always been an border space. Take a look of aligning code and you will see that it
works in pixels!!!
I agree that the current align system is not good. I even remember I had arguing with arpi
about it. Anyway with mine patch, the benefit is that all images
that need borders already set correctly the ALIGN flag, while with the patched 
system alignment should be done at hand when calling get_image(). Well I agree that
the already committed patch is better but it needs careful review all ALL video filters!!!
Not something I would like to do before release.

About Jindrich comment that some filters use mpi->width/height values - this is not
bug, but intended behavior as these values should contain the border aligned values!
 
 
> > 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!

It looks like my sentence is too hard to follow ;)
Currently any fix would have one of these issues:
VO won't work with aligned/bordered image, as by config time it is not requested.
VO will use bordered image but will display wrongly scaled (by few pixels) picture 

All of the fixes so far have one of these issues. The only workaround is to
have explicit passing of visible dimensions (v_height,v_width), physical
dimensions (height,width) and display dimensions (d_height,d_width) at config.
This is what should be done. Sooner or later, probably later.

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

I do not propose to add this only to this function, but also to all functions
that it calls, all sami-parameter config's. Is it OK? (This function by itself
doesn't do much, it just calls other functions, after img_fmt negotiation).

The image dimensions and parameters should be calculated at config time.
I think that this is the direction that Jindrich is pointing us to.


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

Jindrich already started committing stuff here and there to fix alignment bugs...
I may be wrong at this...
Anyway I wasn't aware that there are problems with the current design, 
so saying that you don't have problems doesn't mean that something is not
broken ;)


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

Pepsi and Fanta Water is all you may get from me. I keep the Coca Cola for myself :P

Wish You Best
   Ivan Kalvachev
  iive