dlls/wined3d/device.c GetCreationParameters

[Al Tobey]

I'd just post to wine-patches, but I think this needs another set of
eyes.   According to MSDN, this just sets a pointer to the creation
parameters.    The attached patch makes this function work, but I'm
not sure if returning a pointer to the original parameters is the
right approach or if they should be copied first.

This patch makes a bunch of the Ogre3d demos work, which are great for
testing since you can run the same demo in OpenGL, D3D9, and D3D7
modes.

http://msdn.microsoft.com/archive/default.asp?url=/archive/en-us/directx9_c/...

http://www.ogre3d.org/index.php?option=com_remository&Itemid=57&func=selectc...
http://easynews.dl.sourceforge.net/sourceforge/ogre/OgreDemos-1.0.4.zip

I'm currently playing with compiling Ogre3d against winelib and
finding it's shaking out a bunch of missing stuff in d3d9 - patches
may follow.

-Al Tobey

[Al Tobey]

Ack, after thinking about it and talking with people on IRC, I think I
did the wrong thing.   New patch attached that just copies the
parameters one-by-one to the passed-in struct.   Both methods work,
though, so this probably needs to be tested on Windows.

In any case, this one is probably safest for now.
-Al Tobey

On 1/14/06, Al Tobey <tobert(a)gmail.com> wrote:
> I'd just post to wine-patches, but I think this needs another set of
> eyes.   According to MSDN, this just sets a pointer to the creation
> parameters.    The attached patch makes this function work, but I'm
> not sure if returning a pointer to the original parameters is the
> right approach or if they should be copied first.
>
> This patch makes a bunch of the Ogre3d demos work, which are great for
> testing since you can run the same demo in OpenGL, D3D9, and D3D7
> modes.
>
> http://msdn.microsoft.com/archive/default.asp?url=/archive/en-us/directx9_c/...
>
> http://www.ogre3d.org/index.php?option=com_remository&Itemid=57&func=selectc...
> http://easynews.dl.sourceforge.net/sourceforge/ogre/OgreDemos-1.0.4.zip
>
> I'm currently playing with compiling Ogre3d against winelib and
> finding it's shaking out a bunch of missing stuff in d3d9 - patches
> may follow.
>
> -Al Tobey

[Aric Cyr]

Al Tobey <tobert <at> gmail.com> writes:
> Ack, after thinking about it and talking with people on IRC, I think I
> did the wrong thing.   New patch attached that just copies the
> parameters one-by-one to the passed-in struct.   Both methods work,
> though, so this probably needs to be tested on Windows.
>
> In any case, this one is probably safest for now.
> -Al Tobey

Interesting that your original patch made things work at all... you set the
pParameters variable, which is then promptly forgotten upon return from the
function.  In effect your original patch does nothing... so why it would make
things work is beyond me :)

In any case, your second patch looks much better, but as Robert said, can't you
simplify it with a memcpy() instead of an exhaustive list of the structure
members?  That is, assuming This->createParms and pParameters are of the same
type you could just do:

{
  ...
  memcpy(pParameters, &This->createParms, sizeof(D3DDEVICE_CREATION_PARAMETERS));
  return D3D_OK;
}

This is much less error-prone than a list of structure members.

Regards,
  Aric

[H. Verbeet]

On 16/01/06, Aric Cyr <Aric.Cyr(a)gmail.com> wrote:
> Interesting that your original patch made things work at all... you set the
> pParameters variable, which is then promptly forgotten upon return from the
> function.  In effect your original patch does nothing... so why it would make
> things work is beyond me :)
> If the D3DDEVICE_CREATION_PARAMETERS struct got zeroed out before
> being passed to GetCreationParameters, DeviceType would now be 0 while
> it was 1 (D3DDEVTYPE_HAL) before. I don't know if that's enough to
> make things work.
> {
>   ...
>   memcpy(pParameters, &This->createParms, sizeof(D3DDEVICE_CREATION_PARAMETERS));
>   return D3D_OK;
> }
>
> This is much less error-prone than a list of structure members.

*pParameters = This->createParms;
should probably work as well, if "struct assignment" is acceptable in wine.
And of course checking if pParameters is a valid pointer in the first
place is probably a good idea as well.

[Aric Cyr]

On 1/16/06, H. Verbeet <hverbeet(a)gmail.com> wrote:
> If the D3DDEVICE_CREATION_PARAMETERS struct got zeroed out before
> being passed to GetCreationParameters, DeviceType would now be 0 while
> it was 1 (D3DDEVTYPE_HAL) before. I don't know if that's enough to
> make things work.

Yes good point.  I forgot that his original patch also included the
deletion of a few lines.  That is the only reason I could see for his
changes to actually have any effect as the pointer-assignment clearly
does nothing.
> And of course checking if pParameters is a valid pointer in the first
> place is probably a good idea as well.

I agree, definitely a NULL check should be added for pParameters.

