Merge lp://qastaging/~vanvugt/compiz-core/fix-880707.2 into lp://qastaging/compiz-core/0.9.5

This proposal also appears to solve LP: #798868 and LP: #876575.

This will be merged as soon as the testing branch in https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.fix_880707.2.test/+merge/82961 is complete

(I also don't like working around driver bugs inside of the opengl plugin since these can break if the driver developers change something. If you need to add a workaround, add it to the workarounds plugin by replacing the GL::getVideoSync function pointer with your work-around wrapper)

I'm generalizing the "driver bug workaround" today because similar code is needed for all drivers in some circumstances.

Done. The changes to plugins/opengl/src/screen.cpp are no longer just for nouveau, but now important for all users.

Major improvement today. Reduced tearing enough to also resolve bug 755841.

Daniel, is there a specific reason this change won't make into Precise? I have noticed real positive improvements on on Intel(smooth minimize animations), nvidia-current (no tearing) and nouvea (no tearing)

Omer, you misinterpreted my comment in bug 880707. What I meant was that this fix is so complex that it won't go into any oneiric updates. But getting it into precise is my absolute top priority.

I agree; the number of improvements this branch brings is much larger than I ever envisioned. I never even noticed the stuttering bugs until I saw they were fixed by this code. It was a nice surprise.

We do absolutely want everyone to have these improvements by the time precise is released. :)

I'm going to have another look at how we can do automated testing from this branch again tomorrow.

I already did automate testing... using an army of testers now subscribed to ppa:vanvugt/compiz ;)

OK, maybe you meant a different kind of automated...

Indeed :)

I've started updating lp:~smspillaz/compiz-core/fix-880707.2.test with the stuff we discussed there earlier. Namely testing for phase (eg, that it doesn't take > 4ms to get from a vblank begin fence to finishing a swapbuffers / paint) and also testing that we don't fall out of phase when we are throttling the framerate with the composite plugin.

I don't really think its possible to be testing for an exact fix to bug 880707 without instantiating a separate opengl context and running out constructs in there, but doing that is going to be far too much effort than its worth really.

Please let me know of any other parts of the composite plugin's code that you think can be tested.

(correction: lp:~smspillaz/compiz-core/compiz-core.fix_880707.2.test)

(and thank you for setting up the PPA)

Comments in the other branch.

More work may be required on this proposal. Some anecdotal evidence from a minority of testers, and a brief test on a slow netbook, both suggest that this proposal can make performance worse. I will need to look into this.

Sure. I'll keep working on testcases for the current stuff we've got and rebase on any other work you come up with when that happens.

Fixed the slow-motion animations problem in r2900.

I agree with fixing the plugins for what its worth.

Hey Daniel,

I had a think about the problem that you were encountering with the plugins. As you said earlier, the plugins are expecting a situation where msSinceLastPaint is going to be reflective of optimalRedrawTime, but with the tickless algorithm, msSinceLastPaint could be quite large. On slower systems, if it took > optimalRedrawTime to actually process a paint, then forcing msSinceLastPaint to be optimalRedrawTime would result in slow animations since frame skipping would not work correctly.

Since the only case we really wanted to cover was the case where the screen went from idle to active, what do you think about doing something like this:

    struct timeval tv;
    int timeDiff;

    gettimeofday (&tv, 0);

    timeDiff = TIMEVALDIFF (&tv, &mLastRedraw);

    /* handle clock rollback */

    if (timeDiff < 0)
 timeDiff = 0;

    /*
     * Now that we use a "tickless" timing algorithm, timeDiff could be
     * very large if the screen is truely idle.
     * However plugins expect the old behaviour where timeDiff is never
     * larger than the frame rate (optimalRedrawTime).
     * So enforce this to keep animations timed correctly and smooth...
     */

    if (timeDiff > mOptimalRedrawTime && !reschedule)
 timeDiff = mOptimalRedrawTime;

    ...

At least on your branch, reschedule is going to be set every time a plugin requests a redraw in donePaint or addWindowDamage (which is the correct way to do it), so that means that in those cases, msSinceLastPaint is always going to be accurate. And where we have switched from inactive to active, msSinceLastPaint is always going to be mOptimalRedrawTime (in fact, it could be zero, but that might require further testing).

I've updated my changes to reflect this.

Your suggestion looks incorrect. priv->reschedule is always false there. I don't think it will work.

Best to use my r2900 below. Mine has been tested for a few days, whereas yours would appear to probably not work (or compile in that form :)

I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)

(It works quite well here, and I've tested it with all the plugins)

I think that checking if the timeDiff is more than 100 only really works around the original problem which I outlined earlier as well. There's a case where some really slow plugin could take more than 100 ms per frame to process some animation, which would result in perceived slowness in its animation code ;-)

"I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)"

That still makes no sense in my mind. But if you can reproduce the slow animations bug consistently, and have tested and verified your fix works then go nuts. If however this is another code change you're proposing without actually testing it, then please reconsider.

On Thu, 22 Dec 2011, Daniel van Vugt wrote:

> "I forgot to mention - the check would have to be moved to before priv->reschedule has been set to false (I've roughly transposed the code from my branch to yours)"
>
> That still makes no sense in my mind. But if you can reproduce the slow animations bug consistently, and have tested and verified your fix works then go nuts. If however this is another code change you're proposing without actually testing it, then please reconsider.

I'm running it right now and haven't run into any problems whatsoever. I
can probably give it a try on some of my other systems as well, but I put
some cases in to force it to run slowly and it still works then.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-880707.2/+merge/83472
> You are requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-880707.2 into lp:compiz-core.
>

Yep, a simple sleep or something to make painting slow should allow you to reproduce the problem and then verify you're actually fixing it.

In my real-world example, I have an old netbook which takes between 2-4 frames to paint each frame (up to 64ms) during animations. So it's easy to reproduce "slow motion animations" on a machine that only achieves 15-30 FPS.

Oddly enough, as I have mentioned before, this "slow" machine gets a perfect 60 FPS with "Force full screen redraw (buffer swap) on repaint" enabled. But only when using this branch. The difference is staggering.

OK, I think there's been enough discussion here.

I'm confident with the changes this branch is introducing, and think we just need to land it now. I have created an entire testing framework for it, which can be found at lp:~smspillaz/compiz-core/compiz-core.fix_880707.2.test . That test tests for both phase and period timings. On a period of aggressive vblank scheduling for 70 seconds, that test passes each time on my machine.

The testing branch will need to land a little bit after the main one does, once Daniel is ok with it, but ideally it needs to be in before feature freeze.