Merge no-flip-rows r1142 to trunk.

Merge no-flip-rows r1142 to trunk.
Committed: https://code.google.com/p/angleproject/source/detail?r=1162

[apatrick1, 2012-06-07 21:03:19]

Not for review just yet, just for reference.

This is what merging no-flip-rows to trunk looks like. Not nearly as bad as I
thought :) I'll patch it into chrome for testing.

Some notes inline on things that didn't merge cleanly.

Also, I wonder if anything needs to be updated for depth textures?

https://codereview.appspot.com/6304052/diff/1/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

https://codereview.appspot.com/6304052/diff/1/src/compiler/OutputHLSL.cpp#new...
src/compiler/OutputHLSL.cpp:288: if (mUsesTexture2DLod0)
The tex lod 0 functions were new to the no-flip-rows branch and I had to change
them to not flip.

https://codereview.appspot.com/6304052/diff/1/src/compiler/OutputHLSL.cpp#new...
src/compiler/OutputHLSL.cpp:1221: outputTriplet(visit, "ddy(", "", ")");
This line had changed.

https://codereview.appspot.com/6304052/diff/1/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

https://codereview.appspot.com/6304052/diff/1/src/libGLESv2/Program.cpp#newco...
src/libGLESv2/Program.cpp:1596: pixelHLSL += "    gl_PointCoord.x =
input.gl_PointCoord.x;\n";
These two lines were changed.

[nicolas, 2012-06-07 21:32:30]

