All surfaces follow D3D Y convention

All surfaces follow D3D Y convention, i.e. (0, 0) is "top-left" rather than GL's "bottom-left". This eliminates the need to flip the default FBO to the D3D convention using additional blits when presenting and reduces VRAM usage for redundant window sized surfaces.

I took out the gl_Position.y flip from the vertex shader so FBOs are rendered
according to D3D conventions.

Texture lookups are flipped on Y to compensate. Cube map +Y and -Y faces are swapped. Y is now flipped in various other places, including uploading and reading back texture data from / to system memory, functions that take pixel space coordinates, winding order for culling, the implementation of ddy, the calculation of gl_Position and gl_FragCoord in fragment shaders and the flipping of compressed texture tiles.

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

[apatrick1, 2010-11-23 02:04:31]

Hi Daniel,

I'm sure this will need considerably more work but I think this is ready for
some feedback.

Thanks,

Al

[dgkoch, 2010-11-23 22:06:15]

Haven't had a chanced to go over this one yet in detail, but I wanted to mention
a couple of other cases that may need to be checked:
 - test renderable textures
 - test interactions between the default and non-default FBOs 
     - ie CopyTex{Sub}Image and especially glBlitFramebufferANGLE

[dgkoch, 2010-11-24 21:07:06]

Adding Nicolas in as a reviewer.

Our previous Surface::swap definitely seems more complex that it should have
been. I haven't been able to figure out why we needed the backbuffer, a render
target, AND a texture.  Nicolas, do recall why we had so many? 

So far I haven't been able to spot any problems with this, other than my
concerns noted earlier (make sure Blits from default<->non-default FBOs are
oriented properly, as are copyTeximage from default, non-default FBOs).

I'd also like to see testing of ddy, ensure the CTS and WebGL CTS still pass,
and all possible webgl content and any other stand-alone programs run without
problems.  Once you think this is done, we can also do a pass of testing here
too.

Daniel

[apatrick1, 2010-11-24 23:14:02]

This new approach is getting increasingly complicated. I found I have to reverse
the culling mode for the default FBO, which also means flagging the front face
state as invalid in the context when a new draw FBO is bound.

The SM3 optimized calculation of gl_FragPoint.y in fragment shaders also stopped
working because VPOS y is flipped. I just fell back to the SM2 approach because
I think the SM3 differences are just an optimization? Calculation of
gl_FragPoint.x does not need to be changed I think.

My modifications to dFdy(x) did not work. I now translate it like this:

ddy(x) * sign(ddy(gl_FragCoord.y))

which should work because in GLSL, sign(ddy(gl_FragCoord.y)) should always equal
1. Alternatively, I could add a uniform for fragment shaders as well.

No patch yet; I'll send over something next week.

[kbr1, 2010-11-24 23:40:46]

On 2010/11/24 23:14:02, apatrick1 wrote:
> This new approach is getting increasingly complicated. I found I have to
reverse
> the culling mode for the default FBO, which also means flagging the front face
> state as invalid in the context when a new draw FBO is bound.
> 
> The SM3 optimized calculation of gl_FragPoint.y in fragment shaders also
stopped
> working because VPOS y is flipped. I just fell back to the SM2 approach
because
> I think the SM3 differences are just an optimization? Calculation of
> gl_FragPoint.x does not need to be changed I think.

No, the use of VPOS is a correctness issue with point sprites of size > 1.

> My modifications to dFdy(x) did not work. I now translate it like this:
> 
> ddy(x) * sign(ddy(gl_FragCoord.y))
> 
> which should work because in GLSL, sign(ddy(gl_FragCoord.y)) should always
equal
> 1. Alternatively, I could add a uniform for fragment shaders as well.
> 
> No patch yet; I'll send over something next week.

[kbr1, 2010-11-24 23:41:44]

