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

[D Richard Felker III]

On Thu, Dec 02, 2004 at 04:30:30PM +0200, Ivan Kalvachev wrote:
> 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!!!

NO!!!
Alignment is totally different. It's for when the code that processes
the image needs each line to start on an aligned boundary for
word-access (or vector access).

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

I disagree. Unless a filter was broken before, this patch can only
make things less likely to crash. :)

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

But they use these for the WRONG PURPOSE! I know because I'm the one
who wrote those filters, and I didn't understand how brain damaged
Arpi's vf api was at the time... :)

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

These names are dumb. They should be visible dimensions (width/height
or w/h, either name is ok), borders (left, top, right, and bottom --
should be possible to have borders on EITHER side, not just
right/bottom!!!!!), and pixel aspect ratio (d_width/d_height is stupid
as discussed many times..).

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

I agree if we use the above more-general system. But it's a big
overhaul and not worth it for g1's shitty filter layer, IMO. Instead
save it for a new filter layer. In the mean time, the committed fix
will prevent segfaults and WILL WORK IN ALL CURRENT CASES. The only
problem is that it may prevent DR with shitty movies.

Rich