--
Aric Cyr <Aric.Cyr at gmail dot com>    (http://acyr.net)

[Al Tobey]

Thanks for the feedback everybody.   I'll rewrite it along with a test
of some sort tonight.

-Al Tobey

On 1/16/06, Aric Cyr <aric.cyr(a)gmail.com> wrote:
> On 1/16/06, H. Verbeet <hverbeet(a)gmail.com> wrote:
> > If the D3DDEVICE_CREATION_PARAMETERS struct got zeroed out before
> > being passed to GetCreationParameters, DeviceType would now be 0 while
> > it was 1 (D3DDEVTYPE_HAL) before. I don't know if that's enough to
> > make things work.
>
> Yes good point.  I forgot that his original patch also included the
> deletion of a few lines.  That is the only reason I could see for his
> changes to actually have any effect as the pointer-assignment clearly
> does nothing.
> > And of course checking if pParameters is a valid pointer in the first
> > place is probably a good idea as well.
>
> I agree, definitely a NULL check should be added for pParameters.
>
> --
> Aric Cyr <Aric.Cyr at gmail dot com>    (http://acyr.net)

[Al Tobey]

Here is the new patch.   I did some additional testing and couldn't
get the same app to fail on that function again.   It still works with
this new patch, so I'm assuming for now that it's correct.    Anyways,
thanks again.

-Al Tobey

On 1/16/06, Aric Cyr <aric.cyr(a)gmail.com> wrote:
> On 1/16/06, H. Verbeet <hverbeet(a)gmail.com> wrote:
> > If the D3DDEVICE_CREATION_PARAMETERS struct got zeroed out before
> > being passed to GetCreationParameters, DeviceType would now be 0 while
> > it was 1 (D3DDEVTYPE_HAL) before. I don't know if that's enough to
> > make things work.
>
> Yes good point.  I forgot that his original patch also included the
> deletion of a few lines.  That is the only reason I could see for his
> changes to actually have any effect as the pointer-assignment clearly
> does nothing.
> > And of course checking if pParameters is a valid pointer in the first
> > place is probably a good idea as well.
>
> I agree, definitely a NULL check should be added for pParameters.
>
> --
> Aric Cyr <Aric.Cyr at gmail dot com>    (http://acyr.net)

[Aric Cyr]

Al Tobey <tobert <at> gmail.com> writes:
> Here is the new patch.   I did some additional testing and couldn't
> get the same app to fail on that function again.   It still works with
> this new patch, so I'm assuming for now that it's correct.    Anyways,
> thanks again.

Hi Al,

Almost got it this time :)
You should be checking that pParameters is not NULL, not &This->createParms. 
Note that if This is not-NULL, &This->creatParms will be not null as well since
you are just getting the address of a struct.  Also on failure, according to
MSDN, you shoudl be returning D3DERR_INVALIDCALL not OUTOFVIDEOMEMORY.

Probably the following would be most correct:

---
{
  IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *) iface;

  if (This == NULL || pParameters == NULL)
    return D3DERR_INVALIDCALL;

  memcpy(pParameters, &This->createParms, sizeof(D3DDEVICE_CREATION_PARAMETERS));
  return D3D_OK;
}
---

Regards,
  Aric

[Al Tobey]

> Hi Al,
>
> Almost got it this time :)
> You should be checking that pParameters is not NULL, not &This->createParms.
> Note that if This is not-NULL, &This->creatParms will be not null as well since
> you are just getting the address of a struct.  Also on failure, according to
> MSDN, you shoudl be returning D3DERR_INVALIDCALL not OUTOFVIDEOMEMORY.
>
> Probably the following would be most correct:

I've patchified Aric's corrections.   That's what I get for not doing
any C/C++ for 5 years ...
I'll remember to check MSDN as well next time for the return code - I
just scanned the rest of the file and saw a lot of stuff returning
OUTOFVIDEOMEMORY.

-Al

[Robert Shearman]

Aric Cyr wrote:
> Al Tobey <tobert <at> gmail.com> writes:
> > Here is the new patch.   I did some additional testing and couldn't
> > get the same app to fail on that function again.   It still works with
> > this new patch, so I'm assuming for now that it's correct.    Anyways,
> > thanks again.
>
> Hi Al,
>
> Almost got it this time :)
> You should be checking that pParameters is not NULL, not &This->createParms. 
> Note that if This is not-NULL, &This->creatParms will be not null as well since
> you are just getting the address of a struct.  Also on failure, according to
> MSDN, you shoudl be returning D3DERR_INVALIDCALL not OUTOFVIDEOMEMORY.
>
> Probably the following would be most correct:
>
> ---
> {
>  IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *) iface;
>
> if (This == NULL || pParameters == NULL)

This should never be NULL as where did the application get the address 
of the function from?

-- 
Rob Shearman

[H. Verbeet]

On 17/01/06, Robert Shearman <rob(a)codeweavers.com> wrote:
> This should never be NULL as where did the application get the address
> of the function from?
> Well, The function could be called directly from inside wined3d, or
> the application could store the address somewhere. Something along the
> lines of:

some_function_ptr = object1->lpVtbl->GetCreationParameters;
some_function_ptr(object1, ¶meters);
some_function_ptr(object2, ¶meters);

However, in either case you probably want to fail hard and get a backtrace.

