February 2014 - Wine-Devel - list.winehq.org

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Henri Verbeet

On 19 February 2014 20:06, Ken Thomases <ken(a)codeweavers.com> wrote: > Hmm. The one issue is that, if the context_set_pixel_format() in context_update_window() fails, the context is marked as invalid and left that way. If we simply set needs_set instead, then context_acquire() will call context_set_gl_context(). That will try context_set_pixel_format() again but this time, if it fails, it will try the swapchain backup DC. I don't really know if that's desired. I assume the code is structured the way it is for a reason (i.e. a call to context_set_pixel_format() immediately before a call to context_set_gl_context(), which already begins with a call to the former). > Trying the backup DC would be correct. I probably just forgot to get rid of the context_set_pixel_format() call in context_update_window() back when I made context_restore_gl_context() try to restore the pixel format as well. The backup DC code was added even later by Matteo.

Feb. 19, 2014

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Henri Verbeet

On 19 February 2014 19:25, Ken Thomases <ken(a)codeweavers.com> wrote: >> I think that mostly works, but you'll probably want to clear needs_set >> in context_set_gl_context() instead of in context_release(). > > Hmm. The old logic had context_acquire() doing context_set_gl_context() each time if context_enter() had set restore_ctx at the outmost level. I didn't want to change that because I wasn't confident it was superfluous. > If you only clear it in the outer context_release(), inner context_acquire() calls will call context_set_gl_context() without needing to. >> I think >> there's also an argument somewhere that context_update_window() should >> set needs_set instead of calling context_set_pixel_format() and >> context_set_gl_context(). > > Yeah, it could do that. Actually, looking at the current code, context_acquire() calls context_update_window() before context_enter(). Couldn't that screw up context_enter()'s ability to a) notice that it needs to restore the context, or b) record the proper context to restore? > Yeah, I think so. > > I have to say that my approach would be to record the state to restore at exactly the place where it is changed. So, the old WGL context and DC would be remembered at the call to wglMakeCurrent() or wglMakeContextCurrentARB(). In my pixel format patch, the old pixel format is remembered at the call to SetPixelFormat(). Etc. It's conceptually not that different, context_acquire() / context_enter() is the wined3d equivalent of wglMakeCurrent(). The split between context_acquire() and context_enter() may have room for improvement. Also note that some parts are just left over from back when we still had ActivateContext().

Feb. 19, 2014

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Ken Thomases

On Feb 19, 2014, at 12:25 PM, Ken Thomases wrote: > On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote: > >> I think >> there's also an argument somewhere that context_update_window() should >> set needs_set instead of calling context_set_pixel_format() and >> context_set_gl_context(). Hmm. The one issue is that, if the context_set_pixel_format() in context_update_window() fails, the context is marked as invalid and left that way. If we simply set needs_set instead, then context_acquire() will call context_set_gl_context(). That will try context_set_pixel_format() again but this time, if it fails, it will try the swapchain backup DC. I don't really know if that's desired. I assume the code is structured the way it is for a reason (i.e. a call to context_set_pixel_format() immediately before a call to context_set_gl_context(), which already begins with a call to the former). -Ken

Feb. 19, 2014

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Ken Thomases

On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote: > On 19 February 2014 04:30, Ken Thomases <ken(a)codeweavers.com> wrote: > >> @@ -3051,7 +3055,7 @@ struct wined3d_context *context_acquire(const struct wined3d_device *device, str >> if (!context_set_current(context)) >> ERR("Failed to activate the new context.\n"); >> } >> - else if (context->restore_ctx) >> + else if (context->needs_set) >> { >> context_set_gl_context(context); >> } > I think that mostly works, but you'll probably want to clear needs_set > in context_set_gl_context() instead of in context_release(). Hmm. The old logic had context_acquire() doing context_set_gl_context() each time if context_enter() had set restore_ctx at the outmost level. I didn't want to change that because I wasn't confident it was superfluous. > I think > there's also an argument somewhere that context_update_window() should > set needs_set instead of calling context_set_pixel_format() and > context_set_gl_context(). Yeah, it could do that. Actually, looking at the current code, context_acquire() calls context_update_window() before context_enter(). Couldn't that screw up context_enter()'s ability to a) notice that it needs to restore the context, or b) record the proper context to restore? I have to say that my approach would be to record the state to restore at exactly the place where it is changed. So, the old WGL context and DC would be remembered at the call to wglMakeCurrent() or wglMakeContextCurrentARB(). In my pixel format patch, the old pixel format is remembered at the call to SetPixelFormat(). Etc. You'd check to make sure that something isn't already remembered, of course, so you don't overwrite the remembered app-set state with wined3d-set state. My approach can be less efficient because you check the state too often. The current code uses the context->level transition from 0 to 1 to limit it. But we can use flags (something like "restore_ctx_valid", for example) to remember if we've checked since that transition and avoid checking again (even though restore_ctx itself may remain NULL). -Ken

