Xv Reput/Reget behaviour changes

[Ville Syrjälä]

On Sat, Oct 30, 2010 at 02:57:25PM +0200, ext Luc Verhaegen wrote:
> On Fri, Oct 29, 2010 at 09:18:56PM +0300, ville.syrjala at nokia.com wrote:
> > I'm trying to eliminate some rather nasty looking on/off blinking
> > that's bothering my video overlays.
> > 
> > The xfree86 xv code hooks into ClipNotify, WindowExposures and
> > AdjustFrame and does something a little different in each of them.
> > I tried to decipher the intention of the original code, but even
> > after trawling through the xfree86 cvs, I was unable to find any
> > explanation for most of the differences. I suspect the code reached
> > it's current state simply due to some ad-hoc copy pasting in an
> > effort to fix some specific issues.
> > 
> > So I just gave up on the history and tried to think what would make
> > the most sense for each case and did that. The end result is that
> > Reput/Reget is done in most cases if the window is visible,
> > StopVideo is done if the window is invisible, and if ReputImage
> > isn't supported the port is removed from the window.
> > 
> > With these changes most of the blinking is gone. There is still at
> > least one case left though. Window (un)redirection does an internal
> > UnmapWindow+MapWindow cycle which causes ClipNotify to get called with
> > visibility = NotViewable. If anyone has a suggestion how to eliminate
> > that problem without a majore rewrite, I'd be happy to try it.
> > 
> > I'd also like the overlays to track RandR state properly so I added a
> > new hook 'ModeSet' that gets called when something interesting happens.
> > I just realized that I should probably call it from
> > xf86DisableUnusedFunctions() as well. Hmm, actually it looks like
> > xf86DisableUnusedFunctions() already has this crtc_notify hook I could
> > maybe use. Would there be objections to adding more calls to that
> > hook from set_origin (and perhaps some other mode setting functions)?
> > 
> > Also CCing Luc since he did something for xf86XVAdjustFrame at some
> > point in the past...
> 
> Hi Ville,
> 
> Since i have a rather busy time now (family visits, and then visiting 
> the nokia family later next week), i will only be able to review this 
> properly on the train over to the munich airport on thursday. Sadly, i 
> will not be able to give this code a run on a reputimage capable driver 
> in at least two weeks (i am not dragging a unichrome over to helsinki 
> :))
> 
> The modeset hook does make me frown, that is a serious addition to the 
> api, which will make backwards compatibility (of for instance the omapfb 
> driver) impossible.

You mean this? http://cgit.pingu.fi/xf86-video-omapfb/
It doesn't seem to have any RandR support so it should not hit the new
hook.

> I am not sure whether this is really necessary. Can 
> you explain why this is needed and why driver internal modesetting 
> cannot handle this?

I suppose you can push everything to the driver if you want, but then
why is AdjustFrame handled by the xf86xv code?

Also AdjustFrame is most conveniently implemented by simply calling
set_origin on the crtc, and if you do that you end up doing two Reputs
for one AdjustFrame operation (one via the driver's set_origin hook and
another one via the xf86xv AdjustFrame hook).

Drivers would also need to keep more Xv state around (most of the
original arguments passed to PutImage) to be able to recalculate the
clipping and scaling. In fact they already have to do some of that to
support ReputImage since ReputImage doesn't pass all the necessary
coordinates to the driver, but that's something I'd like to change.
ReputImage is also somewhat poorly named since it will be used for
stills as well as images.

If there's some real compatibility issue with the hook, I could perhaps
add a new flag to the Xv registration which would allow the driver to
specify whether the hook is used or not.



BTW another problem with ReputImage occured to me when I was thinking
about this stuff. If the original PutImage/Still is fully clipped the
port is never marked as ON/PENDING and the driver's hook is never
called. Thus ReputImage will never be called and that image/still
never gets shown even if the window clipping changes. That seems OK
if PutImage/Still is supposed to work like a normal XPutImage.

However when the PutImage/Still is not fully clipped, the area covered
by the image/still can actually expand if the window clip changes. That
seems wrong, and doesn't match XPutImage behaviour. So I was thinking
of making the composite clip unable to grow, even if the window clip
grows. I suppose for PutVideo/GetVideo it would be OK to allow the
composite clip to grow since they're not supposed to be one-shot
operations.

That also means that CLIP_TO_VIEWPORT is fundementally broken wrt.
ReputImage as it can cause the initial PutImage/Still to be fully
clipped, and thus ReputImage is never called even if the viewport
moves to a more favorable position later.

-- 
Ville Syrjälä