On 2010/11/24 23:40:46, kbr1 wrote:
> On 2010/11/24 23:14:02, apatrick1 wrote:
> > This new approach is getting increasingly complicated. I found I have to
> reverse
> > the culling mode for the default FBO, which also means flagging the front
face
> > state as invalid in the context when a new draw FBO is bound.
> > 
> > The SM3 optimized calculation of gl_FragPoint.y in fragment shaders also
> stopped
> > working because VPOS y is flipped. I just fell back to the SM2 approach
> because
> > I think the SM3 differences are just an optimization? Calculation of
> > gl_FragPoint.x does not need to be changed I think.
> 
> No, the use of VPOS is a correctness issue with point sprites of size > 1.

See http://code.google.com/p/angleproject/issues/detail?id=71 for a test case.

> > My modifications to dFdy(x) did not work. I now translate it like this:
> > 
> > ddy(x) * sign(ddy(gl_FragCoord.y))
> > 
> > which should work because in GLSL, sign(ddy(gl_FragCoord.y)) should always
> equal
> > 1. Alternatively, I could add a uniform for fragment shaders as well.
> > 
> > No patch yet; I'll send over something next week.

[dgkoch, 2010-11-25 19:10:41]

Ah yes -- FrontFace and CullMode (and thus all the stencil operations) need to
factor in the inversion as well.

After looking at our existing swap a bit more, I remembered why we have the
extra RenderTarget and StretchRect there.  They are necessary if the EGL surface
is multisampled.  There is no way to create a multisample texture in D3D9.  That
said, we don't actually advertise MS configs from EGL at the present, so we can
optimize that step out when not required.   We'll take a crack at that over the
next couple days.  This will eliminate one of the render targets as well as the
StretchRect, so this should accelerate this path a fair bit.

[apatrick1, 2010-12-02 01:35:25]

I think the end might be in sight!

I ended up adding a fragment shader uniform for y scale. I could have done
without it were it not for gl_FragCoord in point sprite fragment shaders. It did
not affect the number of registers available.

I tested all variants of the copyTexImage function for both 2D textures and cube
map textures and for both default and non-default FBOs by modifying the mip map
and cube map samples respectively and enabling and disabling the overrides in
DefaultFramebuffer. Ditto for blits. I inspected the resulting copied pixels
were for the right sub-rectangle and had the right vertical orientation using
Pix.

I tested ddy by modifying the simple texture sample's shader to render the
gradient of the interpolated u and v coordinates.

I tested gl_FragCoord in a similar way, independently for meshes and point
sprites, this time with a modified version of the particle system sample. I
checked that gl_PointCoord did not need any changes to compensate for flipping
on y.

Similarly I used modified samples to check that culling was working by turning
it on in the hello triangle sample.

I tested all the samples on the Khronos wiki, all the samples at
http://code.google.com/p/webglsamples/, all the WebGL content at
http://planet-webgl.org/ that I know to work. I tested the repro case that kbr
noted previously.

I checked the WebGL conformance tests for regressions. There was one relating to
calling the copyTexImage functions with bad arguments that I have noted inline.

I'm sure I missed something though.

[apatrick1, 2010-12-02 01:35:43]

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

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3531: // Fails for sub-rectangles and if source is
default FBO while dest is non-default FBO, which would require a vertical flip.
I couldn't figure out a way around this. I considered applying StretchRect to
individual rows in reverse order but StretchRect appears to only supports
copying the whole surface rather than subrectangles. Any thoughts?

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:1324: sourceRect.left = clamp(sourceRect.left, 0,
source->getColorbuffer()->getWidth());
There appears to be an issue where a color buffer's storage's dimensions get
out-of-sync with the associated texture's dimensions. That is making the WebGL
conformance tests assert when calling these functions with deliberately bad
arguments. Repro at
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/more/....

[dgkoch, 2010-12-07 04:29:20]