Feb. 19, 2014

Re: ff-core effect id handling in case of a failed effect upload

by Elias Vanderstuyft

On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula <anssi.hannula(a)iki.fi> wrote: > (added Dmitry to CC) > > 19.02.2014 13:42, Elias Vanderstuyft kirjoitti: >> Hi, >> > > Hi, > >> In the process of reviewing the Wine DInput translation layer, I >> noticed an inconvenience (in the ff-core implementation?) that can >> possibly lead to confusing problems to application developers (not >> only for Wine), in short: >> If a new (id==-1) effect was uploaded (look at >> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL), >> ff-core will have assigned a positive number to the effect id. This >> can be confusing because the dev->ff->effects[] array will not contain >> an element at the index of that new effect id. > > I agree that this is a bit confusing, and the kernel code should > probably be modified to not clobber the ioctl-provided effect on failure > (effect->id is set to an "undefined" value, i.e. next free effect slot). > > Dmitry, WDYT? > > The downside is that if changed, any new userspace code may unknowingly > depend on the fixed non-clobbering behavior (though apparently they > already do, as evidenced by Wine DInput, so might not be much of an > argument...). I don't think that's a problem. Something that can be more problematic, is that a newer application assumes the fixed non-clobbering behaviour, but runs on a kernel with the old clobbering behaviour, could experience problems. > >> Here is a more elaborated description/discussion: >> - This is a bug that is either the responsibility of the Linux kernel, >> or of Wine (and possibly other applications that do the same thing as >> described below): >> It is caused by the following situation: >> When uploading an effect, the specific kernel device driver >> may return an error, >> e.g. EINVAL for example when a periodic's effect period is set to zero. >> This error will then be returned by "ioctl(*(This->fd), >> EVIOCSFF, &This->effect)". >> With or without error, one can find out that >> /drivers/input/ff-core.c:input_ff_upload(...) is called, >> which will set effect->id >= 0, if it was set to -1 (=> new >> effect created) by the user. >> >> But in case of an error: >> - Assume effect->id was set to -1 by the user: >> The error is reported by ff->upload(...) at >> /drivers/input/ff-core.c:167, >> the effect->id will also be set >= 0 (*). >> The offending effect will not be saved in the >> ff->effects[] array (***). >> - Assume effect->id was set >= 0 by the user (and >> ff->effects[effect->id] is a valid existing effect): >> The error is reported by ff->upload(...) at >> /drivers/input/ff-core.c:167, >> the effect->id will remain unchanged (**). >> The offending effect will not overwrite the >> ff->effects[effect->id] element (****). >> >> Is this (see *, **, *** and ****) desired behaviour? >> - If yes: >> Change the following in Wine's dinput/effect_linuxinput.c:90 : >> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) { >> to : >> int effectId_old = This->effect.id; >> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) { >> This->effect.id = effectId_old; >> - If no for *: >> Kernel code /drivers/input/ff-core.c:input_ff_upload(...) >> has to be patched to revert "effect->id" back to its original value >> set by the user, >> which is only needed when the initial (by user) value of >> "effect->id" was equal to -1. >> - If no for **** (or maybe also ***): >> ff->effects[effect->id] could be replaced by an 'empty' >> effect (however this can get complex because the effect's type has to >> remain unchanged) >> This would be a change in the kernel code >> /drivers/input/ff-core.c:input_ff_upload(...). >> - If no for **: >> I don't really know. Discussion is needed. >> >> - In my opinion, **, *** and **** are desired behaviour, while >> * should leave the effect->id at -1. > > Right. > >> In that case, Wine's dinput implementation does not have >> to be patched, and the kernel code only should apply a minor patch. > > Well, maybe better to change dinput anyway so that it can work properly > with current kernel versions (assuming this gets changed in the future)? > Not my call, though. Yes, agreed. I'll include the code (with effectId_old variable) and this discussion when I submit my whole patchset for Wine's DInput libs to Wine-devel mailing list. Right now, I don't know who I should personally mail to. > > -- > Anssi Hannula Elias

Feb. 19, 2014

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Henri Verbeet

On 19 February 2014 17:10, Ken Thomases <ken(a)codeweavers.com> wrote: > But wined3d doesn't always restore when returning to the app (because the app hadn't made a GL context current) and then doesn't set its context when the app calls back into it. But between those points, the app could set a pixel format on a window. > Well, no, because SetPixelFormat() can't set the pixel format after wined3d did. (Ignoring hypothetical applications that know about WGL_WINE_pixel_format_passthrough.)

Feb. 19, 2014

Re: ff-core effect id handling in case of a failed effect upload

by Anssi Hannula

(added Dmitry to CC) 19.02.2014 13:42, Elias Vanderstuyft kirjoitti: > Hi, > Hi, > In the process of reviewing the Wine DInput translation layer, I > noticed an inconvenience (in the ff-core implementation?) that can > possibly lead to confusing problems to application developers (not > only for Wine), in short: > If a new (id==-1) effect was uploaded (look at > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL), > ff-core will have assigned a positive number to the effect id. This > can be confusing because the dev->ff->effects[] array will not contain > an element at the index of that new effect id. I agree that this is a bit confusing, and the kernel code should probably be modified to not clobber the ioctl-provided effect on failure (effect->id is set to an "undefined" value, i.e. next free effect slot). Dmitry, WDYT? The downside is that if changed, any new userspace code may unknowingly depend on the fixed non-clobbering behavior (though apparently they already do, as evidenced by Wine DInput, so might not be much of an argument...). > Here is a more elaborated description/discussion: > - This is a bug that is either the responsibility of the Linux kernel, > or of Wine (and possibly other applications that do the same thing as > described below): > It is caused by the following situation: > When uploading an effect, the specific kernel device driver > may return an error, > e.g. EINVAL for example when a periodic's effect period is set to zero. > This error will then be returned by "ioctl(*(This->fd), > EVIOCSFF, &This->effect)". > With or without error, one can find out that > /drivers/input/ff-core.c:input_ff_upload(...) is called, > which will set effect->id >= 0, if it was set to -1 (=> new > effect created) by the user. > > But in case of an error: > - Assume effect->id was set to -1 by the user: > The error is reported by ff->upload(...) at > /drivers/input/ff-core.c:167, > the effect->id will also be set >= 0 (*). > The offending effect will not be saved in the > ff->effects[] array (***). > - Assume effect->id was set >= 0 by the user (and > ff->effects[effect->id] is a valid existing effect): > The error is reported by ff->upload(...) at > /drivers/input/ff-core.c:167, > the effect->id will remain unchanged (**). > The offending effect will not overwrite the > ff->effects[effect->id] element (****). > > Is this (see *, **, *** and ****) desired behaviour? > - If yes: > Change the following in Wine's dinput/effect_linuxinput.c:90 : > if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) { > to : > int effectId_old = This->effect.id; > if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) { > This->effect.id = effectId_old; > - If no for *: > Kernel code /drivers/input/ff-core.c:input_ff_upload(...) > has to be patched to revert "effect->id" back to its original value > set by the user, > which is only needed when the initial (by user) value of > "effect->id" was equal to -1. > - If no for **** (or maybe also ***): > ff->effects[effect->id] could be replaced by an 'empty' > effect (however this can get complex because the effect's type has to > remain unchanged) > This would be a change in the kernel code > /drivers/input/ff-core.c:input_ff_upload(...). > - If no for **: > I don't really know. Discussion is needed. > > - In my opinion, **, *** and **** are desired behaviour, while > * should leave the effect->id at -1. Right. > In that case, Wine's dinput implementation does not have > to be patched, and the kernel code only should apply a minor patch. Well, maybe better to change dinput anyway so that it can work properly with current kernel versions (assuming this gets changed in the future)? Not my call, though. -- Anssi Hannula

Feb. 19, 2014

Re: [1/2] shell32: Correct create flags when SEE_MASK_NO_CONSOLE isn't specified (resend)

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > Hi, > > > Changelog: > shell32: Correct create flags when SEE_MASK_NO_CONSOLE isn't specified There's more to it than that, now it's popping up consoles all over the place during make test. -- Alexandre Julliard julliard(a)winehq.org

Feb. 19, 2014

Re: [PATCH 2/4] d3d9/tests: Test that Direct3D9 doesn't modify the pixel format of the window it targets. (try 4)

by Ken Thomases

On Feb 19, 2014, at 4:36 AM, Henri Verbeet wrote: > On 19 February 2014 02:53, Ken Thomases <ken(a)codeweavers.com> wrote: >> OK, I can see how that would happen. However, my wined3d patch didn't change that part of the logic. I think my test just exposed a pre-existing bug. It doesn't show up without my wined3d patch applied because, in that case, wined3d fails to restore the window's pixel format and leaves it with a double-buffered pixel format. The two bugs cancel each other out and fixing one allowed the other to be revealed. >> > With the current code, the pixel format only needs changing if the GL > context also does, that's just an implication of setting/restoring > both at the same time. But wined3d doesn't always restore when returning to the app (because the app hadn't made a GL context current) and then doesn't set its context when the app calls back into it. But between those points, the app could set a pixel format on a window. -Ken

Feb. 19, 2014

Re: [idea]wine support for Chrome OS

by Vincent Povirk

I don't think it's possible for existing Windows code to run in Native Client. As I understand it, NaCl verifies all x86 code to make sure it's safe before executing it, and Wine needs to load x86 code compiled for Windows directly, which will not be using the safe subset of x86 that NaCl requires. One would need a proper virtual machine to make that work, something like qemu perhaps. However, the pipelight project may be interested in support for PPAPI.

Feb. 19, 2014