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

[Ivan Kalvachev]

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.

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 ;)



Wish You Best
  Ivan Kalvachev
 iive



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

> On Wed, Dec 01, 2004 at 03:18:19AM +0200, Ivan Kalvachev wrote:
> > On Tue, 23 Nov 2004 21:56:10 +0100
> > Reimar D_ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> > 
> > > Hi,
> > > > > [...]
> > > > > > > the desired destination size or just write it somewhere to
> > > > > > > vf_instance_s and let it be. get_image would be then
> > > > > > > called with the current and next vf as argument, instead
> > > > > > > of next and x/y-size.
> > > > > > 
> > > > > > again you totally misunderstand the video layer. i don't
> > > > > > have time to test and debug this stuff right now, but imo
> > > > > > someone like myself, ivan, or michael who understands it
> > > > > > should be the one to fix it, rather than you adding crazy
> > > > > > hacks based on misunderstandings.
> > > > > 
> > > > > Oh, I'm sure we would not mind that. If only it looked like
> > > > > this is ever going to happen. This bug is know at least since
> > > > > months, and nobody gave even any useful comment.
> > > > 
> > > > could you do some research then and find out why it doesn't
> > > > happen with vd_ffmpeg.c?
> > > 
> > > No, I can't because it does happen with vd_ffmpeg.c as well (maybe
> > > not in all cases though). See my second patch in the "segfault in
> > > vf_scale" thread.
> > 
> > It usually doesn't happen with normal ffmpeg codecs because most of
> > them support emu_edge, this they don't need alignment.
> > 
> > The problem is real and quite messy.
> > Actually there are few problems:
> > 
> > First. Alignment
> > 
> > The horizontal line alignment is not an issue as it is easily to
> > workaround by flags. The real problem is vertical block alignment,
> > as it is not specified anywhere.
> > 
> > As really really messy workaround we could make 16 lines height
> > alignment when horizontal flag alignment is set.
> > 
> > Second.
> > Video initialization.
> > By legacy reasons the vo is initialized before even first mpi
> > structure is filled. This makes vo_xv and probably other vo allocate
> > memory, init devices even before knowing what alignment they may
> > need.
> 
> This is nonsense. Yes it's true, but in this case direct rendering can
> just fail, no problem. The vo's "get_image" implementation can see
> the requested width/height, and if they don't match the configured
> ones, it can just refuse to do dr. The normal non-dr functions of the
> vo DO NOT CARE about stride/alignment crap for their source image.

This is quite fun, because the current fix is doing just that!
I thought you gave permission for commit? No?


> 
> Granted this is sub-optimal in terms of performance, but it only
> affects shitty files, and it's STILL much better than the current
> situation where these files cause sig11!! Also it's a minimal change
> so we can do something better later if we want.

There are no shitty files. There is only our shitty mess.
 
> > 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.
BTW I was talking about direct rendering.
 
> > Finally.
> > 
> > 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
...


> > Probably an better way to fix it is to pass mpi structure to
> > vo_config(), as mpi already have both visible and "physical"
> > dimensions and alignment info. An better and more generic alignment
> > parameters could be added. Probably adding aspect info to mpi would
> > be interesting change,too ;)
> 
> 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.


> > 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...
 
> > p.s.
> > i wrote this mail right before going to sleep, if I overlooked
> > something, point it kindly;)
> 
> That wouldn't be any fun... ;)
> We need some flames now and then.

Nature doesn't hold empty space, one flamer leaves, another rise.

If you want flames you can go and talk with gabucino, i'm sure he will
never refuse to flame you on any subject.
Don't count on me for flames or I will start to ignore you.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: get_image.patch
Type: application/octet-stream
Size: 4371 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20041201/56bd8568/attachment.obj>