Attempt to ensure pipeline flush on rendertarget change

Attempt to ensure pipeline flush on rendertarget change

Some ATI cards appear not to flush the pipeline correctly, causing textures not to pick up newly-drawn contexts, so do an extra no-op draw when necessary.

BUG=169
TEST=chromium on hulu.com

Committed: http://code.google.com/p/angleproject/source/detail?r=693

[John Bauman, 2011-06-09 22:04:45]

This seems to be the fastest and smallest change that works.

[dgkoch, 2011-06-10 03:00:14]

I'll let Vangelis LGTM as appropriate

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp
File src/libGLESv2/Blit.cpp (right):

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp#newcode326
src/libGLESv2/Blit.cpp:326: bool Blit::emptyBlit()
Don't you need saveState()/restoreState() calls in here, as well as configuring
some of the other state to know values (like formatConvert and boxFilter do)?
Otherwise the behaviour may be dependant on order of operations and dirty other
state that isn't flagged…
 or is the caller expected to verify that in this case?

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1587: if (identifier->VendorId != 0x1002)
It would be nice to be able to restrict this to a smaller set of ATI cards if we
could.  I couldn't reproduce the issue on a HD 2600, so I suspect it maybe only
occurs on certain generations of cards…  That said, I guess if something is
needed for expediency -- this will suffice.

[John Bauman, 2011-06-10 05:27:36]

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp
File src/libGLESv2/Blit.cpp (right):

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp#newcode326
src/libGLESv2/Blit.cpp:326: bool Blit::emptyBlit()
On 2011/06/10 03:00:14, dgkoch wrote:
> Don't you need saveState()/restoreState() calls in here, as well as
configuring
> some of the other state to know values (like formatConvert and boxFilter do)?
> Otherwise the behaviour may be dependant on order of operations and dirty
other
> state that isn't flagged…
>  or is the caller expected to verify that in this case?
We don't really care what the other state is, as we're not actually going to be
doing much with it. As long as the looks like a draw but doesn't actually draw
anything it should be ok.

Also, I didn't want to do saveState and restoreState, as they involve
stateblocks and can be too expensive to do every time the render target changes.
The caller is expected to restore all the changed state when necessary.
Currently we restore all the changed state at the next draw call, so it works
fine as is.

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Context.cpp#newcod...
src/libGLESv2/Context.cpp:1587: if (identifier->VendorId != 0x1002)
On 2011/06/10 03:00:14, dgkoch wrote:
> It would be nice to be able to restrict this to a smaller set of ATI cards if
we
> could.  I couldn't reproduce the issue on a HD 2600, so I suspect it maybe
only
> occurs on certain generations of cards…  That said, I guess if something is
> needed for expediency -- this will suffice.

Yeah, I'd like to reduce this some more, but I don't really know what range of
cards are affected. I think it's probably better to be too cautious, rather than
accidentally allow some people to experience buggy behavior.

[dgkoch, 2011-06-10 17:40:55]

LGTM then (once you add the comment), provided you've tested it fairly well

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp
File src/libGLESv2/Blit.cpp (right):

http://codereview.appspot.com/4576051/diff/1/src/libGLESv2/Blit.cpp#newcode326
src/libGLESv2/Blit.cpp:326: bool Blit::emptyBlit()
On 2011/06/10 05:27:37, John Bauman wrote:
> On 2011/06/10 03:00:14, dgkoch wrote:
> > Don't you need saveState()/restoreState() calls in here, as well as
> configuring
> > some of the other state to know values (like formatConvert and boxFilter
do)?
> > Otherwise the behaviour may be dependant on order of operations and dirty
> other
> > state that isn't flagged…
> >  or is the caller expected to verify that in this case?
> We don't really care what the other state is, as we're not actually going to
be
> doing much with it. As long as the looks like a draw but doesn't actually draw
> anything it should be ok.
> 
> Also, I didn't want to do saveState and restoreState, as they involve
> stateblocks and can be too expensive to do every time the render target
changes.
> The caller is expected to restore all the changed state when necessary.
> Currently we restore all the changed state at the next draw call, so it works
> fine as is.
> 
Please include a comment to this effect for emptyBlit then.

[John Bauman, 2011-06-13 21:28:22]

Ok, here's a different workaround that shouldn't have nearly as many performance
issues. Apparently if we always avoid setting the color write enable to 0 the
problem goes away.

[nicolas, 2011-06-14 07:22:45]

That's an intriguing driver bug. Nice find by the way!

Patch looks good as far as I'm concerned.

[nicolas, 2011-06-14 07:27:40]

Did spot a couple of things...

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1892: if (!colorMask && !zeroColorMaskAllowed)
Nitpick: Perhaps write colorMask == 0 instead of !colorMask (makes it more clear
its an integer instead of a boolean).

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1896: device->SetRenderState(D3DRS_ALPHABLENDENABLE,
TRUE);
I believe D3DRS_SEPARATEALPHABLENDENABLE should be disabled here.

[John Bauman, 2011-06-14 07:38:38]

Thanks for looking at this.

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1892: if (!colorMask && !zeroColorMaskAllowed)
On 2011/06/14 07:27:40, nicolas wrote:
> Nitpick: Perhaps write colorMask == 0 instead of !colorMask (makes it more
clear
> its an integer instead of a boolean).
Yeah, I guess that makes sense.

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1896: device->SetRenderState(D3DRS_ALPHABLENDENABLE,
TRUE);
On 2011/06/14 07:27:40, nicolas wrote:
> I believe D3DRS_SEPARATEALPHABLENDENABLE should be disabled here.
We've disabled writing to the alpha channel so that doesn't affect anything, and
I wanted to reduce the number of state changes required.

[dgkoch, 2011-06-14 14:09:17]

Look as reasonable to me, with that colormask compare bit changed.

[vangelis, 2011-06-16 00:25:12]

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/4576051/diff/10001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:1742: // Apparently some ATI cards have a bug where a
draw with a zero color
nity-nit:  can we move the comment up before the setting of the bool to make it
clear what part of the code it refers to.  It's customary to have comments
proceed the code.