[Aric Cyr]

H. Verbeet <hverbeet <at> gmail.com> writes:
> On 17/01/06, Robert Shearman <rob <at> codeweavers.com> wrote:
> > This should never be NULL as where did the application get the address
> > of the function from?
> > Well, The function could be called directly from inside wined3d, or
> > the application could store the address somewhere.
>
> However, in either case you probably want to fail hard and get a backtrace.

Ya, I thought about that after I sent my previous mail as well...  an assert
would probably be more useful for checking "This".  I also disagree that "This"
is guaranteed to always be non-NULL.  There really is no way you can force
policy how a user calls the function, so minimally checking (or aborting) on
NULL is a sane thing to do.  It doesn't hurt the code, and catches potential
usage problems.

Regards,
  Aric

[Alexandre Julliard]

Aric Cyr <Aric.Cyr(a)gmail.com> writes:
> Ya, I thought about that after I sent my previous mail as well...  an assert
> would probably be more useful for checking "This".  I also disagree that "This"
> is guaranteed to always be non-NULL.  There really is no way you can force
> policy how a user calls the function, so minimally checking (or aborting) on
> NULL is a sane thing to do.  It doesn't hurt the code, and catches potential
> usage problems.

Not checking at all and crashing works just as well to catch problems,
and doesn't hurt performance. There's no reason to add NULL checks
unless there is a Windows app that depends on it.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Andreas Mohr]

Hi,

On Tue, Jan 17, 2006 at 11:16:42AM +0100, Alexandre Julliard wrote:
> Aric Cyr <Aric.Cyr(a)gmail.com> writes:
> > Ya, I thought about that after I sent my previous mail as well...  an assert
> > would probably be more useful for checking "This".  I also disagree that "This"
> > is guaranteed to always be non-NULL.  There really is no way you can force
> > policy how a user calls the function, so minimally checking (or aborting) on
> > NULL is a sane thing to do.  It doesn't hurt the code, and catches potential
> > usage problems.
>
> Not checking at all and crashing works just as well to catch problems,
> and doesn't hurt performance. There's no reason to add NULL checks
> unless there is a Windows app that depends on it.

Exactly!
"Stupid" NULL pointer checks even actively hurt debugging since in severe
cases you may have a function "properly" (*cough*) failing due to a NULL
pointer check, but then "unfortunately" you notice the effect of this
"properly checked" anomaly "only" 3 layers and 5000 relay log lines later
when something almost entirely unrelated really breaks with a SEGV.
Have fun wasting the time to trace back those 3 layers to the real offender...

Andreas Mohr

-- 
No programming skills!? Why not help translate many Linux applications! 
https://launchpad.ubuntu.com/rosetta

[Aric Cyr]

Andreas Mohr <andi <at> rhlx01.fht-esslingen.de> writes:
> On Tue, Jan 17, 2006 at 11:16:42AM +0100, Alexandre Julliard wrote:
> > Aric Cyr <Aric.Cyr <at> gmail.com> writes:
> > > Ya, I thought about that after I sent my previous mail as well...  an 
> > > assert would probably be more useful for checking "This".
> >
> > Not checking at all and crashing works just as well to catch problems,
> > and doesn't hurt performance. There's no reason to add NULL checks
> > unless there is a Windows app that depends on it.
>
> "Stupid" NULL pointer checks even actively hurt debugging since in severe
> cases you may have a function "properly" (*cough*) failing due to a NULL
> pointer check, but then "unfortunately" you notice the effect of this
> "properly checked" anomaly "only" 3 layers and 5000 relay log lines later
> when something almost entirely unrelated really breaks with a SEGV.
> Have fun wasting the time to trace back those 3 layers to the real offender...

I'd have to (and did) agree that the NULL check for "This" wasn't a great idea,
and thus suggested an assert.  However, as wine does have a built in debugger,
even that would really be unnecessary (as Alexandre pointed out), and crashing
on the access would be just as good.  I personally like asserts for getting
debug info without needing to fire up a debugger since it is not always easy to
reproduce a problem, especially when you aren't expecting any (and please no one
suggest that I always launch all my apps with gdb... ;)

Cheers,
  Aric

[Robert Shearman]

Al Tobey wrote:
> HRESULT  WINAPI  IWineD3DDeviceImpl_GetCreationParameters(IWineD3DDevice *iface, D3DDEVICE_CREATION_PARAMETERS *pParameters) {
>     IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *) iface;
> -
> -    FIXME("(%p) : stub\n", This);
> -    /* Setup some reasonable defaults */
> -    pParameters->AdapterOrdinal = 0; /* always for now */
> -    pParameters->DeviceType = D3DDEVTYPE_HAL; /* always for now */
> -    pParameters->hFocusWindow = 0;
> -    pParameters->BehaviorFlags =0;
> +    pParameters = &This->createParms;
> +    FIXME("(%p) : Partially implemented ... probably needs some checks\n", This);
>     return D3D_OK;
> }

This doesn't do what you think it does. pParameters is a local variable 
to assigning it to something won't affect what the caller sees. You 
probably want to use memcpy.

-- 
Rob Shearman