Have you measured any performance benefits from this approach?

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

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3503: // StretchRect does not support negative
scaling.
What if you flipped the dimensions of the rectangle around -- or is the flip
actually required here?

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3517:
getBlitter()->formatConvert(readFramebuffer->getRenderTarget(),
this won't work for multisample resolve (ie where the source is multi-sampled).

Presumably you'd have to do this as a two-step process using a temporary surface
-- this is something we tried really hard to avoid in the implementation so
far...

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3531: // Fails for sub-rectangles and if source is
default FBO while dest is non-default FBO, which would require a vertical flip.
On 2010/12/02 01:35:43, apatrick1 wrote:
> I couldn't figure out a way around this. I considered applying StretchRect to
> individual rows in reverse order but StretchRect appears to only supports
> copying the whole surface rather than subrectangles. Any thoughts?

Correct.  MSDN says that for DepthStencil surfaces:
 * The entire surface must be copied (that is: sub-rectangle copies are not
allowed).
 * Format conversion, stretching, and shrinking are not supported.

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:1324: sourceRect.left = clamp(sourceRect.left, 0,
source->getColorbuffer()->getWidth());
On 2010/12/02 01:35:43, apatrick1 wrote:
> There appears to be an issue where a color buffer's storage's dimensions get
> out-of-sync with the associated texture's dimensions. 

This is probably related to Issues 51 and 52.

[dgkoch, 2010-12-07 04:32:34]

I don't have any good suggestions for the unsupported cases in BlitFramebuffer
at the moment.  That is the one of the cases that I had some serious concerns
about the feasibility of this approach…

Daniel

[apatrick1, 2010-12-07 19:12:23]

Thanks for the feedback.

To clarify, this patch is more related to stability than performance, though
there is a possibility that it could improve performance. It depends on whether
the savings on the extra blit and reduced memory pressure  make up for the
additional costs of flipping vertically in the shaders and suchlike and that
will depend on the application.

The issue I am actually having is that, for some applications, ANGLE tends to
reset my graphics adapter when it is rendering to a large window, particularly
when I repeatedly resize the window.

Obviously ANGLE cannot prevent an application from allocating render targets and
other resources to the point of failure. That will need to be handled at a
higher level. I would like to try and get parity with OpenGL though.

To that end, I am trying to remove as many surfaces that are locked into video
memory as possible, particularly those that are likely to be large and regularly
reallocated, which might lead to fragmentation, such as those related to the
default FBO.

In terms of measurement, with this patch I am able to run the aquarium demo with
ANGLE at 2560 x 1600 without it resetting my graphics adapter  (Vista 64-bit +
Quadro FX 380 with 256MB). Likewise, I am able to run it with OpenGL. Without
the patch, the graphics adapter resets. Not particularly scientific, I know :)

http://webglsamples.googlecode.com/hg/aquarium/aquarium.html

Note that there is a bug in the aquarium demo linked above that the author is
reluctant to fix, possibly to encourage someone to fix it in ANGLE, that causes
it to allocate a very large multisampled non-default FBO and associated
attachments. It is rounded up to the power of two larger than the window (4096 x
2048 in my case). I am using a locally modified version of the aquarium demo
that clamps to the actual window size (2560 x 1600) and I still have issues.

