Feedback please.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 03.05.2011 um 16:51 schrieb Adam Martinson:
+#ifndef WINE_NO_TRACE_MSGS
for (; i < query->n_ids; ++i)TRACE("Allocated timestamp query %u in context %p.\n", query->ids[i], context);+#endif
use if(TRACE_ON(d3d)) instead. It'll be statically FALSE if there are no trace messages, and you also avoid the loop at runtime when traces aren't on.
+struct _wined3d_timestamp_query +{
- struct list entry;
- struct wined3d_context *context;
- int n_ids;
- GLuint ids[1];
+};
I'm not sure if this is necessary, I think we can just give struct wined3d_timestamp_query a ids[3] field.
+typedef struct D3D10_QUERY_DATA_TIMESTAMP_DISJOINT {
- UINT64 Frequency;
- BOOL Disjoint;
+} D3D10_QUERY_DATA_TIMESTAMP_DISJOINT;
Wrong code style, it looks like you copypasted .idl code into a .c file. Please define this in wined3d.idl using WINED3D_QUERY_... names. The wined3d.idl file will soon be gone and replaced with a plain .h header.
If you don't need those types for implementing the d3d9 queries ignore the d3d10 stuff for now.
static BOOL warned = FALSE;if (!warned){WARN("GL_ARB_timer_query not supported, returning WINED3DERR_NOTAVAILABLE\n");warned = TRUE;}
I don't think we have to limit WARNs to one output only. WARNs aren't shown by default. This kind of limiting is used for FIXMEs only to make sure repeated FIXMEs don't spam the terminal and hide other output.
I have to go now, I'll be back later. If I have time and am not tired I'll read this a bit more and check the tests. But probably Henri has sent some comments by then too.
On 3 May 2011 16:51, Adam Martinson amartinson@codeweavers.com wrote:
Feedback please.
This is in addition to what Stefan already said:
+/* GL_ARB_timer_query */ +#ifndef GL_ARB_timer_query +#define GL_ARB_timer_query 1 +#define GL_TIME_ELAPSED_ARB 0x88BF +#define GL_TIMESTAMP_ARB 0x8E28 +#endif
+typedef void (WINE_GLAPI *PFNGLQUERYCOUNTERPROC) (GLuint id, GLenum target); +typedef void (WINE_GLAPI *PFNGLGETQUERYOBJECTI64VPROC) (GLuint id, GLenum pname, GLint64 *params); +typedef void (WINE_GLAPI *PFNGLGETQUERYOBJECTUI64VPROC) (GLuint id, GLenum pname, GLuint64 *params);
Where did you get these? The style reminds me of glext.h.
@@ -95,26 +95,31 @@ static HRESULT WINAPI IDirect3DQuery9Impl_GetDevice(IDirect3DQuery9 *iface, static D3DQUERYTYPE WINAPI IDirect3DQuery9Impl_GetType(IDirect3DQuery9 *iface) { IDirect3DQuery9Impl *This = impl_from_IDirect3DQuery9(iface);
- HRESULT hr;
D3DQUERYTYPE ret;
TRACE("iface %p.\n", iface);
wined3d_mutex_lock();
- hr = wined3d_query_get_type(This->wineD3DQuery);
- ret = wined3d_query_get_type(This->wineD3DQuery); wined3d_mutex_unlock();
- return hr;
- return ret;
}
This makes sense, but should clearly be a separate patch. If you're changing the name anyway, "type" would probably be a more reasonable name than "ret".
+struct _wined3d_timestamp_query +{
- struct list entry;
- struct wined3d_context *context;
- int n_ids;
- GLuint ids[1];
+};
...
query->extendedData = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct _wined3d_timestamp_query));
...
+struct wined3d_timestamp_query +{
- struct list entry;
- struct wined3d_context *context;
- int n_ids;
- GLuint ids[];
+};
That's ugly and fragile.
Please try to write coherent patches. Patch 4/7 in particular looks like it was just copy-pasted together. Patch 5/7 could have easily been at least 4 separate patches.
OK, I think I addressed those issues, let me know how these look.
On 4 May 2011 20:38, Adam Martinson amartinson@codeweavers.com wrote:
OK, I think I addressed those issues, let me know how these look.
+#define GL_TIME_ELAPSED_ARB 0x88BF +#define GL_TIMESTAMP_ARB 0x8E28
Most of wined3d uses lowercase hex literals.
- type = wined3d_query_get_type(This->wineD3DQuery);
- if (type == D3DQUERYTYPE_TIMESTAMPDISJOINT)
That's not a wined3d type. Also, this patch doesn't make sense on its own.
+static void test_timestamp_queries(IDirect3D9 *pD3d, HWND hwnd)
Please don't use names like pD3d, pDevice, etc. There's also still a fair amount of inconsistencies in this patch.
Patch 7/12 just adds dead code. It also seems to me the way of splitting the d3d9 and d3d10 disjoint queries is more complicated than needed. The difference can be handled completely in d3d10, since "ARB_TIMER_QUERY_FREQ" is just a constant that could easily just be part of the wined3d interface.
On 05/09/2011 10:03 AM, Henri Verbeet wrote:
+static void test_timestamp_queries(IDirect3D9 *pD3d, HWND hwnd)
Please don't use names like pD3d, pDevice, etc. There's also still a fair amount of inconsistencies in this patch.
Most of the places I did that I did so to match the naming conventions in the rest of the file, I think I've got that cleaned up now. As for inconsistencies, I need something more specific...
Patch 7/12 just adds dead code. It also seems to me the way of splitting the d3d9 and d3d10 disjoint queries is more complicated than needed. The difference can be handled completely in d3d10, since "ARB_TIMER_QUERY_FREQ" is just a constant that could easily just be part of the wined3d interface.
Ok, I've reworked things a bit. I removed the stuff for handling the d3d10 stuff, but I think this way it should be easy to handle inside d3d10 when we get there. Let me know what you think.
-
Adam Martinson -
Henri Verbeet -
Stefan Dösinger