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

[Michael Stefaniuc]

Hello Joerg,

and addendum inline

Michael Stefaniuc wrote:
> Joerg-Cyril.Hoehle at t-systems.com 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/5b16e6e0fdb35e57a034fd5e6df61765ad11fa71
> 
> 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