Re: [PATCH 01/12] d3d10: Fix a HeapFree() in d3d10_effect_Release().
On 10/28/2009 06:32 AM, Stefan Dösinger wrote: Am 27.10.2009 um 20:10 schrieb Rico Schüller: Hi, this patch series implements a couple of ID3D10EffectVariable::As*() functions (patches 2-11). Do we have any tests for the As* behavior? Most conversions don't make any sense, so if the app wants a constant buffer as a render target view returning a null obj is probably a sane thing. But ::AsString potentially makes sense for all objects, maybe in a similar way Object::ToString is used to obtain a String for debug output in Java. This is a rather odd looking API - it actually looks a lot more like C# as operator than Java.ToString(). That's basically a safe cast, which returns NULL if the object is not of the type you're trying to cast. ...except that in C# it's a language operator that works on any pointer type. Ivan
Re: [5/6] WineD3D: Write the vshader footer in a separate function
Re: [5/6] WineD3D: Write the vshader footer in a separate function I like this patch - would be great to have more of these: Lines that fit on the screen in my gnome-terminal: 50 Average size of six large ARB shader functions, before this patch: 205 (generate pshader, vshader, handle_ins, arb_declarations, init_output_register, gen_arbfp_ffp...) A couple of other shader questions/suggestions, not related to the patch: - The handle_instruction code seems to be moving away from the design to handle each instruction separately into its own handler - is this really necessary? Does it make sense to create a loop intermediate representation in baseshader if the ARB backend needs it to better handle loops? - There are some elaborate bitfields to track which instructions are called in baseshader.c. Does it make sense to have a more formal framework, where all instructions are tracked or maybe each backend requests which instructions it is interested to track? Ivan
Re: [3/5] wined3d: Only apply shader constants that changed.
Henri Verbeet wrote: This looks like data structure design mixed with GL and shader code. I think it would be better if the data structure was in its own file, without any GL or D3D dependencies, and wined3d and other components could use it. - Ivan
Re: [4/5] wined3d: Use a bitmask to store which bool and int constants are set.
Re: [4/5] wined3d: Use a bitmask to store which bool and int constantsare set. Why is this a good thing - this just artificially caps your constants at 32. If I remember correctly I had to change shaders to do the exact opposite change, because newer versions introduced more than 32 constants. Ivan
Re: [4/5] wined3d: Merge pshader_hw_map2gl() and vshader_hw_map2gl()
This isn't the prettiest way to merge those functions, but it's a start Will you be cleaning this up further to get rid of the huge if-else ? I think at least all the MOV-specific stuff should go in its own instr. function. Ivan
Re: d3dx9: Implementation of D3DXGetVertexShaderProfile 2/2 try 4
Luis Busquets wrote:
+if ((caps.VS20Caps.NumTemps=13)
+(caps.VS20Caps.DynamicFlowControlDepth=24)
+(caps.VS20Caps.CapsD3DPS20CAPS_PREDICATION))
+{
+return vs_2_a;
+}
Please re-check - the second condition is an assignment and always true.
- Ivan
Re: general question..
James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan
Re: general question..
Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan
Re: [1/10] WineD3D: Add extension information to the states
Stefan Dösinger wrote:
A function pointer is a nice idea actually. I don't think we'll ever need
more than one extension per state line though,
You already have this case:
} else if(GL_SUPPORT(NV_REGISTER_COMBINERS)
GL_SUPPORT(NV_TEXTURE_SHADER2)) {
return nvts_fragment_pipeline;
IMO the current way is easier to read,
I don't know about that - a pointer called is_something_supported
seems just as readable to me.
and until we need a more flexible
solution I'd pledge to keep the current code, since no matter what we(I)
would have to rewrite it. It is a pain to rewrite/modify a set of patches so
I don't want to do it unless we're certain we'll need it
I don't feel strongly about it either way, but the argument that it
will never happen doesn't seem very convincing. It will compile
several ms slower is probably a better case to make, but at Init3D-time
we probably don't care how fast it is.
Ivan
Re: [1/10] WineD3D: Add extension information to the states
Stefan Dösinger wrote: This doesn't look bad on the surface, but I have two concerns: 1. Does not scale to more complex support checks. I suspect you'll have to have to rewrite this as soon as a case comes up that requires more than a single support check to establish support. A function pointer may help here. It would also be interesting to look at similarities and differences with the ffp pipeline replacement - which is essentially the same thing on a much larger scale, where multiple states are replaced rather than a single state. Can these support checks handle the case where an extension would control replacing a group of states ? 2. Not necessarily relating to this patch, but in general I would be wary of trying to replace every if check with a pointer. Splitting on extension support is probably ok. Ivan
Re: [WineHQ][WWN] WWN 349
Zachary Goldberg wrote: ... but did not spell check :) Granted, the movie quote is pretty funny, even if isn't spelled right. Ivan
Re: Crash in test d3d9:stateblock with warn+heap is a wine bug
Michael Karcher wrote: Does anyone knows whether the test has this been written buggy on purpose (some app does this), or it is by accident? Probably the first one - I wrote the test with rather limited understanding of how rendertargets work. I don't think there was a specific application need driving this test. The intention was to test how the stateblock behaves under different event sequences - for example if the rendertarget is changed. I see the test is doing its job uncovering interesting behavior :) Ivan
Re: Time to revert bad commits?
Stefan Dösinger wrote: Am Montag, 2. Juni 2008 08:11:13 schrieb Roderick Colenbrander: The bugs I'm talking about: Bug 10580 Sorry that I did not have time yet to look into that, but instead of ranting you could also look at the GLSL docs and make a patch. I did see a precision patch by Vitaliy fixing this problem, which was not applied, presumably since there is a better way to fix this. Renaming the local constants from LC to LVC for vertex and LPC for pixel shaders, and then loading the uniforms after program linking should fix the issue for GLSL. In ARB, just read them from program local environment and load the constants after compiling the shader. The patch being reverted seems interesting - it basically undoes a prior effort to get local constants into the same namespace as global ones due to relative addressing. I assume this is done for performance reasons, is this correct ? I see your patch is special-case disabled in the case of relative addressing. Do we really want to support multiple constant loading code paths for performance savings ? Ivan
Re: Crash in test d3d9:stateblock with warn+heap is a wine bug
Ivan Gyurdiev wrote: Michael Karcher wrote: Does anyone knows whether the test has this been written buggy on purpose (some app does this), or it is by accident? Probably the first one - I wrote the test with rather limited understanding of how rendertargets work. I don't think there was a specific application need driving this test. The intention was to test how the stateblock behaves under different event sequences - for example if the rendertarget is changed. I see the test is doing its job uncovering interesting behavior :) s/ first one/second one/ I guess the reason why switching the render target was tested in particular was some weird code manipulating the stateblock when Wine was switching rendertargets, which I was interested in testing. The last event sequence should probably be dropped if it's invalid, but I would keep the rest, since the test has uncovered a number of bugs in the past. Ivan
Re: How to phrase Wine's success rate
Of the apps we have tested with Wine, about 20% work well, and about another 20% work but have some glitches. Just my opinion, but that's a terrible idea to put in the 1.0 announcement. The impression I get after reading this is (1) wine is full of glitches, and (2) it's being released with 80% of the apps not working well. If you're trying to appeal to new users and/or sponsors you should put more focus on the positives. I don't see Fedora saying Release features include X, Y and Z, but X has a bunch of bugs (X might be full of bugs, but you don't point this out in your release announcement ). Ivan
Re: How to phrase Wine's success rate
How about this for more positive yet realistic phrasing: Of the apps we have tested with Wine, about half work well enough to try, and of those, half work well enough to use routinely. In particular, Photoshop CS2, World of Warcraft, Firefox 2, and many small apps run very well. .NET based apps and Photoshop CS3 are not yet supported. That just sounds more confusing... Is percent of apps we tested a formal release criterion for Wine 1.0 ? If not, the stats seem irrelevant as far as this release is concerned. If yes, why release with so many apps not working well ? If you want to set realistic expectations - list major supported apps along with a link to appdb called Will my app work under WINE?. Then the users will have a realistic expectation of whether their individual apps work. Setting realistic expectations with a rather low percentage number just means a lot of people won't even bother trying. Ivan
Re: WineD3D: WineD3D: Use the shader backend to enable / disable atifs and nvts
What are your opinions? I can't comment on the specifics of the state table design - would have to leave that to Henri. However, anything that increases modularity is a good thing. I'm in favor of the overall direction to break up the pipeline into component stages. Smaller components are more easy to manage - specifically in your case this should allow separate backends to be chosen per pipeline stage. It seems reasonable that these changes would be deferred until after 1.0. Ivan
Re: WineD3D: WineD3D: Use the shader backend to enable / disable atifs and nvts
Stefan Dösinger wrote: Alexandre didn't commit the patch, I think we should come to an agreement on this issue, otherwise it is going to come up again and again. The fundamental issue is pretty straightforward - not sure why it's so difficult to come to an agreement. - You want to mix and match vertex and fragment GL backends - The only maintainable way to do that is to define an interface between vertex and fragment objects - You're concerned about the interface constraining the ability of fragment to talk to vertex - Write a smarter interface, I also suggested an object to manage it (linker). You can't have it both ways - ability to mix and match backends, and unconstrained interface. The interface doesn't have to be the lowest common denominator - it could be the highest common denominator if properly written. As Henri pointed out the fact that both GL and the hardware are vertex/fragment aligned naturally suggests we should break up things the same way. Even the D3D programmable pipeline is broken up this way (there are Pixel and Vertex shader objects) - and the fixed pipeline is going away, so if anything we should move away from its interface. - You want to replace the fixed pipeline using GL shaders. - Then replace the fixed pipeline - this has nothing to do with the programmable pipeline, therefore should not affect any programmable pipeline interfaces. You've pointed out that shader_backend_t is more aligned to a GL extension than it is to a D3D shader. If that's the case I see little value to having that interface - it will just cause confusion as unrelated functionalities are packed into it. The disparity between GL extensions will get greater as we try to add new features using a particular extension, and we'll see a lot of functions that are just empty forwards to other shader backends, since they don't support something themselves. Ivan
Re: tools: Add Spanish Translation to wine.desktop
Name=Wine Windows Emulator I like the expanded version better: Wine is not an emulator windows emulator. Should help clear up any questions people might have :)
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Shader Model 3.0 and earlier versions have different ways to pass varyings from vertex to pixel shader. In the end this means that the pixel shader is dependent on the vertex shader. If the vertex shader is a GLSL shader, it can read either the builtin varyings or the declared ones, the GLSL vertex shader writes both and the driver (should) sort it out. If the vertex shader is not a GLSL shader, we can only write to builtin varyings, so a GLSL pixel shader has to use different code. Currently the GLSL linking code sorts it out, but we have dependencies between vertex and pixel shaders. In the end I think at least the linker object has to know way too many things about the shaders, and the shaders have to know things about each other. You mean the code in generate_param_reoder_function ? That's exactly the kind of thing that should go in a linker object - it takes as input a vertex and pixel shader, and handles how they talk to each other. The semantics map is the interface between them that describes varyings - of course the interface has to be available to the linker to read the vertex outputs and make decisions as to the pixel inputs . Your linker could be backend-independent and try to support odd combinations like software vs / glsl ps or something like that, or it could be glsl-specific, and basically know how to link one glsl vertex shader to one glsl pixel shader (you could use a different linker for other things). Why is the vertex/pixel linkage related to how you generated the vertex (or pixel) shader ? Does it matter if your shader was created from the fixed or programmable interface on the d3d-side in order to link it to the other one? How will geometry shaders fit into the picture ? Ivan
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Henri once suggested making the shader_backend_t structure a COM object, that is created and destroyed by the device. That would make the state of the data it manages clearer(belongs to the shader object), though it would add some stuff we don't need, like the IUnknown parts, refcounting. If we did that we could call the shader function routing inheritance and are well within OOP slang again. Yes, what you are doing is exactly inheritance - whether it's done through COM or not doesn't really matter. However, since there are 3 base things to inherit from (arb, glsl, none), you're inheriting each of them and now you have 6 backends. It seems that the base functionality is fairly independent of the derived functionality, so that's why I suggested composition over inheritance here. I understand you're concerned with linking vertex and fragment together - but that should probably go in a separate object. My concern is that if we break up the shader structure into multiple objects(e.g. vertex shader handler, pixel shader handler, fixed function vertex replacement, fixed function pipeline replacement, depth blit handler, pipeline linker), then we'll get simple single objects, but the putting these parts together adds more complexity than we save in the first place. Often in OOP programs(and other paradigms as well) you don't know what one component does without knowing the whole program. Sure, there are always tradeoffs - it's your call, I'm just offering a semi-informed opinion :) I'm sure any approach will be successful in the end, but it will be interesting to see how code that's written now scales to GL3, D3D10, geometry shaders, and upcoming extensions. In that respect, it seems like a good idea to try an ATIfs backend now as a test of the underlying framework. Ivan
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Stefan Dösinger wrote: Am Mittwoch, 19. März 2008 07:46:07 schrieb Ivan Gyurdiev: Why do you need to reroute the shader path through atifs to support an unrelated set of functionality (ffp replacement)? Isn't it possible to have an ffp_backend, and a shader_backend (shader being the d3d shader), and you can implement both differently, with different APIs? Sounds all great and cool, but: How do you handle a case of a d3d vertex shader + fixed function fragment processing using a GLSL shader replacement? An Uber-Shader-Backend that links the shaders together? I can happily drop the strange routing through GLSL and the none shader backend for ATIFS, or with ARB pixel shaders enabled. In that case we'll just don't make use of it on ATI 9500+ cards until we have an ARB or GLSL replacement. I'll get back to you on that later tonight, need to think about this some more - way late for work right now... (thanks to you!) However, yes, I think there needs to be distinction between a standalone shader concept, and a pipeline concept, which is concerned with linking several multifunctional shaders together - your uber-shader-backend. Lack of distinction on this point is causing all this confusion. Ivan
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Stefan Dösinger wrote: I'll get back to you on that later tonight, need to think about this some more - way late for work right now... (thanks to you!) However, yes, I think there needs to be distinction between a standalone shader concept, and a pipeline concept, which is concerned with linking several multifunctional shaders together - your uber-shader-backend. Lack of distinction on this point is causing all this confusion. Cool, I'm looking forward to suggestions. It looks to me as if shader_backend is being overloaded for many different purposes that are not really related to each other. Typical object structure is to group related data an functions in one place, but what's happening in shader_backend is that it has no data of its own, and it's a vtable routing between different GL extensions with the data being scattered across multiple different places. - some functions are related to management of a single shader and its resources [ state is BaseShader ] - other code manages the link between vertex and fragment shader [ glsl programs stored in the Device ] - other code manages a group of 2 shaders to handle some fixed function purpose [ _blt, using Device-shader_priv_data ] - now you want to replace the main fixed function fragment processing [ state is in the state table ] I think it would be worthwhile to review all of this, and see if this organization makes sense. Why aren't the functions grouped together with the data ? Why is some of the data in the device object ? Why are functions managing different data containers in the same vtable ? I am no longer familiar with the details, but there's way too many things called shader by now - d3d shader, gl shader, this made up shader_backend that actually does fixed function stuff and represents neither. Maybe it makes sense to create new object types - like a 'pipeline', containing a 'vertex processor', 'fragment processor' (not necessarily implemented via shaders). Maybe each of these should have a fixed and dynamic facing d3d API, but attach or detach to the pipeline in the same way. Maybe they can have different gl extensions backend implementing each. Meanwhile, I've separated the ATIFS implementation and the shader backend changes in my patches. The result is attached. The patches named 1, 2, ... will be merged together to avoid regressions due to partial implementations, and they need some reordering. I've hacked that together during my train ride, so I've no idea if it really works. Will take a look...
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Stefan Dösinger wrote: Hi, Given the past discussion, do you agree with the code now? Alexandre wants your OKs before applying the patches. I am not familiar with the state table, atifs, or recent developments in the d3d codebase. That's why I suggested a diagram, so that everyone can understand the discussion. My concern is long-term maintainability of the shader API. I think the problem is with the definition of shader backend. The original intent, and the way it's currently used is: A backend for the d3d shader pipeline (d3d shader backend), which happens to be implemented using some kind of gl shader. The way this patchset is heading is: A (gl shader backend), which implements both d3d shader and ffp pipeline, depending on the circumstances, through a mixed api I don't mind sharing the GL shader code to implement FFP or D3D shader pipelines - whether it's ATIfs, ARBfp/ARBvp, GLSL, or some other thing providing the implementation. What I do mind is sharing that code through the same API for both the FFP and D3D pipeline codepaths. This leads to combining APIs that don't belong together, and odd multiplexing like you have in patch 004 - forcing the shader path through atifs, even though atifs currently doesn't support handling shaders properly, and has to borrow code from other backends, and implement routing to the right one based on what flags are set. Why do you need to reroute the shader path through atifs to support an unrelated set of functionality (ffp replacement)? Isn't it possible to have an ffp_backend, and a shader_backend (shader being the d3d shader), and you can implement both differently, with different APIs? Ivan
Re: d3dx9_36: Implements D3DXGetShaderVersion
Luis C. Busquets Pérez wrote: This functions gets the Version Token of a binary shader. This Token is the first of the binary stream so it is as easy as picling the pointed DWORD. This is all public information available to the public. What happens when the shader is NULL? Yours will crash - MSDN says otherwise. Does this work on Pixel and Vertex shaders ? Does it always return back the first dword as is - what happens if an invalid shader version is passed in. To implement a test would be far more complicated as it would mean to include a binary shader. Anyway, I have tested the results with Civ4 (by making a derivation on d3dx9_32 with this function and using the original d3dx9_36.D3DXGetShaderVersion function) and the results where the same for all the shaders that where present. I would suggest an automated test across [ vs: 1.0, 1.1, 2.0, 2.1, 3.0 ] , [ ps: 1.0, 1.1, 1.2, 1.3, 1.4, 2.0, 2.1, 3.0 ] , NULL, and an invalid token. You can see some binary shaders in the d3d9 tests. Ivan
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
I don't quite understand why it's necessary to write ARB, GLSL, and NONE shader descriptors inside the ati_shader file. How will this infrastructure scale to a new shader backend added a year from now ? For such a large patchset maybe there should be a high level design diagram that explains how it works ? I very much like that backend-specific functionality ends up in the appropriate backend - as per patches 10, 11, and 14. However, it seems those patches stand on their own, independent of the rest of the patchset and this feature. Ivan
Re: [4/22] WineD3D: A shader backend descriptor for the ati fragment shader backend
Stefan Dösinger wrote: Am Sonntag, 16. März 2008 15:00:49 schrieb Ivan Gyurdiev: I don't quite understand why it's necessary to write ARB, GLSL, and NONE shader descriptors inside the ati_shader file. How will this infrastructure scale to a new shader backend added a year from now ? Currently I am doing that so ATI cards which have ARB_fp or GLSL can use the fragment replacement code and d3d pshaders at the same time. To me this suggests that there should be two separate shader backends, selected differently for fixed pipeline replacement purposes and for shaders. I don't like the argument that this is only an intermediate step - if no one writes the glsl/arb replacement code, it becomes permanent. Ivan
Re: Wine Security Disclosure
No matter what if your running a program, be it linux or windows (on wine) it always has the potential to mess up things that the user running it has access too. Not true - you should be running in a SELinux sandbox to solve this problem. I'm not sure if the technology is mature enough for this, but it can and will constrain beyond user privs. So if you run it as a normal user the worst it can do is mess up that users files All of your important system files are owned by root (or they should be...) Quite the opposite in fact - anything important's already in the user's files - saved passwords, crypto keys, personal data, financial files. The root stuff is not interesting at all, except as a means to get to the user's files. - Ivan
Re: Bugs fixed in 0.9.56
Dan Kegel wrote: Here are the bugs marked fixed during the 0.9.56 cycle. (A few of them were fixed earlier, but not marked as such before.) It might be nice if the Lots of bugfixes line in each release ANNOUNCE file would link to a list like this one. Proof there were lots of bugfixes... Ivan
Re: ENTER_GL and ActivateContext question
A lot of code in WineD3D avoids the getters and setters, and just accesses the implementation structure. While any object oriented programmer screams when doing that, we need it for performance reasons. Currently we spend 10 to 20 per cent of the CPU time spent in WineD3D just in surface::GetContainer and its callees, if we'd use Getters and Setters everywhere then goodbye performance(I want to replace GetContainer somewhen). So inline it... imho the performance argument is a bad reason to abandon maintainability. Your codebase is constantly getting more complex, and now you're having threading issues, which will only get more important with multi-core and other such things becoming standard. Meanwhile graphics cards are getting a lot more powerful - I solved the performance problem 2 weeks ago w/ a GeForce 8800 GT. As for the object oriented programmer - I would hope every programmer thinks in terms of objects, even when writing C code. The GLSL backend wouldn't be available today if the shader compiler was still a big series of if statements. Ivan
Re: d3dx8 [patch 7/7] Implement D3DXMatrixTransformation
Any chance the formatting can be improved to include whitespace between blocks and proper indentation?
Re: wined3d: Enable clip planes with GLSL shader backend
+if (use_vs(stateblock-wineD3DDevice) stateblock-wineD3DDevice-vs_selected_mode == SHADER_ARB) GLSL specific logic should go in glsl_shader.c. Please try to get away from writing code in terms of flags and if statements - use the OOP model. Backend-specific logic should be removed from pixelshader and vertexshader too...
Re: [5/5] WineD3D: Add sincos support to arb shaders
Stefan Dösinger wrote: Am Donnerstag, 27. September 2007 02:57:01 schrieb Ivan Gyurdiev: Aren't most of these 2.0 and 3.0 instructions ? What's the goal of adding them to ARB - you won't be able to implement full 2.0/3.0 support in ARB. We can implement 2.0(but not 2.a/b). Whenever it's easy to do, I'm adding 3.0-Specific behavior too. The reason is that GLSL is a bloody pain on MacOS, with ARB I can bypass many abstraction layers that have a huge number of software fallback pitfalls. Are you saying you intend to advertise 3.0 support on ARB only? If not, then why add any 3.0 features at all ? Ivan
Re: [5/5] WineD3D: Add sincos support to arb shaders
Aren't most of these 2.0 and 3.0 instructions ? What's the goal of adding them to ARB - you won't be able to implement full 2.0/3.0 support in ARB. I think most of these were left unimplemented on purpose. Ivan
Re: [1/5] WineD3D: Fall back to drawStridedSlow if fog coords are used
Stefan Dösinger wrote: Up to now we've been lucky that all the games requireing manual fog coords haven't used vertex buffers, thus we always went through drawStridedSlow. Make sure that the fog also works with vertexbuffers As for the rejected patches from yesterday, I've moved them back to the bottom of the queue when I fixed them, so I will resend them later. This patch is probably fine, but it makes me look into the surrounding code [ software blending, changing the strided data ], which is still incredibly misleading. That should go into its own function, with the FIXVBO macro removed (since all such macros are evil, without exception), and a big comment should be added that named access to the strided data is only valid in the fixed function pipeline and will not work in the shader pipeline. Maybe it makes sense to use two different structures to make this clearer - not sure.
Re: Direct3D 10 design considerations
H. Verbeet wrote: On 18/08/07, Roderick Colenbrander [EMAIL PROTECTED] wrote: From what I have seen GL3 is very different. It would be like maintaining a GTK and a QT backend in one library. They use very different calls (glBegin/glEnable and so are gone), need different WGL contexts and so on. Sure in case of a wined3d-gl3 you'll end up fixing bugs twice. But for quite a number of bugs you'll need to patch both gl2 and gl3 anyway. That's true, but there's also a rather large part of wined3d that's not GL specific in the first place. The most obvious examples are most of the shader stuff and state management, but that's certainly not all. The shader stuff uses two different backends and an intermediate interface (SHADER_OPCODE_ARG) to isolate the asm parser. Other parts of wined3d could do the same thing - you are interface constrained from the top (d3d) and from the bottom (gl), so it makes sense to focus on intermediate representations of the data. If it's hard to replace your GL backend, then the code isn't properly structured. It's interesting that all this work was done to move towards convergence: D3D1-9 - Wined3d Pixelshaders, Vertexshader - shared backend GLSL, ARB - single shader parser ...and splitting things up is being considered yet again. I'm not sure I like the idea of throwing away working code to start fresh - but I'm no longer writing d3d code, so :)
Re: [5/5] WineD3D: Implement high order patches
Stefan Dösinger wrote: There are other things too which could be moved, but I'm not sure if that would make the code more readable. One big function which does things step by step vs a few smaller functions which disturbs the readflow. All of this code pasted 3 times could probably benefit from some loops over an index. Without knowing anything about the code - typically big if statements are candidates for a sub-function (if (normal))
Re: mentoring
What Stefan says about discussing patches is true--it helps if there's not a lot of debate around a patch. This quote should go down in the history of open source :) I hope the word you're looking for is controversy.
Re: [5/5] WineD3D: Software vertex blending
H. Verbeet wrote:
+for(i = 0; i 16 /* Max vertex attribs */; i++) {
I think you should be using MAX_ATTRIBS there.
Changing the strided data like that in drawPrimitive looks rather
hacky to me.
You cannot access the strided data by both named approach
(u.s.something) and indexed approach.
The two are completely independent strided formats - one is used for the
fixed pipeline, and the other is used for the shader pipeline.
I think both of you are wrong, and the correct number is 15 +
WINED3DDP_MAXTEXCOORD, which is the bigger of the two unioned together.
Only modifying the first 16 will break: normal2, tangent, binormal,
tessFactor, fog, depth, sample in the fixed pipeline case (true, I don't
see these being used anywhere right now ...).
Please leave writing of the strided data to the functions designed for
this purpose.
Ivan
Re: [3/20] WineD3D: Implement IWineD3DDeviceImpl_CreateVertexDeclarationFromFVF
Stefan Dösinger wrote: You're returning a size_t here, which does not match the WINED3DERR_OUTOFVIDEOMEMORY returned on the HeapAlloc (granted, that's probably incorrect behavior by d3d9, but at least the return types should be consistent). Does that mean we'll be removing the other two versions of this code in existence (d3d9, and drawprim conv)? By the way, who wrote the very first version - I was looking through git history to figure out the proper copyright attribution, and it predates the git tree. I guess we should update the copyright in d3d9/vertexdeclaration.c to list that person if it isn't done already. I was just curious... Ivan
Re: [5/5] D3D9: Add a test for the converted vertex decl
Looks good, but this comment is misleading - s/second/first. +/* The contents should correspond to the second conversion */ +VDECL_CHECK(compare_elements(result_decl1, test_elements1)); Also, I thought when Henri was testing this that the object kept changing between sequential Get() calls ? Did you mention something about a driver bug causing this ? Ivan
Re: wined3d: Mark vertex shader 3.0 as foggy shaders if they write out the fog coord
Fabian Bieler wrote: Vertex shaders are marked as 'foggy shaders' in wined3d if they write out the fog coord. Previously this was not done for vertex shaders 3.0. This patch corrects this problem. Please don't do that - the design is flawed enough as it is (GLSL being invoked from vertexshader and such..). The reg_maps was meant to be: - computed in shader pass 1 [ register tracking ] - used in shader pass 2 [ code generation ]. Here you're doing this: - computing reg_maps at the very end of shader pass 2 [ unpack stage ] - using this right afterwards in the calling function to set yet another flag (This-usesFog). I'm not sure what needs to be done instead, since it all looks so broken... - I would say this type of analysis about shader usage needs to go into pass 1 (baseshader), since it is backend independent. It's kind of vertex-shader specific, but we're trying to merge vertex and pixel shaders, and remove code from these two files, not add more of it. - The usesFog looks like it's trying to persist information about register usage after the shader has been compiled for optimization purposes...except that this information is already persisted - if you look in baseshader, you will see the entire reg_maps structure is kept in there, exactly for that purpose [ was done for software shaders actually, but that never happened ]. - Also, other code outside the shaders should not be accessing these flags directly - there should be a function which looks inside the reg_maps, and code from other file should call that function (we are emulating OOP programming in C, so we should try to use encapsulation). Ivan
Re: wined3d: Mark vertex shader 3.0 as foggy shaders if they write out the fog coord
Here is how this was meant to work at some point [ since I see all kinds of incorrect things being done ]: Entry point Code generation == Pixelshader Baseshader --- ARB backend Vertexshader --- --- GLSL backend Code should be moving towards the center from both directions. Vertex and pixelshader: Should contain minimum amount of things Baseshader: Should contain anything backend [ and frontend ] independent. I think it's acceptable to put some things that are frontend-dependent in baseshader, as long as they can be written in a generic way, without tons of (if (pshader) do_x else if (vshader) do_y statements). Pass 0: Tracing: 100% done in baseshader. Pass 1: Register tracking: 100% done in baseshader. Pass 2: Backend independent gen code: In baseshader [ this includes the opcode loop, and all parsing of the shader asm ] Pass 2: Backend dependent gen code: In ARB or GLSL files [ ideally should work with pre-processed shader asm, and broken out tokens - a sort of an intermediate representation if you prefer ]
Re: [2/2] wined3d: Remove usesFog flag from IWineD3DVertexShaderImpl
Stefan Dösinger wrote: Honestly I do not really agree with getter methods like this inside WineD3D. Yes, they do hide the implementation details, namely how the flag is stored. Yes, they do encapsulate data, like the Object Oriented Programming model says. But honestly, how much use is it to do a function call just to read a value inside wined3d? Well, your use of a redundant top-level flag does kind of remove the need for a getter - I only brought it up, since I've moved the reg_maps structure around one too many times, and that's where the first flag is stored. I don't argue that we should make wined3d internals visible to outside wined3d. But inside wined3d, my personal preference is to just access the implementation structure directly. The more complex and interconnected the codebase, the more it makes sense to encapsulate things. I think abstraction also benefits the less experienced developer. You make a good point that this level of detail should be internal to the shader to begin with, regardless of how it's accessed. How much abstraction does such a function give us at the cost of performance? You can always make the function inline (although that's also rather ugly).
Re: WineD3D: Handle WINED3DSPSM_DZ and WINED3DSPSM_DW in texcrd in arb shaders
From b1df18be85d1ca6a9904be05420f975f6933b030 Mon Sep 17 00:00:00 2001 From: Stefan Doesinger [EMAIL PROTECTED] Date: Fri, 23 Mar 2007 19:03:58 +0100 Subject: [PATCH] WineD3D: Handle WINED3DSPSM_DZ and WINED3DSPSM_DW in texcrd in arb shaders Isn't there a function to handle modifiers? get_modifier_line...
Re: WineCfg and DirectX options
Stefan Dösinger wrote: Hi, implementing GLSL checkbox, OffScreenRenderingMode dropdown menu and VideoMemorySize textbox into winecfg would be easy. GLSL is OK IMO, because some drivers(*cough* macos *cough*) have serious problems with glsl. It could be included into the shader dropdown box. The issue that needs to be dealt with is that we can't combine arb vertex shaders and glsl pixel shaders or vice versa. Spec says otherwise... I've also seen a software vertex shader with a GLSL pixel shader. What I have considered is some performance vs correctness slider. We want incorrect graphics in wined3d? With decreased correctness things like render target locking or drawStridedSlow are disabled. If the correctness is decreased even more some properly implemented, but known to be expensive settings could be deactivated, like smooth shading, per pixel fog(ok. not sure if that is expensive). Windows drivers have such a setting, so why shouldn't we :-) . Are you sure it wasn't performance vs quality :)
Re: wined3d: Replace inline static with static inline
Why should inline static be replaced with static inline. Does it improve compatibility with certain compilers? Also, does wine aim to be compliant with ANSI, C99, or somewhere in between (-gnu89)?
Re: Shader instruction table whitespace cleanup
Kevin Wallerbos wrote: This formatting patch cleans up the spacing in the pixel/vertexshader instruction table, making it somewhat nicer on the eyes. The lines have become a bit longer than practical on standard resolutions, but that shouldn't be a problem as most lines were already too long. A better thing to do is to remove the useless GLNAME_REQUIRE_GLSL, and undefined instruction names. In fact, it may be nice to break out the instruction names out of this table completely, since they are only necessary for some instructions, required by the map2gl function. This table is also organized poorly, you have to search through the whole thing, and then do version comparisons to find anything. If possible, it would be great to make it O(1) accessible. = Also, as you point out: Before patch : most lines were already too long [ I'd say some lines were too long ] After patch:all lines are too long Not sure what is gained... please don't force others to your resolution preferences. [ looking at this on a widescreen at 1680 x 1050 and it still wraps! ]
Re: WineD3D: Blit the offscreen texture into the drawable if needed
This time cube texture support is added. Hopefully the coords are right, I had no test app for them. If not it should be easy to spot the very characteristic flipping of the image. I'm confused - you replaced a patch which had no support for cube maps, with one that has untested support for cube maps?
Re: [2/3] wined3d: The texldl instruction takes 3 arguments
Changelog: - The texldl instruction takes 3 arguments Can the SW shader handlers be deleted now that the rest of the SW code is gone. They just get out of sync when you do things like this.
Re: [2/6] winex11.drv: Try to get an opengl visual with aux buffers
Something's wrong with this patch: it has more 2 nested if statements.
I suggest early returns on negated condition, gotos, and subroutines to
fix the problem :)
I don't know if that patch wasn't applied because other patches of my patchset
weren't applied, or because something is wrong with this patch. If something
is wrong with this patch please tell me
From 029e9756abed6c2647950f86e9227ba47d366ebe Mon Sep 17 00:00:00 2001
From: Stefan Doesinger [EMAIL PROTECTED]
Date: Tue, 27 Feb 2007 15:53:43 +0100
Subject: [PATCH] winex11.drv: Try to get an opengl visual with aux buffers
Applications may profit from auxilliary buffers. Because we have to find
the visual at initialization time try to find a visual with aux buffers
first, if none is found fall back to the usual default visual without
aux buffers
---
dlls/winex11.drv/opengl.c | 35 +++
1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c
index fdb92ae..9c1060a 100644
--- a/dlls/winex11.drv/opengl.c
+++ b/dlls/winex11.drv/opengl.c
@@ -3083,33 +3083,44 @@ BOOL X11DRV_SwapBuffers(X11DRV_PDEVICE *physDev)
XVisualInfo *X11DRV_setup_opengl_visual( Display *display )
{
XVisualInfo *visual = NULL;
-/* In order to support OpenGL or D3D, we require a double-buffered visual
and stencil buffer support, */
-int dblBuf[] = {GLX_RGBA,GLX_DEPTH_SIZE, 24, GLX_STENCIL_SIZE, 8,
GLX_ALPHA_SIZE, 8, GLX_DOUBLEBUFFER, None};
+/* In order to support OpenGL or D3D, we require a double-buffered visual
and stencil buffer support,
+ * WineD3D and Some applications can make use of aux buffers
+ */
+int dblBuf[] = {GLX_RGBA,GLX_DEPTH_SIZE, 24, GLX_STENCIL_SIZE, 8,
GLX_ALPHA_SIZE, 8, GLX_AUX_BUFFERS, 1, GLX_DOUBLEBUFFER, None};
if (!has_opengl()) return NULL;
wine_tsx11_lock();
visual = pglXChooseVisual(display, DefaultScreen(display), dblBuf);
wine_tsx11_unlock();
if (visual == NULL) {
-/* fallback to 16 bits depth, no alpha */
-int dblBuf2[] = {GLX_RGBA,GLX_DEPTH_SIZE, 16, GLX_STENCIL_SIZE, 8,
GLX_DOUBLEBUFFER, None};
-WARN(Failed to get a visual with at least 24 bits depth\n);
+/* Try without aux buffers */
+int dblBuf2[] = {GLX_RGBA,GLX_DEPTH_SIZE, 24, GLX_STENCIL_SIZE, 8,
GLX_ALPHA_SIZE, 8, GLX_DOUBLEBUFFER, None};
+WARN(Failed to get a visual with at least one aux buffer\n);
wine_tsx11_lock();
visual = pglXChooseVisual(display, DefaultScreen(display), dblBuf2);
wine_tsx11_unlock();
if (visual == NULL) {
-/* fallback to no stencil */
-int dblBuf2[] = {GLX_RGBA,GLX_DEPTH_SIZE, 16, GLX_DOUBLEBUFFER,
None};
-WARN(Failed to get a visual with at least 8 bits of stencil\n);
+/* fallback to 16 bits depth, no alpha */
+int dblBuf3[] = {GLX_RGBA,GLX_DEPTH_SIZE, 16, GLX_STENCIL_SIZE, 8,
GLX_DOUBLEBUFFER, None};
+WARN(Failed to get a visual with at least 24 bits depth\n);
wine_tsx11_lock();
-visual = pglXChooseVisual(display, DefaultScreen(display),
dblBuf2);
+visual = pglXChooseVisual(display, DefaultScreen(display),
dblBuf3);
wine_tsx11_unlock();
if (visual == NULL) {
-/* This should only happen if we cannot find a match with a
depth size 16 */
-FIXME(Failed to find a suitable visual\n);
-return visual;
+/* fallback to no stencil */
+int dblBuf4[] = {GLX_RGBA,GLX_DEPTH_SIZE, 16,
GLX_DOUBLEBUFFER, None};
+WARN(Failed to get a visual with at least 8 bits of
stencil\n);
+
+wine_tsx11_lock();
+visual = pglXChooseVisual(display, DefaultScreen(display),
dblBuf4);
+wine_tsx11_unlock();
+if (visual == NULL) {
+/* This should only happen if we cannot find a match with
a depth size 16 */
+FIXME(Failed to find a suitable visual\n);
+return visual;
+}
}
}
}
Re: [10/13] D3D9: Fix the circular converted vertex declaration reference
H. Verbeet wrote: On 20/02/07, Stefan Dösinger [EMAIL PROTECTED] wrote: If anyone knows a nicer way to release the converted declaration from SetVertexDeclaration feel free to comment. Not applying this patch won't affect patches 11-13 Wouldn't it be better to directly compare This against convertedDecl from the device in IDirect3DVertexDeclaration9Impl_Release, rather than adding a convertedDecl flag to IDirect3DVertexDeclaration9Impl and setting / unsetting it? I think it makes things harder than it has to be. Also, afaik Ivan has some tests for the exact behaviour. Some tests attached. I cannot guarantee whether the tests make any sense or are correct (don't remember what I was trying to do , but maybe they'll be helpful :) The converted declaration is evil... I lost patience when trying to figure out the exact behavior, so decided to leave it broken, rather than fix it incorrectly. I think I wasn't sure whether the conversion happens on the setFVF call, or on the GetDeclaration call following it. some.tests.tar.bz2 Description: BZip2 compressed data
Re: [4/9] d3d8: Add an IDirect3DVertexDeclaration8 class to hold the wined3d vertex declaration
The idea of this patch and the next few is to create an internal d3d8 object to hold the wined3d declaration and give the d3d8 vertex shader a field to hold the d3d8 vertex declaration object. Setting the d3d8 vertex shader will then set both the wined3d shader and the wined3d vertex declaration. Excellent. I don't know if the implementation works, but the idea is certainly on the right track. This was done incorrectly to begin with, and also made it difficult to remove the dependency on d3d8/d3d9 includes (which I see you're on track to complete). Ivan
Re: D3D9: Implement IDirect3DDevice9::StretchRect
Stefan Dösinger wrote: This call can be handled fully by IWineD3DSurface::Blt. So where did the filter argument go.. there's 3 different filter types to support.
Humus Water demo vs FBOs
Here's a demo that breaks FBOs: http://www.humus.ca/3D/Water.zip Also download: http://www.humus.ca/3D/Framework2.zip You'll notice all the glClear calls fail [ or at least a lot of them ]. That's because the framebuffer is incomplete at that point - checkFBOstatus() should be called before glClear, as well as drawprim to see that this is FRAMEBUFFER_INCOMPLETE_DIMENSIONS_EXT (0x8cd9). I think the call that fails is the clear call in the drawFrame() function of the app - the others set a 0 depth stencil buffer immediately prior, while this one is probably using wined3d's initial depth fbo. I think the Framework sets EnableAutoDepthStencil, so that an fbo is created. It probably ends up being a different size from the color fbo, which breaks a framebuffer completeness requirement.
Re: [15/20] WineD3D: Clean up drawprim a bit
Stefan Dösinger wrote: Am Samstag 06 Januar 2007 22:25 schrieb Ivan Gyurdiev: Stefan Dösinger wrote: Now that almost everything is gone from drawPrimitiveDrawStrided we don't need a subfunction for calling drawStridedSlow/Fast any longer I think while we're cleaning up things, all of software shaders should be removed. There's tons of code in drawprim and *shader.c, which is disabled, doesn't work, and isn't on the right track if you ask me. None of the changes to make the shader parser 3.0 compliant are used in the disabled paths. There's even an Emulation value in winecfg which just confuses the user, since it doesn't work. Perhaps. We will need software shaders for a correct implementation of IWineD3DDevice::ProcessVertices. It supports Vertex shaders, but I don't really think OpenGL feedback mode is what we want here. Maybe we should remove it for now, but keep the code somewhere(in the wiki maybe). If someone is extra-ambitious we can do something like Softwire/SwiftShader does. But I think ProcessVertices is a good oportunity to verify our vertex shader implementation. I think it would be better to start from scratch rather than reuse what's currently there: you can't verify the current vertex shader implementation by adding a completely independent code path. You have to share parts of the processing to do that.
Re: [15/20] WineD3D: Clean up drawprim a bit
Stefan Dösinger wrote: Now that almost everything is gone from drawPrimitiveDrawStrided we don't need a subfunction for calling drawStridedSlow/Fast any longer I think while we're cleaning up things, all of software shaders should be removed. There's tons of code in drawprim and *shader.c, which is disabled, doesn't work, and isn't on the right track if you ask me. None of the changes to make the shader parser 3.0 compliant are used in the disabled paths. There's even an Emulation value in winecfg which just confuses the user, since it doesn't work. I did have a working implementation of software shaders some time ago, which broke things up into functions, and shared the tracing and parser with hardware shaders - but in the end it was too slow, and I abandoned it as the wrong approach too. Unless we generate asm on the fly for the shader or something like that, I don't think software shaders will be made to work soon.
Re: [3/10] WineD3D: Move decoding the vertex declaration to the vertexshader state handler
Regarding the concern of storing the decoded strided data after finishing drawing: This is intentional, the decoded vertex declaration will remain valid after the draw is finished and the arrays loaded. Future draws can use it, if the state is not dirtified again. This sounds like a good idea... Wrt the upward references: When we have multithreading with multiple contexts we will need a per-context tracking of most of the stuff we have in the device now(last_was_rhw, ...). This structure does not exist yet, and I do not see a point in passing the device impl to every function when we can get it from the stateblock too. I still think this caching stuff needs to go into its own structure [ maybe device-cache or something like that ]. I can see how it's a bit different from the standard stateblock data, since it's just a cache, rather than something that's set/get by the application - cache state vs configured state. It's not so clear to me that this is all per-thread data - the strided streams are directly tied to the vertex declaration and stream data, which is in turn shared across threads as part of the stateblock. === Note: you just made drawPrimitiveTraceDataLocations into dead code. (previously called in d3d8 fixed function code path). I don't care much for this function, but please remove it if getting rid of its callers.
Re: appdb rating inflation
Jan Zerebecki wrote: On Tue, Jan 02, 2007 at 07:00:20PM -0800, Dan Kegel wrote: I'd like to change this to make it clear that cracks are a no-no for anything Silver and above, and make Platinum and Gold rather more rigorous: I don't think that is helpful. What is more important: to know that it works only with cosmetic problems after quite some work(arounds) or that it does not work at all without workarounds and to have no information about how it works with workarounds? I think that depends on your target audience. Paying customer - no workarounds, misleading advertising New user, unfamiliar with wine - no workarounds, user will be confused, will not understand Wine expert user - doesn't care about rating, wants to see the HowTo instructions and get the app to work Developer - doesn't care about rating, wants to see how close we are to making app work (what workarounds are left, etc..)
Re: [5/8] WineD3D: Readd the strided data trace
Stefan Dösinger wrote: I removed that call accidentally in one of my former patches, but I think it is still a good idea to trace the final strided sources used by drawprim You removed it from a d3d8 fixed function FVF-only codepath, and are adding it back into a shared codepath. That function doesn't work properly with vertex declarations, it will only work with FVFs, which have different strided layout. FVFs use fixed semantic index [u.s.position], vdecl asks the shader for an index [u.s.input[idx]]. Furthermore, the trace isn't needed for declarations, because they have their own (better) traces.
Re: Resend: d3d9: fix failing tests on windows, when no 3d hardware acceleration is available
Louis. Lenders wrote: Hi, is there anything wrong with this patch? If so, could you please tell me what. Thx */Louis. Lenders [EMAIL PROTECTED]/* wrote: Hi, this patch should fix failing tests in d3d9, like you see for example here: http://test.winehq.org/data/200612231000/ Appearently the tests in stateblock.c didn't need to be modified, as they create the device with flag D3DDEVTYPE_NULLREF; these tests do not fail anywhere. thanks for some advice from stringfellow So why not fallback to REF or NULLREF type of device everywhere else if HAL is unavailable? Do those tests need HAL support in the first place?
Re: [9/16] WineD3D: Move decoding the vertex declaration to the device
+if (stateblock-wineD3DDevice-vs_selected_mode != SHADER_NONE stateblock-vertexShader + ((IWineD3DVertexShaderImpl *)stateblock-vertexShader)-baseShader.function != NULL) +memcpy(stateblock-wineD3DDevice-strided_streams, stateblock-wineD3DDevice-up_strided, sizeof(stateblock-wineD3DDevice-strided_streams)); +primitiveDeclarationConvertToStridedData((IWineD3DDevice *) stateblock-wineD3DDevice, useVertexShaderFunction, +stateblock-wineD3DDevice-strided_streams, stateblock-wineD3DDevice-streamFixedUp); My eyes hurt - can we *please* store the device in a local variable. Also, I am not sure how we got to the point where this many upward references to the device from the stateblock are necessary - there has to be something wrong with that: a stateblock captures the state of a device, so why is it referring back to its parent device and looking up state there? This type of thing was introduced into the shader code as well, and IIRC it was wrong there too. Also, all this transient data from the drawprim codepath moved into the device - can you explain again why that's necessary, does it really need to persist after the drawprim call?
Re: d3d9: Add a render target test
H. Verbeet wrote: On 20/12/06, H. Verbeet [EMAIL PROTECTED] wrote: This time as part of the device.c test. Changelog: - Add a render target test Use the attached patch instead, the previous version doesn't apply. This test really belongs in stateblock.c, along with every other set -- get test if you ask me, that's what the framework there was written for. It might be kind of a pain to get this particular one to work though, since changing the rendertarget is currently considered an event, across which all other set/get tests are ran, but I'm sure it's possible. The question of whether stateblocks track render targets should be answered eventually...
Re: [2/8] wined3d: Select the right shader backend when creating the device
H. Verbeet wrote: Changelog: - Select the right shader backend when creating the device Imho there should be either ps_selected/vs_selected flags, or shader_backend flag, but definitely not both [ does not make sense ]. I'd opt for allowing different backends on pshader vs vshader. Maybe the current drawprim code is implemented to use ARB or GLSL for both, but that's not a good reason to disallow capability by writing future code as if the backend was unified. More importantly, you should be able to mix software shaders, with the other backends. I'm also not sure replacing all if statements with a function table is a good idea - restructuring code is dangerous, better not to do it unless new features are being added, or the code is being rewritten, or it's completely unmaintainable (I think that's a bit hard to argue).
Re: [3/8] wined3d: Create a separate function for sampling a texture
H. Verbeet wrote: There are a couple of instructions that have to sample from a texture, right now most of them duplicate that code, and hardcode the texture type. Changelog: - Create a separate function for sampling a texture You should continue to use add_param or get_register_name. There should be a single point in the code where register names are constructed, adding another one is a bad idea. This function is specifically tasked with naming registers, and taking into account things like relative addressing, so that when samplers support relative addressing in the future, changing the naming scheme would be trivial.
Re: [2/8] wined3d: Select the right shader backend when creating the device
- Software shaders currently simply don't work. If we do get software shaders it'll likely be vertex only. So, we should allow hardware pixel shaders, and software vertex shaders. I could see someone using software vertex shaders if he's got no hardware support for shaders, although it's questionable if such a card would be able to effectively run an application that requires them anyway. Software shaders are chosen if (1) there's no support for hardware, (2) the user elected software, or (3) the application elected software, and (3) is required as part of the DirectX spec. Mesa is possibly a better option there. Either way, you're not going to be using software vertex shaders together with HW pixel shaders, that's just silly Well, I was able to identify at least one bug that way [ improper initialization of vertex shader output register ]. So imo it just adds complexity to the code without giving much of a benefit. You're taking two logically separate concepts, and merging them together for no good reason at all. I'm just not sure what the purpose is - at the very least you'd be able to turn off pixel and vertex separately as a user override. Once geometry shaders come into play, I think they should get their own flag too.
Re: [3/8] wined3d: Create a separate function for sampling a texture
H. Verbeet wrote: On 27/11/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: H. Verbeet wrote: There are a couple of instructions that have to sample from a texture, right now most of them duplicate that code, and hardcode the texture type. Changelog: - Create a separate function for sampling a texture You should continue to use add_param or get_register_name. There should be a single point in the code where register names are constructed, adding another one is a bad idea. This function is specifically tasked with naming registers, and taking into account things like relative addressing, so that when samplers support relative addressing in the future, changing the naming scheme would be trivial. Except that that's not going to work. Some instructions, like texm3x3tex, only give you an index of the sampler to work with. That's because they're shaders v1 instructions, which are being phased out [ not applicable to glsl backend at all ]. Old things should be layered on new infrastructure, so those should be handled by making a fake sampler. I think there's already code in baseshader that does that.
Re: [2/8] wined3d: Select the right shader backend when creating the device
- Mixing ARB and GLSL backends is pretty silly as well. Why? I believe you can e.g. perfectly mix GLSL vertex programs together with multitexturing setups. ARB as in ARB_vertex_program or ARB_fragment_program, I'm not sure what multitexturing has to do with it. You can't, for example, properly use ARB_fragment_program together with a GLSL vertex shader. (You might be able to get away with it due to the way GLSL is implemented on a specific implementation, but you're certainly not supposed to). http://oss.sgi.com/projects/ogl-sample/registry/ARB/vertex_shader.txt Interactions with ARB_vertex_program and ARB_fragment_program Mixing a high level ARB_vertex_shader shader with a low level ARB_fragment_program shader is allowed. However, a high level ARB_vertex_shader shader and a low level ARB_vertex_program shader cannot be active at the same time. http://oss.sgi.com/projects/ogl-sample/registry/ARB/fragment_shader.txt: Interactions with ARB_vertex_program and ARB_fragment_program Mixing a high level ARB_fragment_shader shader with a low level ARB_vertex_program shader is allowed. However, a high level ARB_fragment_shader shader and a low level ARB_fragment_program shader cannot be active at the same time. allowing the mixing of types probably leads to a more complete feature set in the end. Except that there are no actual meaningful combinations to make. Well, at least you can turn the pixel and vertex shaders off individually, rather than turn off shaders completely (using the NONE backend). I think up until a few releases ago pixel shaders were off by default.
Re: [3/5] wined3d: Make the offscreen render mode a registry setting
This flag needs to be per adapter - should be added to device.c like the shader mode.
Re: [2/5] wined3d: Fix depth buffer formats to use actual depth textures
H. Verbeet wrote: Two things to note here: - We use GL_DEPTH_COMPONENT24_ARB as internal format for 16-bit depth textures. This is needed because GL_DEPTH_COMPONENT16_ARB doesn't work with FBOs. - We don't properly support combined depth and stencil buffer formats yet. To support this properly we need to be able to detect if the relevant extensions are available or not. Was GL_ARB_DEPTH_TEXTURE detection added to wine recently? I don't have the latest copy. If not, needs to be added to take advantage of the formats you're adding. I think GL_EXT_PACKED_DEPTH_STENCIL should be used for stencil/depth..
Re: [3/5] wined3d: Make the offscreen render mode a registry setting
Ivan Gyurdiev wrote: This flag needs to be per adapter - should be added to device.c like the shader mode. I meant the device structure. Also, the mode selection should include configuration and support detection - I think you're missing the support detection. Imho there should be a select_offscreen_mode() function like the shader mode is done, selecting between fbo, pbuffer, and none.
Re: [3/5] wined3d: Make the offscreen render mode a registry setting
H. Verbeet wrote: On 17/11/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: This flag needs to be per adapter - should be added to device.c like the shader mode. Well, the registry setting is global. I'm not sure if it really makes sense to use different offscreen rendering modes for different devices. Sure it does - different cards have different capabilities [ but like I mentioned in the other mail, I think you're missing the support bit of configuration + support that should be used to choose offscreen mode ].
Re: [3/5] wined3d: Make the offscreen render mode a registry setting
H. Verbeet wrote: On 17/11/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: H. Verbeet wrote: On 17/11/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: This flag needs to be per adapter - should be added to device.c like the shader mode. Well, the registry setting is global. I'm not sure if it really makes sense to use different offscreen rendering modes for different devices. Sure it does - different cards have different capabilities [ but like I mentioned in the other mail, I think you're missing the support bit of configuration + support that should be used to choose offscreen mode ]. I think you're missing the point of the setting. It's something internal for developers. No, you're adding code all over the place that looks up this setting - this will have to be changed later. It's the exact same thing I did with shader mode, so just trying to shortcut to the end result...
Re: [2/5] wined3d: Fix depth buffer formats to use actual depth textures
That's still missing runtime check for GL_ARB_DEPTH_TEXTURE extension. I don't like developers only and registry key - feature should be complete or not in the tree. The trouble with the current setup is that we can't use different formats based on what extensions are present. Then the setup should be fixed, rather than adding dependent code without extension checks. I might have mentioned before that statically initialized tables are evil Btw, good job, I am glad you're working on this does it make Battlefield 2 work?
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
Mirek wrote:
Ok, i just found why is it not working with this patch, and why is it
working without this patch. It is based on position of definition MOVA
in vertexshader.c, if MOVA is on after patch position it is not
working, but if i move line
{WINED3DSIO_MOVA, mova, NULL, 1, 2, vshader_mova, vshader_hw_map2gl,
shader_glsl_mov, WINED3DVS_VERSION(2,0), -1}, to position of definition
before patch, evrything is working.
...
that's because Jason seems to have introduced order requirements on the
table that weren't previously there. The mnxn implementation will
attempt to access the table as an array, which will break once MOVA is
moved before DP3:
glsl_shader.c:tmpArg.opcode =
IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP4];
glsl_shader.c:tmpArg.opcode =
IWineD3DVertexShaderImpl_shader_ins[WINED3DSIO_DP3];
The instruction table currently does not support this, and it only
happens to work for the first few instructions [ which the MOVA change
breaks ].
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
Tom Wickline wrote: I see the same thing as Mirek, 3dmark03 is totally broken now. And yes I tested GLSL and ARB and before and after with the same results as his. So yes, this is a major regression :D When testing with the GLSL backend, can you please do a +d3d_shader trace, find the MOVA instruction, and send me the entire shader that it's part of (most importantly the shader version). The only way I see that this patch could break the GLSL backend is if the instruction version check is wrong. MSDN claims this instruction is available in shaders 2.0 and higher, but it wouldn't be the first time MSDN is dead wrong.
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
Here is log with 3DMark running for about 3 seconds with patch and without, trace+d3d. http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar.gz There's no mova instruction in that log.
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
Ivan Gyurdiev wrote: Here is log with 3DMark running for about 3 seconds with patch and without, trace+d3d. http://www.czela.net/pub/Czela_Debian_2.5/mirek/wine/regression/3DMark03.tar.gz There's no mova instruction in that log. ...grep for mova, or for unrecognized or Unrecognized or unhandled or Unhandled. I see the app uses a number of vertex shaders 1.1, so maybe mova is a valid 1.1 instruction.
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
The MOVA instruction is a 2.0 instruction, which cannot be fully implemented in ARB. However, the patch shouldn't have caused a regression - if you're seeing a black screen with GLSL also, that means another patch is responsible for this.
Re: Regression in patch wined3d: Implement D3DSIO_MOVA in ARB backend.
Mirek wrote: I tried it three times with same result. Can someone else test if 3DMark 2003 is working with latest wine? You shouldn't test if it's working now - test if it's working before the MOVA patch, and if it's not working after the MOVA patch [ no other patches in between ].
Re: WineD3D State management - going live(TM)
Can you describe what this would look like in detail I haven't thought it through in detail - just making sure you have. I think you're going in the right direction, as long as considering things other than render states in the design - won't bother you anymore, until I see some more of the code :) --- I've tracked down why wine deadlocks on my computer (miscompiled libX11 on Fedora Rawhide), so hopefully I can finish my texture/sampler testcases now, and vtf-related things [ and namespace cleanups ]. Actually VTF depend on what you're doing, and so do FBOs - they both require delayed application of certain states at draw time.
Re: WineD3D State management - going live(TM)
Stefan Dösinger wrote:
Am Montag 23 Oktober 2006 00:57 schrieben Sie:
What does addDirtyState() do when called multiple times with the same
representative?
There is a stateblock-changed.somestate field in the stateblock, which is a
boolean right now. This can be made a pointer to the list element, and set to
the element when the state is first add to the dirty list, and set to NULL
when the state is applied. When addDirtyState finds that a state already has
a list entry it doesn't have to dirtyfy it again
Ok, but that sounds rather messy...
This is still a bunch of code, not a magic instruction how to call gl
directly. To apply a state to gl
States[State]-func(State, stateblock);
How would I mark vertex shader constant #3653 dirty, and apply it using
your mechanism ?
Uhh, shader constants, must have forgotten them :-( . Well, Henri and I
silently agreed to leave them as they are right now.
I don't like how the number of things staying as they are right now is
growing, while the number of things being changed remains confined to
render states. To have a proof-of-concept state management system, it
would be best to take things that are as different as possible, and
manage to get them successfully updated via the new state manager.
Otherwise you won't find out whether the design is flawed or not until
much later.
There is no point in putting them into the main state list, instead
there should be a dirty list per supported sampler.
Again, it seems to me that two different concepts are being mixed
together - I certainly don't want to be typing the function to apply per
sampler, when there should be one entry in a table somewhere, with the
sampler passed as an argument. Dirtification is another topic, which I
don't think you can solve with a list anyway.
No worries, we don't need a seperate function for each sampler :-) The apply
function gets a DWORD state value passed which it can use to find out to
which sampler to apply things and where to read the values from.
Yes, but it sounds like you need an entry for this function into the map
for each sampler...
map { SAMPLER(0) - sampler_apply_function }
map { SAMPLER(1) - sampler_apply_function }
map { SAMPLER(2) ...
...and that seems wrong, but maybe I'm misunderstanding.
Re: WineD3D State management - going live(TM)
I don't like how the number of things staying as they are right now is growing, while the number of things being changed remains confined to render states. To have a proof-of-concept state management system, it would be best to take things that are as different as possible, and manage to get them successfully updated via the new state manager. Otherwise you won't find out whether the design is flawed or not until much later. Agreed, in theory. But I also think that we shouldn't try to put different things in one model by force. If some things work differently, why shouldn't we handle them seperately? Because that creates confusion and disorder - the idea is to move from chaos (gl code in drawprim, shaders, device.c) to order (gl code in one place, following a uniform pattern, with the ability to do locking and/or switch the gl backend). Shader constants are different from fixed function things in two ways: There is a dynamic number of them, That's only a problem, because you keep trying to force everything into a constant array :) and there are no dependencies between 2 shader constants. So there is no use for a table like the fixed function elements, Sure there is... it creates a uniform structure for applying states, bringing some benefits as described above. Dirty-tracking is only one of the goals, and the stateblock already does some of that..
Re: WineD3D State management - going live(TM)
Jaap Stolk wrote: On 10/22/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: and we loose the ability to set up a constant table in the code. The constant table is usually a bad idea, and this demonstrates why - I'm in favor of flexibility, but is it correct to assume that the number of functions that won't work very well with the table is small ? In that case we could put some special number in the table that indicates something like look in the switch statement instead for those functions. This way we keep the flexibility, keep the switch statement small and keep the standard functions optimized. I shouldn't have mentioned a switch statement, that was to illustrate an example. My concern with this particular table is that at the moment it seems too narrow, targeted at render states [ and discussion shows adding sampler states isn't as straightforward as it seems. ]. I imagine a constant map from keys [ APPLY_SAMPLERS, APPLY_ZENABLE, ... ] to function pointers. I'm not quite sure why we're looking into adding pointers per sampler - doesn't really make sense to me. There's three things involved here: (1) how to store the D3D data, (2) how to store the GL data, and (3) how to apply the GL data. Discussion of anything per-sampler has to pertain to (1) or (2), but really has nothing to do with (3)... For storing the D3D data, a different type of structure is necessary, I like Henri's tree ideas, maybe they can be used to enhance the current stateblock. That structure would be passed as an argument to the functions that the above table points to. A list of dirty states could specify which apply functions to execute, but you still need to pass in data to those functions, and the sampler number seems like it should be part of the data, not part of how things are indexed. To apply changes to sampler#6 you'd call the APPLY_SAMPLERS function, with a data arg that marks the 6th sampler dirty, and contains data for sampler 6.
Re: WineD3D State management - going live(TM)
n0dalus wrote: On 10/22/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: Constant is convenient, but if it can't meet all necessary requirements, I wouldn't hesitate to drop the idea - never compromise on design in favor of C optimizations. Tomorrow's hardware will make any non-algorithmic optimizations irrelevant. While this is true for most things, it shouldn't be applied in all cases. For things like graphics processing, I would say every bit of optimization is worth it, even at the expense of a little design flexibility. Bah.. excuses for bad design. Constant-time access is important, but you need to index on the right thing - see other mail. Keep in mind that having everyone in the world constantly upgrading their hardware because of attitudes like this is not sustainable -- Sure it is, my computer at work disagrees w/ you. a far better future would be where a standard computer is cheaper, needs less power, produces less noise and heat, and just does its job. Why? Just like you upgrade software to get new features and solve problems, you should upgrade hardware for the same purpose. Some problems are best solved in the hardware, rather than wasting programmers' time. What is it with software developers and old computers ?
Re: WineD3D State management - going live(TM)
device-addDirtyState(This, States[State]-representative) This way the code applying the states has to be called only once. What does addDirtyState() do when called multiple times with the same representative? This is still a bunch of code, not a magic instruction how to call gl directly. To apply a state to gl States[State]-func(State, stateblock); How would I mark vertex shader constant #3653 dirty, and apply it using your mechanism ? I think you agree that the state grouping is a bit inflexible. For example, the vertex declaration affects the fog settings. This way it would be consequent to group them. However, as other things depend on the vertex decl this will get a way to big group. The bigger the groups are the more likely it is that one of the states is changed, so this will eat performance. I think you're mixing dirtification of states with mapping which function applies those states, and I don't fully understand how this is going to work yet... Sampler states and bound textures are more difficult, as they are per-texture settings in gl and per device settings in d3d. That doesn't sound entirely correct - there are per-texture-unit settings in GL, and per-sampler settings in d3d. I thought we had to map the samplers to GL texture units. There is no point in putting them into the main state list, instead there should be a dirty list per supported sampler. Again, it seems to me that two different concepts are being mixed together - I certainly don't want to be typing the function to apply per sampler, when there should be one entry in a table somewhere, with the sampler passed as an argument. Dirtification is another topic, which I don't think you can solve with a list anyway. I hope that explained the whole plan a bit better than my last mail :-) Sure, but I'm still confused :)
Re: wined3d
Makes sense - reapplying the stateblock causes the states to be written back to the same stateblock *and* GL state to be applied. What we want is the states to be applied without being written back to the same stateblock. You can add if(capturing) write_to_update_stateblock(); else apply_states(); in device.c code to patch up the problem - it's done in some places, but not everywhere. However, what this is really showing is that the design doesn't make sense. Stateblock management should never have been mixed with applying GL states - all the GL code needs to be moved out of device.c and somewhere else. Hopefully Stefan D.'s work is heading in that direction. Ivan message for the wined3d folks... running the d3d tests under valgrind gives this list of calls of memcpy with overlapping buffers, which could give bad results (depending how the buffers overlap) is this a side effect of the tests, a harmless warning or something to be fixed ? A+ ==10159== Source and destination overlap in memcpy(0x1BD2F0, 0x1BD2F0, 16) ==10159==at 0x401C663: memcpy (mc_replace_strmem.c:116) ==10159==by 0x692FA93: IWineD3DDeviceImpl_SetVertexShaderConstantF (device.c:4856) ==10159==by 0x695A76D: IWineD3DStateBlockImpl_Apply (stateblock.c:645) ==10159==by 0x6939655: device_reapply_stateblock (device.c:7184) ==10159==by 0x6939E62: IWineD3DDeviceImpl_ActiveRender (device.c:7399) ==10159==by 0x6939075: IWineD3DDeviceImpl_SetRenderTarget (device.c:7034) ==10159==by 0x68DECFD: IDirect3DDevice9Impl_SetRenderTarget (device.c:391) ==10159==by 0x45A275E: switch_render_target (stateblock.c:291) ==10159==by 0x45A1EA8: execute_test_chain (stateblock.c:180) ==10159==by 0x45A2F3D: execute_test_chain_all (stateblock.c:467) ==10159==by 0x45A5BEE: test_state_management (stateblock.c:1446) ==10159==by 0x45A5C97: func_stateblock (stateblock.c:1466) ==10159== ==10159== Source and destination overlap in memcpy(0x1B22F8, 0x1B22F8, 16) ==10159==at 0x401C663: memcpy (mc_replace_strmem.c:116) ==10159==by 0x692F7E0: IWineD3DDeviceImpl_SetVertexShaderConstantI (device.c:4809) ==10159==by 0x695A7C5: IWineD3DStateBlockImpl_Apply (stateblock.c:650) ==10159==by 0x6939655: device_reapply_stateblock (device.c:7184) ==10159==by 0x6939E62: IWineD3DDeviceImpl_ActiveRender (device.c:7399) ==10159==by 0x6939075: IWineD3DDeviceImpl_SetRenderTarget (device.c:7034) ==10159==by 0x68DECFD: IDirect3DDevice9Impl_SetRenderTarget (device.c:391) ==10159==by 0x45A275E: switch_render_target (stateblock.c:291) ==10159==by 0x45A1EA8: execute_test_chain (stateblock.c:180) ==10159==by 0x45A2F3D: execute_test_chain_all (stateblock.c:467) ==10159==by 0x45A5BEE: test_state_management (stateblock.c:1446) ==10159==by 0x45A5C97: func_stateblock (stateblock.c:1466) ==10159== ==10159== Source and destination overlap in memcpy(0x1B22B8, 0x1B22B8, 4) ==10159==at 0x401C663: memcpy (mc_replace_strmem.c:116) ==10159==by 0x692F55E: IWineD3DDeviceImpl_SetVertexShaderConstantB (device.c:4763) ==10159==by 0x695A81D: IWineD3DStateBlockImpl_Apply (stateblock.c:655) ==10159==by 0x6939655: device_reapply_stateblock (device.c:7184) ==10159==by 0x6939E62: IWineD3DDeviceImpl_ActiveRender (device.c:7399) ==10159==by 0x6939075: IWineD3DDeviceImpl_SetRenderTarget (device.c:7034) ==10159==by 0x68DECFD: IDirect3DDevice9Impl_SetRenderTarget (device.c:391) ==10159==by 0x45A275E: switch_render_target (stateblock.c:291) ==10159==by 0x45A1EA8: execute_test_chain (stateblock.c:180) ==10159==by 0x45A2F3D: execute_test_chain_all (stateblock.c:467) ==10159==by 0x45A5BEE: test_state_management (stateblock.c:1446) ==10159==by 0x45A5C97: func_stateblock (stateblock.c:1466) ... ==10159== Source and destination overlap in memcpy(0x1AF808, 0x1AF808, 64) ==10159==at 0x401C663: memcpy (mc_replace_strmem.c:116) ==10159==by 0x69580DB: stateblock_savedstates_copy (stateblock.c:83) ==10159==by 0x695B2D5: IWineD3DStateBlockImpl_Apply (stateblock.c:805) ==10159==by 0x6939655: device_reapply_stateblock (device.c:7184) ==10159==by 0x6939E62: IWineD3DDeviceImpl_ActiveRender (device.c:7399) ==10159==by 0x6939075: IWineD3DDeviceImpl_SetRenderTarget (device.c:7034) ==10159==by 0x68DECFD: IDirect3DDevice9Impl_SetRenderTarget (device.c:391) ==10159==by 0x45A275E: switch_render_target (stateblock.c:291) ==10159==by 0x45A1EA8: execute_test_chain (stateblock.c:180) ==10159==by 0x45A2F3D: execute_test_chain_all (stateblock.c:467) ==10159==by 0x45A5BEE: test_state_management (stateblock.c:1446) ==10159==by 0x45A5C97: func_stateblock (stateblock.c:1466) (similar traceback several times) ... ==10159== Source and destination overlap in memcpy(0x1BA994, 0x1BA994, 24) ==10159==at 0x401C663: memcpy (mc_replace_strmem.c:116) ==10159==by 0x6924FD7: IWineD3DDeviceImpl_SetViewport (device.c:3264) ==10159==
Re: WineD3D State management - going live(TM)
States will be applied in drawprim. BltOverride and UnlockRect change states on their own, and they can put the states the change onto the dirty list so drawprim will reset them to what the application wants. What about Clear()? Maybe you should also provide the capability of applying a single dirty state [ or a group of them ].
Re: WineD3D State management - going live(TM)
States will be applied in drawprim. It will be good if *all* texture-related states were applied in drawprim, specifically. This is a prerequisite to VTF support, since that involves repacking pixel and vertex textures into a single array, and changing their indices [ should happen at drawprim, breaking any previously applied state on that texture ].
Re: WineD3D State management - going live(TM)
More? What are your plans for dealing with these: = SetLight() SetLightEnable() SetTexture() SetDepthStencilBuffer() SetRenderTarget() SetSomethingElseThatsNotARenderState().
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
Ivan Gyurdiev wrote: Ivan Gyurdiev wrote: Karsten Elfenbein wrote: * don't prevent IUnknown_Release of pDecl in the exit section and fix for http://bugs.winehq.org/show_bug.cgi?id=5656 This seems wrong, d3d9 does not keep an internal reference of the vdecl. Try the attached patch instead, let me know if it works... ...better yet, try this one Based on further tests, I don't believe this patch is correct.. but it will fix the memory leak. I'm still unsure what kind of insanity the native API is doing internally - in process of figuring that out. When adding this code, I spent a lot of time testing conversion between the fvf and decl, and that has been shown to be correct - but management of the decl afterwards seems rather wrong to me now, and that it needs additional tests. Tests Show: Refcount of converted decl is 1 after GetVertexDeclaration() Tests Show: Refcount of converted decl *remains* 1 over a SetFVF call Tests Show: Refcount of converted decl *remains* 1 over a SetVertexDeclaration(NULL) call I currently suspect that the convertedDecl is shared internally, and refcount behavior does not follow normal code path on Get(). Another possibility is that the internal storage is FVF-based, and conversions are ran in the GetVertexDecl() code [ or the object is cloned ]... but other tests show this not to be the case.
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
H. Verbeet wrote: On 14/10/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: Ivan Gyurdiev wrote: Based on further tests, I don't believe this patch is correct.. but it will fix the memory leak. I'm still unsure what kind of insanity the native API is doing internally - in process of figuring that out. When adding this code, I spent a lot of time testing conversion between the fvf and decl, and that has been shown to be correct - but management of the decl afterwards seems rather wrong to me now, and that it needs additional tests. Tests Show: Refcount of converted decl is 1 after GetVertexDeclaration() Tests Show: Refcount of converted decl *remains* 1 over a SetFVF call Tests Show: Refcount of converted decl *remains* 1 over a SetVertexDeclaration(NULL) call That's what happens with regular declarations as well, see test_get_set_vertex_declaration() in the d3d9 vertexdeclaration tests. Calling SetVertexDeclaration() doesn't change the refcount, but calling GetVertexDeclaration does (however, note that wined3d internally does change refcounts). Exactly... so why is the refcount 1 *after* GetVertexDeclaration. What does GetVertexDeclaration() do for the generated decl? I don't think it increases the refcount like it does normally.
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
H. Verbeet wrote: On 14/10/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: Exactly... so why is the refcount 1 *after* GetVertexDeclaration. Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer? No.. I tried that too. The same pointer is returned.
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
Stefan Dösinger wrote: Am Samstag 14 Oktober 2006 20:53 schrieb Ivan Gyurdiev: H. Verbeet wrote: On 14/10/06, Ivan Gyurdiev [EMAIL PROTECTED] wrote: Exactly... so why is the refcount 1 *after* GetVertexDeclaration. Perhaps the declaration in only generated when calling GetVertexDeclaration(). If you call it twice, does it return the same pointer? No.. I tried that too. The same pointer is returned. Perhaps windows has a static internal vertex declaration? Does the refcount increase when you call GetVertexDeclaration more often? Yes, the refcount increases. == Here's probably what it does: - SetFVF()... sets FVF - GetVertexDeclaration() ... vdecl is null, but there is an FVF, grab the FVF, and make a declaration out of it. Store the declaration, and return it to the caller with refcount 1. - GetVertexDeclaration() again - just grab the stored declaration and follow regular code path, increasing the refcount. Now, if the caller does not release the ref, it will leak memory [ because d3d certainly doesn't free it - not on SetFVF, or on SetVertexDecl ]. On the other hand, if the caller does release it, will it cause damage (1) on subsequent get calls? (2) at draw time... and how can we test that. This whole part of the API is very strange, and completely undocumented. Perhaps those conversions don't matter at all in real applications. The primary reason they fixed stuff, is probably that the FVF codepath wasn't working together with shaders, so switching to decl internally caused improvement.
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
Karsten Elfenbein wrote:
* don't prevent IUnknown_Release of pDecl in the exit section and fix
for http://bugs.winehq.org/show_bug.cgi?id=5656
This seems wrong, d3d9 does not keep an internal reference of the vdecl.
Try the attached patch instead, let me know if it works...
---
dlls/d3d9/d3d9_private.h |4
dlls/d3d9/device.c|1 +
dlls/d3d9/vertexdeclaration.c |6 +-
3 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/dlls/d3d9/d3d9_private.h b/dlls/d3d9/d3d9_private.h
index 2d6397e..c5cef9f 100644
--- a/dlls/d3d9/d3d9_private.h
+++ b/dlls/d3d9/d3d9_private.h
@@ -176,6 +176,10 @@ typedef struct IDirect3DDevice9Impl
/* IDirect3DDevice9 fields */
IWineD3DDevice *WineD3DDevice;
+/* A vertex declaration was converted from setFVF.
+ * Keep track of it, so it can be properly freed */
+IDirect3DVertexDeclaration9 *convertedDecl;
+
} IDirect3DDevice9Impl;
diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c
index 6eeaceb..185a7ef 100644
--- a/dlls/d3d9/device.c
+++ b/dlls/d3d9/device.c
@@ -731,6 +731,7 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
hr = IDirect3DDevice9Impl_SetVertexDeclaration(iface, pDecl);
if (hr != S_OK) goto exit;
+ This-convertedDecl = pDecl;
pDecl = NULL;
exit:
diff --git a/dlls/d3d9/vertexdeclaration.c b/dlls/d3d9/vertexdeclaration.c
index 246b52f..88f5909 100644
--- a/dlls/d3d9/vertexdeclaration.c
+++ b/dlls/d3d9/vertexdeclaration.c
@@ -300,8 +300,12 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
IDirect3DVertexDeclaration9Impl *pDeclImpl =
(IDirect3DVertexDeclaration9Impl *)pDecl;
HRESULT hr = D3D_OK;
-TRACE((%p) : Relay\n, iface);
+if (This-convertedDecl This-convertedDecl != pDecl) {
+IUnknown_Release(This-convertedDecl);
+This-convertedDecl = NULL;
+}
+TRACE((%p) : Relay\n, iface);
hr = IWineD3DDevice_SetVertexDeclaration(This-WineD3DDevice, pDeclImpl ==
NULL ? NULL : pDeclImpl-wineD3DVertexDeclaration);
return hr;
--
1.4.2.1
Re: [D3D9] fix for SetFVF memleak [2nd try+patch]
Ivan Gyurdiev wrote:
Karsten Elfenbein wrote:
* don't prevent IUnknown_Release of pDecl in the exit section and fix
for http://bugs.winehq.org/show_bug.cgi?id=5656
This seems wrong, d3d9 does not keep an internal reference of the vdecl.
Try the attached patch instead, let me know if it works...
...better yet, try this one
---
dlls/d3d9/d3d9_private.h |4
dlls/d3d9/device.c|3 +++
dlls/d3d9/vertexdeclaration.c |6 +-
3 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/dlls/d3d9/d3d9_private.h b/dlls/d3d9/d3d9_private.h
index 2d6397e..c5cef9f 100644
--- a/dlls/d3d9/d3d9_private.h
+++ b/dlls/d3d9/d3d9_private.h
@@ -176,6 +176,10 @@ typedef struct IDirect3DDevice9Impl
/* IDirect3DDevice9 fields */
IWineD3DDevice *WineD3DDevice;
+/* A vertex declaration was converted from setFVF.
+ * Keep track of it, so it can be properly freed */
+IDirect3DVertexDeclaration9 *convertedDecl;
+
} IDirect3DDevice9Impl;
diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c
index 6eeaceb..5a836b9 100644
--- a/dlls/d3d9/device.c
+++ b/dlls/d3d9/device.c
@@ -60,6 +60,8 @@ static ULONG WINAPI IDirect3DDevice9Impl
if (ref == 0) {
IWineD3DDevice_Uninit3D(This-WineD3DDevice);
IWineD3DDevice_Release(This-WineD3DDevice);
+ if (This-convertedDecl != NULL)
+ IUnknown_Release(This-convertedDecl);
HeapFree(GetProcessHeap(), 0, This);
}
return ref;
@@ -731,6 +733,7 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
hr = IDirect3DDevice9Impl_SetVertexDeclaration(iface, pDecl);
if (hr != S_OK) goto exit;
+ This-convertedDecl = pDecl;
pDecl = NULL;
exit:
diff --git a/dlls/d3d9/vertexdeclaration.c b/dlls/d3d9/vertexdeclaration.c
index 246b52f..88f5909 100644
--- a/dlls/d3d9/vertexdeclaration.c
+++ b/dlls/d3d9/vertexdeclaration.c
@@ -300,8 +300,12 @@ HRESULT WINAPI IDirect3DDevice9Impl_Se
IDirect3DVertexDeclaration9Impl *pDeclImpl =
(IDirect3DVertexDeclaration9Impl *)pDecl;
HRESULT hr = D3D_OK;
-TRACE((%p) : Relay\n, iface);
+if (This-convertedDecl This-convertedDecl != pDecl) {
+IUnknown_Release(This-convertedDecl);
+This-convertedDecl = NULL;
+}
+TRACE((%p) : Relay\n, iface);
hr = IWineD3DDevice_SetVertexDeclaration(This-WineD3DDevice, pDeclImpl ==
NULL ? NULL : pDeclImpl-wineD3DVertexDeclaration);
return hr;
--
1.4.2.1
Re: [D3D9 1/8] Remove const qualifier on state_test data
Alexandre Julliard wrote: Ivan Gyurdiev [EMAIL PROTECTED] writes: It's already marked const in the parameters of the set and get functions, which means it can't be modified there (arg 3): +void (*set_handler) (IDirect3DDevice9* device, const struct state_test* test, const void* data_in); +void (*get_handler) (IDirect3DDevice9* device, const struct state_test* test, const void* data_out); Which is precisely why the data pointers have to be const too. Since the tests don't modify anything, Well... the way I see it, the test is composed of get/set, while the init/teardown is just setup for the test, which may or may not include allocating the data pointers on the heap (and correspondingly freeing that data). Therefore the data is const in one place, and non-const in the other where I don't know what the test is going to do (by virtue of polymorphism). Yes, I suppose you could look at that as two completely separate usages of the same data, requiring separate pointers, but that seems rather odd to me. Actually the other direction is possible too, keeping the data const, and just casting off the const in the teardown function, but that seemed uglier. it must be possible to give them constant data. With your scheme you need to cast const off, which is ugly and will cause compiler warnings. I don't know about ugly, but the casts actually prevent warnings, instead of causing them... Is there a compiler flag that you use to cause warnings? If so, please let me know what it is, and I'll fix the code.
Re: [D3D9 1/8] Remove const qualifier on state_test data
Well, ok, I'm starting to see this your way now. I'll make this change, because of two reasons: 1) I'll need some kind of data structure called test_context anyway, whether or not those pointers are copied there. Future tests will probably require the ability to store additional data during the lifetime of the test, that's not one of those data sample points. 2) The teardown function in some of those examples frees certain data pointers, and not others. That's because the same data is used on two pointers, so you can't free it twice. However, that's ugly, and could be better handled using a separate copy of the pointer.
Re: [WINED3D 1/6] Move D3DSIO structure into the WINED3D namespace.
Alexandre Julliard wrote: Ivan Gyurdiev [EMAIL PROTECTED] writes: wined3d_types.h would be for types equivalent to the d3d types - essentially no maintainance, simply map one-to-one to d3d. wined3d_private.h would be for shared structures that we come up with. Also, wined3d_private.h includes wined3d_types.h. I will comply if you want it this way, but I disagree - smaller files are easier to work with. In fact, I think having a single private header per dll is probably a mistake as well. I don't have a problem with creating another file if you think that's better, but please come up with another name, don't use the same name as the global header. Henri and Stefan, any preference? (you better have an opinion since you're making me do this in the first place :)
Re: [WINED3D 1/6] Move D3DSIO structure into the WINED3D namespace.
Alexandre Julliard wrote: Ivan Gyurdiev [EMAIL PROTECTED] writes: Note: A new file was started for this purpose: wined3d_types.h. This is _not_ the same as the one in the wine include folder - that one should be for types which appear in the interface of wined3d, and must therefore be exposed to other dlls. This one is private. I think the dll interface should be strictly enforced, so private structures have no place outside our folder. This will cause massive confusion. There is already a wined3d_private.h file, you should put that stuff in there. The amount of structures added will quickly result in an unmaintainable header. There's a very clear difference between the two - wined3d_types.h would be for types equivalent to the d3d types - essentially no maintainance, simply map one-to-one to d3d. wined3d_private.h would be for shared structures that we come up with. Also, wined3d_private.h includes wined3d_types.h. I will comply if you want it this way, but I disagree - smaller files are easier to work with. In fact, I think having a single private header per dll is probably a mistake as well.