[apatrick1, 2010-12-07 19:21:06]

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

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3503: // StretchRect does not support negative
scaling.
I tried that and it didn't work. I think D3D considered it to be an invalid
argument when top was greater than bottom.

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3517:
getBlitter()->formatConvert(readFramebuffer->getRenderTarget(),
Agreed. This of course depends entirely on the application but I think I prefer
a two-step process for this one code path than having a two-step copy for the
Present. The latter will affect virtually every application. I'll take a look
into implementing a two-step copy.

http://codereview.appspot.com/3265041/diff/12001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:3531: // Fails for sub-rectangles and if source is
default FBO while dest is non-default FBO, which would require a vertical flip.
Right, this might be a deal-breaker, which is unfortunate because I bet this is
going to be a fairly rarely used feature.

[apatrick1, 2010-12-09 00:43:46]

Daniel,

I tried a completely different approach. The idea is to have all surfaces follow
the D3D Y convention. This means there is never any need for BlitFramebuffer to
flip vertically and the multisampling and depth/stencil buffers in the previous
patch are avoided. I'm sure this idea has its own issues though.

This new patch isn't ready for full review. I have barely tested any of it. I
think it illustrates the general approach though and I would welcome early
feedback.

I took out the gl_Position.y flip from the vertex shader so FBOs are rendered
according to D3D conventsions.

The main problem now is renderable textures become vertically flipped according
to OpenGL conventions. I compensated for that by flipping the Y texture
coordinate passed to texture lookup functions in the translated pixel shader for
all textures (renderable and non-renderable) and exchanging the +Y and -Y faces
of cube maps.

Since all textures are vertically flipped, I modified ReadPixels and TexImage2D
to reverse this CPU side prior to transfer to / from the GPU. This was somewhat
complicated for compressed textures because the tiles need to be flipped as
well.

I think this is simpler than the previous approach and has fewer paths, so it
might be easier to test.

Thanks,

Al

[apatrick1, 2010-12-14 23:39:29]

Daniel,

I tested all the changed code with a combination of running modified ANGLE
samples and WebGL content on the web, pretty much as before. It seems to work
but I no doubt missed something important.

I also ran the aquarium demo with various settings to see if there was any
performance regression. At 2560 x 1600 on my Quadro FX 380 with vsync lock
disabled:

50 fish, ANGLE without this patch - 45 FPS
50 fish, ANGLE with this patch - 51 FPS
50 fish, OpenGL - 50 FPS

1000 fish, ANGLE without this patch - 23 FPS
1000 fish, ANGLE with this patch - 23 FPS
1000 fish, OpenGL - 41 FPS

Despite the overhead in the pixel shader to flip Y for texture lookups, for this
demo, it looks like a net win. It looks like the aquarium demo might be vertex
bound now. The patch also saves about 16MB of video memory at 2560 x 1600 and it
no longer resets my display when I repeatedly resize the Chrome window.

I looked at the code the shader compiler generates to do the flip. A typical
texture2D call is now accompanied by an instruction to multiply the Y component
of the source register by -1, the -1 being stored in a constant register.

Ready for review.

Thanks,

Al

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/utilities.cpp
File src/libGLESv2/utilities.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/utilities.cpp#...
src/libGLESv2/utilities.cpp:192: return ComputeCompressedSize(width, 1, format);
I think width can be 1 or 2 here, for example if the compressed texture has a
complete set of mipmaps.

[nicolas, 2010-12-16 15:24:29]

I was concerned that rendering to a cube map might not work correctly with this
approach, but after closely comparing the D3D and GL coordinate systems for cube
maps it looks like it might just work out fine by simply swapping the top and
bottom faces. Great! To be 100% sure it would be best to have a test for it
though.

I'm not sure if the use of faceIndex() is correct for all cases though. It would
probably be a good idea to have a clearer distinction between the index used for
TextureCubeMap::mImageArray[] and the one used for
IDirect3DCubeTexture9::GetCubeMapSurface(). At the least
TextureCubeMap::getCubeMapSurface() should settle on using just one index type.

I'll continue to look for other potential issues, but overall this looks very
promising! With a tiny bit more separation between the GL and D3D side of things
this could even end up being quite elegant.

[apatrick1, 2010-12-17 01:26:08]

I tested the cube map flipping by dropping the modified ANGLE into Chrome and
going to some WebGL demos that use them. The aquarium demo is a good one. The
skybox is a cube map.

http://webglsamples.googlecode.com/hg/aquarium/aquarium.html

[dgkoch, 2010-12-22 04:21:17]

Hi Al,
Sorry I haven't had a chance to go over this in detail yet, I'm planning to do
that tomorrow.  One thing I just realized though -- this will completely break
the pbuffer/sharing approach that the mozilla folks are using for their
compositing (see Issue #91).  They rely on being able to get the share handle
from the offscreen texture we create for the Surface for their compositor
implementation.

Daniel

[dgkoch, 2010-12-23 20:19:22]

Overall this is looking reasonable.  I wonder though, if maybe we should have a
giant switch that we can flip to toggle between operating in top-left or
bottom-left origin mode.  Then we can just predicate all the changes in this
patch based on which way that is set.  I guess it all depends on how committed
we are to making this way work, and if we're convinced it can.  So far I haven't
been able to spot a significant flaw in this approach, but that doesn't mean
there isn't one :-)

Definitely agree with Nicolas that we need a clarification of the faceIndex()
for cube textures
 - we have: 
   - TextureCubeMap::getCubeMapSurface() which does a face swap directly
   - faceIndex() which translates from a <target> to a 0-based array index
(non-swapped) for mImageArray
   - TextureCubeMap::getRenderTarget  uses faceIndex() to
d3d->GetCubeMapSurface. This is likely wrong. 
   - We probably should have a d3dFaceIndex function which does the swap and is
used anytime we want to access the d3d cubemap surface faces.
   - we also have TextureCubeMap::convertToRenderTarget which seems to be
confused as well in it's uses of GetCubeMapSurface.

Other than the whole pbuffer/texture sharing question which we should probably
have an offline discussion about, the rest of my concerns are noted inline.

Daniel

http://codereview.appspot.com/3265041/diff/23001/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/compiler/OutputHLSL.cpp#...
src/compiler/OutputHLSL.cpp:198: 
It would be worth including a comment here explaining the reasoning for the
twiddling of the y-texcoord.  Might also be helpful to include the derivations
as well, particularly for some of the more exotic cases such as 2DProj and Cube
cases.

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp#oldco...
src/libEGL/Surface.cpp:155: result =
device->CreateTexture(presentParameters.BackBufferWidth,
presentParameters.BackBufferHeight, 1, D3DUSAGE_RENDERTARGET,
This part isn't going to play so nicely with the pbuffer implementation & d3d
texture sharing extensions that are proposed in Issue=91, as I don't think it's
possible to get a d3d share handle from a swapchain created surface.

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

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:2205: source + j * lock.Pitch,
I think you missed replacing this lock.Pitch with inputPitch

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:1410: "    gl_FragCoord.y = (input.gl_FragCoord.y *
rhw) * dx_Viewport.y + dx_Viewport.w;\n";
don't we need to adjust this case as well?

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:966: loadCompressedImageData(0, 0, width, height,
-inputPitch, static_cast<const char*>(pixels) + inputSize - inputPitch,
locked.Pitch, locked.pBits);
(xoffset, yoffset) need to be specified here instead of (0, 0).

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:1422: copyNonRenderable(&mImageArray[level],
getFormat(), xoffset, yoffset, x, y, width, height, renderTarget);
do we need to update copyNonRenderable as well?

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2238: IDirect3DSurface9
*TextureCubeMap::getCubeMapSurface(unsigned int faceIdentifier, unsigned int
level)
currently this function can take either a index in the range [0..5], or a GLenum
face target.  After your changes, it can only take the latter.  I believe there
are number of places throughout the code where it is called with an index.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2242: // Map a face identifier or cuve map texture
target to the corresponding
cuve -> cube

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h
File src/libGLESv2/mathutil.h (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h#new...
src/libGLESv2/mathutil.h:42: template<typename T, typename MIN, typename MAX>
Do you really want to allow different types for min/max?  Seems like you'd want
them all to be of type T.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/utilities.cpp
File src/libGLESv2/utilities.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/utilities.cpp#...
src/libGLESv2/utilities.cpp:192: return ComputeCompressedSize(width, 1, format);
On 2010/12/14 23:39:30, apatrick1 wrote:
> I think width can be 1 or 2 here, for example if the compressed texture has a
> complete set of mipmaps.

Yes that seems likely.

[nicolas, 2011-01-04 16:48:50]

The skybox of the Aquarium demo is indeed a cube map, but as far as I know it's
not being rendererd to. It's a static texture. What I'm curious about is whether
this patch also correctly handles rendering to all faces of cube maps. I think
it does, but it's crucial to know for sure. If the coordinate systems are not
compatible, doing things this way will be impossible (the hardware will have a
D3D and a GL way of addressing cube faces but it won't be possible to emulate
the GL way using the D3D coordinate system). A sample application which proves
that it works out fine would be nice.

Although I don't know of any existing OpenGL ES or WebGL samples which render to
cube map faces, it's commonly used to make shiny objects reflect things that
move around in the scene. I'll let you know if I do find such a sample
application...

[apatrick1, 2011-01-07 23:40:22]

I addressed Daniel's comments and uncovered a bug in subImage with additional
testing. This is going to need another big batch of testing...

Gregg put together a demo that renders to cube map faces and the face swapping
worked.

http://webglsamples.googlecode.com/hg/dynamic-cubemap/dynamic-cubemap.html

http://codereview.appspot.com/3265041/diff/23001/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/compiler/OutputHLSL.cpp#...
src/compiler/OutputHLSL.cpp:198: 
On 2010/12/23 20:19:23, dgkoch wrote:
> It would be worth including a comment here explaining the reasoning for the
> twiddling of the y-texcoord.  Might also be helpful to include the derivations
> as well, particularly for some of the more exotic cases such as 2DProj and
Cube
> cases.

Done.

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp#oldco...
src/libEGL/Surface.cpp:155: result =
device->CreateTexture(presentParameters.BackBufferWidth,
presentParameters.BackBufferHeight, 1, D3DUSAGE_RENDERTARGET,
Agreed that there is no way to get a share handle for a swap chain. I can add a
macro to switch between the two implementations but lets wait until we know if
this is really an issue.

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

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:2205: source + j * lock.Pitch,
Agreed.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:1410: "    gl_FragCoord.y = (input.gl_FragCoord.y *
rhw) * dx_Viewport.y + dx_Viewport.w;\n";
On 2010/12/23 20:19:23, dgkoch wrote:
> don't we need to adjust this case as well?

Done.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp
File src/libGLESv2/Texture.cpp (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:1422: copyNonRenderable(&mImageArray[level],
getFormat(), xoffset, yoffset, x, y, width, height, renderTarget);
Yes. I think both the source and dest Y need to be flipped in copyNonRenderable
but neither of the pitches need to be negated since it is a surface to surface
copy.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2051: result =
mTexture->GetCubeMapSurface(static_cast<D3DCUBEMAP_FACES>(f), i, &source);
No need to switch Y faces in source and dest, so long as it is consistent.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2238: IDirect3DSurface9
*TextureCubeMap::getCubeMapSurface(unsigned int faceIdentifier, unsigned int
level)
I added a ConvertCubeFace function to es2dx and tried to consistently use
GLenums to identify faces in this file, converting to a D3D face at the last
moment.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/Texture.cpp#ne...
src/libGLESv2/Texture.cpp:2242: // Map a face identifier or cuve map texture
target to the corresponding
Deleted this code.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h
File src/libGLESv2/mathutil.h (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h#new...
src/libGLESv2/mathutil.h:42: template<typename T, typename MIN, typename MAX>
int min = 0;
LONG max = 1;
clamp(0.5, min, max);

would be an error without different types for T, MIN and MAX. I can go through
and fix all 16 errors if you like :)

[dgkoch, 2011-01-11 20:00:06]

Will look at the new patch next..

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (left):

http://codereview.appspot.com/3265041/diff/23001/src/libEGL/Surface.cpp#oldco...
src/libEGL/Surface.cpp:155: result =
device->CreateTexture(presentParameters.BackBufferWidth,
presentParameters.BackBufferHeight, 1, D3DUSAGE_RENDERTARGET,
Sounds like this shouldn't be necessary.  We'll just have a swapchain for the
window-backed surfaces and a texture (which can be shared if desired) for the
pbuffer-backed surfaces.

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h
File src/libGLESv2/mathutil.h (right):

http://codereview.appspot.com/3265041/diff/23001/src/libGLESv2/mathutil.h#new...
src/libGLESv2/mathutil.h:42: template<typename T, typename MIN, typename MAX>
On 2011/01/07 23:40:22, apatrick1 wrote:
> int min = 0;
> LONG max = 1;
> clamp(0.5, min, max);
> 
> would be an error without different types for T, MIN and MAX. I can go through
> and fix all 16 errors if you like :)

Fair enough.  Can leave it then.

[dgkoch, 2011-01-11 21:38:01]

Haven't been able to spot any serious flaws in this yet.  A couple minor issues
noted below.

How is the testing going?

http://codereview.appspot.com/3265041/diff/56001/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

http://codereview.appspot.com/3265041/diff/56001/src/compiler/OutputHLSL.cpp#...
src/compiler/OutputHLSL.cpp:209: // For texture the mapping is Y -> -Y, which
maps [-1, 1] -> [1, -1]. This is not sufficient on its own for the
"texture" -> "cube textures"

http://codereview.appspot.com/3265041/diff/56001/src/libEGL/Surface.cpp
File src/libEGL/Surface.cpp (right):

http://codereview.appspot.com/3265041/diff/56001/src/libEGL/Surface.cpp#newco...
src/libEGL/Surface.cpp:281: IDirect3DSurface9 *textureSurface = NULL;
this variable is now unused.

http://codereview.appspot.com/3265041/diff/56001/src/libGLESv2/utilities.cpp
File src/libGLESv2/utilities.cpp (right):

http://codereview.appspot.com/3265041/diff/56001/src/libGLESv2/utilities.cpp#...
src/libGLESv2/utilities.cpp:524: // Map a face identifier or cuve map texture
target to the corresponding
- cuve -> cube
- should we remove the bit about "face identifier" since this only maps enums?

[apatrick1, 2011-01-11 23:01:22]

All done.

I wanted to see if you saw any major additional work that might need to be done.
It's a time consuming thing to test unfortunately. I'll try and do another round
this week. I wish we had some automated tests.

[nicolas, 2011-01-12 19:08:47]

Hi Al,

Thanks for those cube map face index translation changes. It looks a lot more
consistent now.

Also a big thanks to Gregg for the dynamic cube map demo. It's the best kind of
prove that the way you implemented the coordinate system transformation works
excellent!

One thing though: I'm seeing blue lines appear in the sphere's reflection, at
the edges of the cube map faces. It's probably just a minor off-by-1 bug that
has crept in recently.

Thanks!

Nicolas

[apatrick1, 2011-01-15 00:45:26]

I forgot to flip the half pixel offset. That was what was causing the blue
fringe in the cube render target, though it actually applied to all render
targets, just not as obviously.

I did testing the same as before. I couldn't find anything wrong but I'm sure I
missed something. It might be time to let the canary channel test it; it has
more bandwidth.

[nicolas, 2011-01-19 16:21:59]

Looks great to me. I'm confident that anything we might have overlooked can be
fixed easily later.

[apatrick1, 2011-01-19 18:39:40]

Daniel, okay to commit?

[dgkoch, 2011-01-19 18:49:49]

Can you edit the description before you commit?  I've noticed that these get
added to the commit log and the one here is very out of date with respect to the
current functionality.  If not, maybe should just create a new review ticket?

Other than that LGTM.  Let's get this into wider testing!

[apatrick1, 2011-01-19 23:16:23]

This is in top of tree chrome now at r71834. It'll be in the canary channel
soon.