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

[D Richard Felker III]

On Tue, Nov 23, 2004 at 06:05:38PM +0100, Jindrich Makovicka wrote:
> D Richard Felker III wrote:
> >On Tue, Nov 23, 2004 at 08:57:15AM +0100, Jindrich Makovicka wrote:
> >
> >>Hi,
> >>
> >>the attached patch modifies the *_get_image to make possible to specify 
> >>an explicit horizontal and vertical alignment. This issue has already 
> >>been discussed in the thread
> >>
> >>http://mplayerhq.hu/pipermail/mplayer-dev-eng/2004-October/030502.html
> >>
> >>The patch
> >>
> >>1) renames vf_get_image to vf_get_image_aligned and adds two additional 
> >>parameters, which specify alignment_bytecount-1 (only 2^n-1 are valid)
> >>
> >>2) creates vf_get_image as wrappers calling vf_get_image_aligned with 
> >>the proper alignment parameters - height_align=0, width_align=either 15 
> >>or 0 depending on MP_IMGFLAG_ACCEPT_ALIGNED_STRIDE.
> >>
> >>3) changes all (they are only a few) occurences of vf_get_image and 
> >>mpcodecs_get_image with aligned dimensions to calls of 
> >>_get_image_aligned with original dimension and explicit specification of 
> >>the alignment.
> >
> >
> >could you please explain why this is necessary? i was under the
> >impression it was just a bug in the codec. stride alignment is already
> >done, anyway...
> 
> as i wrote in cvslog, the current api doesn't allow to allocate aligned 
> images correctly - you can only choose, where you prefer the access 
> violation to occur. if you allocate the exact picture size without 
> safety bands, it can crash in the decoder. with the safety bands, it can 
> crash in the VO.
> 
> when the codec calls get_image with the enlarged size, the elnarged mpi 
> can be eventually propagated up to the VO, which was set up with the 
> original (smaller) size and already allocated its buffer to suit it. 
> this occurs unless you use slices, where the codec's draw_slice usually 
> does the bounds checking. another approach is to do boundary checks in 
> every VO, which is obviously painful, as the VO's handling of the images 
> is a now inconsistent enough to introduce further ad-hocism.

imo you totally misunderstand the video layer and the meaning of the
different fields of mp_image! the actual image area that should be
processed is mpi->w by mpi->h. stride (and possibly the bogus "width"
and "height" fields of mp_image) are used for alignment.

> >with that said, i'd like to recommend a different api. rather than
> >requesting alignment of allocated width/height, the caller should
> >request a (minimum) amount of 'border space' it wants writable on all
> >sides of the image, including top and left. this type of design works
> >even with more complex filter chains.
> 
> sounds reasonable and the code can be easily adapted.
> 
> >furthermore, ALL of this (image size, border, etc.) should be decided
> >at CONFIG TIME, not everytime get_image is called. get_image should
> >implicitly assume the same sizes config() was called with, otherwise
> >it's nonsense. of course this would be a major pain to fix up in g1...
> >let's discuss it more.
> 
> what about vf_scale, vf_expand etc. ? maybe config() could also return 

huh?

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

rich