Thanks for rebasing it! Indeed it still looks quite clean. I have a few minor
comments below (stuff I didn't spot before actually).

I'll try to test everything that would be affected by this before Monday...

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

https://codereview.appspot.com/6304052/diff/1/src/libGLESv2/Context.cpp#newco...
src/libGLESv2/Context.cpp:2000: float h = mSupportsShaderModel3 ?
mRenderTargetDesc.Height : mState.viewportHeight / 2.0f;
The Shader Model 3 case can be removed.

https://codereview.appspot.com/6304052/diff/1/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

https://codereview.appspot.com/6304052/diff/1/src/libGLESv2/Program.cpp#newco...
src/libGLESv2/Program.cpp:1579: // dx_Coord.y contains the render target height.
See Context::applyRenderTarget()
dx_Coord.y comment can be removed.

[apatrick1, 2012-06-07 22:00:47]

Thanks. If you plan to use this with chrome you'll want

--disable-gpu-sandbox --disable-image-transport-surface

or everything will turn upside down.

[dgkoch, 2012-06-08 02:09:52]

On 2012/06/07 22:00:47, apatrick1 wrote:
> Thanks. If you plan to use this with chrome you'll want
> 
> --disable-gpu-sandbox --disable-image-transport-surface
> 
> or everything will turn upside down.

I took a quick look over it, and I didn't spot any obvious problems.  I don't
think anything special should be required for depth textures.   Would like to
look at it in more detail next week, and definitely agree that we should do a
round of thorough testing with it.

I presume you'll fix the image-transport case in chrome once this gets enabled?

[apatrick, 2012-06-08 18:22:25]

On Thu, Jun 7, 2012 at 7:09 PM, <daniel@transgaming.com> wrote:

> On 2012/06/07 22:00:47, apatrick1 wrote:
>
>> Thanks. If you plan to use this with chrome you'll want
>>
>
>  --disable-gpu-sandbox --disable-image-transport-**surface
>>
>
>  or everything will turn upside down.
>>
>
> I took a quick look over it, and I didn't spot any obvious problems.  I
> don't think anything special should be required for depth textures.
> Would like to look at it in more detail next week, and definitely agree
> that we should do a round of thorough testing with it.
>
> I presume you'll fix the image-transport case in chrome once this gets
> enabled?
>
>
https://codereview.appspot.**com/6304052/<https://codereview.appspot.com/6304...
>

I have a local build that flips the image transport surface in Chrome when
a command line switch is given. I can check it in if it's helpful.

Not that formal testing should not be done but I've been using the
no-flip-rows branch in one of my two primary clients for a few weeks and
haven't had any issues.

[nicolas, 2012-06-12 18:18:01]

Sorry for the late response. I found that the OES_standard_derivatives WebGL
conformance test fails. I'll try to find out what might be causing that...

[nicolas, 2012-06-12 20:04:48]

The OES_standard_derivatives test was only failing on my ATI system, not on
NVIDIA. Also, the test only fails for pixels on the diagonal of the image...

It turns out that when anti-aliasing is enabled (which is the default), some ATI
hardware evaluates the shader at the centroid location. This skews the results
for the derivatives near a triangle edge.

The good news is that this isn't a bug in this branch. It's a bug in the
conformance test. I'll inform the people involved.

[apatrick1, 2012-06-12 23:23:58]

On 2012/06/12 20:04:48, nicolas wrote:
> The OES_standard_derivatives test was only failing on my ATI system, not on
> NVIDIA. Also, the test only fails for pixels on the diagonal of the image...
> 
> It turns out that when anti-aliasing is enabled (which is the default), some
ATI
> hardware evaluates the shader at the centroid location. This skews the results
> for the derivatives near a triangle edge.
> 
> The good news is that this isn't a bug in this branch. It's a bug in the
> conformance test. I'll inform the people involved.

Interesting. I wonder if there's a way to turn centroid sampling off in D3D9. It
doesn't seem to specify what the multisampled rasterization rules are, unlike
D3D11. I'll do a bit of digging...

[apatrick1, 2012-06-15 00:10:08]

Rebased against r1148. Almost ready to compensate for the row reversal
downstream:

https://chromiumcodereview.appspot.com/9915001/

Any objections?

[dgkoch, 2012-06-19 17:26:32]

Hi Al,

Sorry for the delay on this.  There are a couple of cleanup things in Surface
but other than that, we're nearly there!

Daniel

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:227: if (mDisplay->shareHandleSupported())
Should we be creating the share handle even in the cases we are doing windowed
rendering?  I don't know if creating a share handle affects performance.

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:238: return error(EGL_BAD_ALLOC, false);
Need the device lost check in here:

        if(isDeviceLostError(result))
        {
            mDisplay->notifyDeviceLost();
            return false;
        }
        else
        {
            return error(EGL_BAD_ALLOC, false);
        }

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:264: result = device->StretchRect(oldRenderTarget, &rect,
mRenderTarget, &rect, D3DTEXF_NONE);
Shouldn't we still have the endScene() before this?

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:305: return error(EGL_BAD_ALLOC, false);
We should likely have the isDeviceLostError check in here to as well.

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:318: if (x < 0 || y < 0 || width < 0 || height < 0)
I'd much rather this check be moved out to the eglPostSubBufferNV entry point.

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:360: HRESULT result = mSwapChain->GetBackBuffer(0,
D3DBACKBUFFER_TYPE_MONO, &backBuffer);
It would be nice if we cached this instead of looking it up every present call.

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:363: device->SetRenderTarget(0, backBuffer);
perhaps we should also SetDepthStencilSurface(NULL)? I know you disabled depth
and stencil writes, but what if it was a weird size?

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:419: if (result == D3DERR_DEVICELOST || result ==
D3DERR_DEVICEHUNG || result == D3DERR_DEVICEREMOVED)
Need to use the device lost check here:

      if (isDeviceLostError(result))
        {
            mDisplay->notifyDeviceLost();
            return false;
        }

https://codereview.appspot.com/6304052/diff/12001/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

https://codereview.appspot.com/6304052/diff/12001/src/libGLESv2/Context.cpp#n...
src/libGLESv2/Context.cpp:2008: // These values are used for computing
gl_FragCoord in Program::linkVaryings(). The approach depends on Shader Model
3.0 support.
You can remove the second half of this comment now.

[apatrick1, 2012-06-19 20:31:01]

Done I think. PTAL.

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
src/libEGL/Surface.cpp:227: if (mDisplay->shareHandleSupported())
On 2012/06/19 17:26:32, dgkoch wrote:
> Should we be creating the share handle even in the cases we are doing windowed
> rendering?  I don't know if creating a share handle affects performance.

This is (unintentionally) new behavior. Looking at how
ANGLE_query_surface_pointer is specified, getting a share handle for a window
surface is not disallowed. I don't need to get share handles for window surfaces
and am happy to put it back the way it was. Alternatively, maybe the
"sharability" of a surface should be part of the config. That would be a
breaking change though.

Will change:
if (mDisplay->shareHandleSupported())

to:
if (!mWindow && mDisplay->shareHandleSupported())

if you want.

[dgkoch, 2012-06-20 16:12:07]

On 2012/06/19 20:31:01, apatrick1 wrote:
> Done I think. PTAL.
> 
> https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp
> File src/libEGL/Surface.cpp (right):
> 
>
https://codereview.appspot.com/6304052/diff/12001/src/libEGL/Surface.cpp#newc...
> src/libEGL/Surface.cpp:227: if (mDisplay->shareHandleSupported())
> On 2012/06/19 17:26:32, dgkoch wrote:
> > Should we be creating the share handle even in the cases we are doing
windowed
> > rendering?  I don't know if creating a share handle affects performance.
> 
> This is (unintentionally) new behavior. Looking at how
> ANGLE_query_surface_pointer is specified, getting a share handle for a window
> surface is not disallowed. I don't need to get share handles for window
surfaces
> and am happy to put it back the way it was. Alternatively, maybe the
> "sharability" of a surface should be part of the config. That would be a
> breaking change though.
> 
> Will change:
> if (mDisplay->shareHandleSupported())
> 
> to:
> if (!mWindow && mDisplay->shareHandleSupported())
> 
> if you want.

Being able to get a share handle from the windowed cases kind of relies on the
exact implementation of how we do the windowed rendering (i.e. if we render to a
texture and flip on present, it'll work.  If we change that again, it won't). 
So rather than expose a capability that we may not always be able to support, I
would prefer if you changed that line as suggested.

Reviewing the rest now.

[dgkoch, 2012-06-20 16:21:11]

Almost done! Just 2 minor things.

https://codereview.appspot.com/6304052/diff/4002/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

https://codereview.appspot.com/6304052/diff/4002/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:241: if (mDisplay->shareHandleSupported())
please do add in !mWindow into this condition.

https://codereview.appspot.com/6304052/diff/4002/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:442: if (result == D3DERR_OUTOFVIDEOMEMORY || result ==
E_OUTOFMEMORY || result == D3DERR_DRIVERINTERNALERROR)
I'd rather this error check comes after the isDeviceLostError check (and we do
consider the DRIVERINTERNALERROR a "loss").

[apatrick1, 2012-06-21 19:04:47]

Done.

[dgkoch, 2012-06-22 03:25:11]

LGTM