[PATCH 5/8] dsound: Make capture behave like native in regards to COM aggregation.

Hi,

Michael Stefaniuc wrote:
> static HRESULT WINAPI IDirectSoundCaptureImpl_CreateCaptureBuffer

+    if (pUnk) {
+        WARN("invalid parameter: pUnk != NULL\n");
+        /* *lplpDSCaptureBuffer = NULL; Not done by native dsound */
+        return DSERR_NOAGGREGATION;
+    }

What's your rationale for doing so?
Bug compatibility with native is fine, however not setting NULL is
problematic before you've reached 100% compatibility, have you?

What I mean is that if there's a situation where Wine's CreateCaptureBuffer
returns some DSERR such as NOTIMPL where native does not, then that's an
unexpected situation for the app which it may not be well prepared to handle,
as it is untestable on native.  Now not zeroing output pointers may make it
even harder for the app to remain in sane state.
Then blame the app or Wine for a crash?

If OTOH you know that the Wine module *always* returns the exact same
values as native, then I've no objection in not zeroing output pointers when
native does neither. Yet you'll have to consider that native may do so in
the future, witness a growing number of MSDN documents covering later
APIs that specifically mention zeroing pointers in case of failure.

IIRC, AJ removed from my mmdevapi tests one snippet that showed that native's GetService
does not zero an output pointer even though MSDN says so -- Wine zeroes. Yet perhaps
I got confused, messed up with patches and submitted the one that did not contain that test
so maybe AJ never had a chance to see it. I'll have to check that.

Regards,
 Jörg Höhle

[PATCH 5/8] dsound: Make capture behave like native in regards to COM aggregation.

[Michael Stefaniuc]

[e-mail] wrote:
> Hi,
>
> Michael Stefaniuc wrote:
> > static HRESULT WINAPI IDirectSoundCaptureImpl_CreateCaptureBuffer
> - if (pUnk) {
> -
>    WARN("invalid parameter: pUnk != NULL\n");
> -
>    /* *lplpDSCaptureBuffer = NULL; Not done by native dsound */
> -
>    return DSERR_NOAGGREGATION;
> - }
>
> What's your rationale for doing so?
> Bug compatibility with native is fine, however not setting NULL is
> problematic before you've reached 100% compatibility, have you?

Not in this case which deals with COM aggregation. dsound doesn't
supports that and it is documented.
> What I mean is that if there's a situation where Wine's CreateCaptureBuffer
> returns some DSERR such as NOTIMPL where native does not, then that's an
> unexpected situation for the app which it may not be well prepared to handle,
> as it is untestable on native.  Now not zeroing output pointers may make it
> even harder for the app to remain in sane state.
> Then blame the app or Wine for a crash?
>
> If OTOH you know that the Wine module *always* returns the exact same
> values as native, then I've no objection in not zeroing output pointers when
> native does neither. Yet you'll have to consider that native may do so in
> the future, witness a growing number of MSDN documents covering later
> APIs that specifically mention zeroing pointers in case of failure.
>
> IIRC, AJ removed from my mmdevapi tests one snippet that showed that native's GetService
> does not zero an output pointer even though MSDN says so -- Wine zeroes. Yet perhaps
> I got confused, messed up with patches and submitted the one that did not contain that test
> so maybe AJ never had a chance to see it. I'll have to check that.

Patch 8/8 has the tests for this. I used the standard COM aggregation
test for classes that do not support COM aggregation. That was designed
by Jacek in
http://source.winehq.org/git/wine.git/commitdiff/5b16e6e0fdb35e57a034fd5e6df...

And the
    ok(!unk, "unk = %p\n", unk);
failed on the Win7 box I used for testing. That test is in because it is
known that Windows has classes that do not follow their own COM
standard, especially when it comes to COM aggregation.

Now i would have had different options how to deal with that.
- Silently drop the test.
- Dropping the test and documenting why it deviates from the standard test.
- Adding the test and marking it todo_wine.
- Add the test and fix the code as the change is trivial.

I picked IMHO the cleanest solution as that matches native 100% and adds
a test for that behavior.

bye
    michael

[PATCH 5/8] dsound: Make capture behave like native in regards to COM aggregation.

[Michael Stefaniuc]

Hello Joerg,

and addendum inline

Michael Stefaniuc wrote:
> [e-mail] wrote:
> > Michael Stefaniuc wrote:
> > > static HRESULT WINAPI IDirectSoundCaptureImpl_CreateCaptureBuffer
> > - if (pUnk) {
> > -
> >    WARN("invalid parameter: pUnk != NULL\n");
> > -
> >    /* *lplpDSCaptureBuffer = NULL; Not done by native dsound */
> > -
> >    return DSERR_NOAGGREGATION;
> > - }
> >
> > What's your rationale for doing so?
> > Bug compatibility with native is fine, however not setting NULL is
> > problematic before you've reached 100% compatibility, have you?
>
> Not in this case which deals with COM aggregation. dsound doesn't
> supports that and it is documented.
> > What I mean is that if there's a situation where Wine's CreateCaptureBuffer
> > returns some DSERR such as NOTIMPL where native does not, then that's an
> > unexpected situation for the app which it may not be well prepared to handle,
> > as it is untestable on native.  Now not zeroing output pointers may make it
> > even harder for the app to remain in sane state.
> > Then blame the app or Wine for a crash?
> >
> > If OTOH you know that the Wine module *always* returns the exact same
> > values as native, then I've no objection in not zeroing output pointers when
> > native does neither. Yet you'll have to consider that native may do so in
> > the future, witness a growing number of MSDN documents covering later
> > APIs that specifically mention zeroing pointers in case of failure.
> >
> > IIRC, AJ removed from my mmdevapi tests one snippet that showed that native's GetService
> > does not zero an output pointer even though MSDN says so -- Wine zeroes. Yet perhaps
> > I got confused, messed up with patches and submitted the one that did not contain that test
> > so maybe AJ never had a chance to see it. I'll have to check that.
>
> Patch 8/8 has the tests for this. I used the standard COM aggregation
> test for classes that do not support COM aggregation. That was designed
> by Jacek in
> http://source.winehq.org/git/wine.git/commitdiff/5b16e6e0fdb35e57a034fd5e6df...
>
> And the
>     ok(!unk, "unk = %p\n", unk);

Although I personally didn't hit that case yet my guess that check isn't
there just to detect the implementation details of the error handling.
It is there to detect if a class pretends to not support COM aggregation
but which still supports it. Just to give their own applications a
"competitive advantage" ...
> failed on the Win7 box I used for testing. That test is in because it is
> known that Windows has classes that do not follow their own COM
> standard, especially when it comes to COM aggregation.
>
> Now i would have had different options how to deal with that.
> - Silently drop the test.
> - Dropping the test and documenting why it deviates from the standard test.
> - Adding the test and marking it todo_wine.
> - Add the test and fix the code as the change is trivial.
>
> I picked IMHO the cleanest solution as that matches native 100% and adds
> a test for that behavior.

bye
    michael

[Alexandre Julliard]

Michael Stefaniuc  writes:
> Now i would have had different options how to deal with that.
> - Silently drop the test.
> - Dropping the test and documenting why it deviates from the standard test.
> - Adding the test and marking it todo_wine.
> - Add the test and fix the code as the change is trivial.
>
> I picked IMHO the cleanest solution as that matches native 100% and adds
> a test for that behavior.

In general, not setting something on error doesn't matter, so it doesn't
need to be tested, unless we find an app that depends on it. If you want
to test and replicate the behavior that's OK, but then you don't need to
add comments like "native does this", that's what the tests are for.