Re: windowscodecs: Add support for IMILBitmapSource interface.

2013-05-20 Thread Charles Davis

On May 19, 2013, at 9:14 PM, Dmitry Timoshkov wrote:
 diff --git a/dlls/windowscodecs/bitmap.c b/dlls/windowscodecs/bitmap.c
 index f8904df..646f3a7 100644
 --- a/dlls/windowscodecs/bitmap.c
 +++ b/dlls/windowscodecs/bitmap.c
 @@ -446,6 +471,226 @@ static const IWICBitmapVtbl BitmapImpl_Vtbl = {
 BitmapImpl_SetResolution
 };
 
 +static HRESULT WINAPI IMILBitmapImpl_QueryInterface(IMILBitmapSource *iface, 
 REFIID iid,
 +void **ppv)
 +{
 +BitmapImpl *This = impl_from_IMILBitmapSource(iface);
 +TRACE((%p,%s,%p)\n, iface, debugstr_guid(iid), ppv);
 +
 +if (!ppv) return E_INVALIDARG;
 +
 +if (IsEqualIID(IID_IUnknown, iid) ||
 +IsEqualIID(IID_IMILBitmapSource, iid))
 +{
 +*ppv = This-IMILBitmapSource_iface;
 +IUnknown_AddRef((IUnknown*)*ppv);
 +return S_OK;
 +}
You're violating COM rules here: when you QI an object for an interface 
pointer, the QI must always return the same result no matter what. Here, 
however, you're returning this interface pointer in response to a query for 
IUnknown, when the already-existing QueryInterface method on the IWICBitmap 
interface returns *itself* for IUnknown! Now when you QI for IUnknown on both 
of these interfaces, they won't compare equal. (I suspect you're not convinced. 
After all, native might violate COM rules here, too. I doubt that (see below), 
but it might. So, write a test that QI's for IUnknown from both interfaces, and 
checks if they're equal or not. Then we'll see who's right.)
 diff --git a/dlls/windowscodecs/wincodecs_private.h 
 b/dlls/windowscodecs/wincodecs_private.h
 index dddb327..29f7726 100644
 --- a/dlls/windowscodecs/wincodecs_private.h
 +++ b/dlls/windowscodecs/wincodecs_private.h
 @@ -27,6 +27,46 @@ DEFINE_GUID(GUID_WineContainerFormatTga, 
 0x0c44fda1,0xa5c5,0x4298,0x96,0x85,0x47
 
 DEFINE_GUID(GUID_VendorWine, 
 0xddf46da1,0x7dc1,0x404e,0x98,0xf2,0xef,0xa4,0x8d,0xfc,0x95,0x0a);
 
 +DEFINE_GUID(IID_IMILBitmapSource,0x7543696a,0xbc8d,0x46b0,0x5f,0x81,0x8d,0x95,0x72,0x89,0x72,0xbe);
 +#define INTERFACE IMILBitmapSource
 +DECLARE_INTERFACE_(IMILBitmapSource,IUnknown)
 +{
 +/*** IUnknown methods ***/
 +STDMETHOD_(HRESULT,QueryInterface)(THIS_ REFIID,void **) PURE;
You know you can just use the STDMETHOD() macro (no trailing underscore), e.g.:

   STDMETHOD(QueryInterface)(THIS_ REFIID,void **) PURE;

for methods that return an HRESULT, right? Or do you prefer explicitly 
declaring the return type like that, even if it is more typing?
 +STDMETHOD_(ULONG,AddRef)(THIS) PURE;
 +STDMETHOD_(ULONG,Release)(THIS) PURE;
 +/*** IMILBitmapSource methods ***/
 +STDMETHOD_(HRESULT,GetSize)(THIS_ UINT *,UINT *);
 +STDMETHOD_(HRESULT,GetPixelFormat)(THIS_ int *);
 +STDMETHOD_(HRESULT,GetResolution)(THIS_ double *,double *);
 +STDMETHOD_(HRESULT,CopyPalette)(THIS_ IWICPalette *);
 +STDMETHOD_(HRESULT,CopyPixels)(THIS_ const WICRect *,UINT,UINT,BYTE *);
 +STDMETHOD_(HRESULT,UnknownMethod1)(THIS_ void **);
 +};
 +#undef INTERFACE
 +
 +#define INTERFACE IMILUnknown1
 +DECLARE_INTERFACE_(IMILUnknown1,IUnknown)
 +{
 +/*** IUnknown methods ***/
 +STDMETHOD_(HRESULT,QueryInterface)(THIS_ REFIID,void **) PURE;
 +STDMETHOD_(ULONG,AddRef)(THIS) PURE;
 +STDMETHOD_(ULONG,Release)(THIS) PURE;
 +};
 +#undef INTERFACE
Wouldn't that just make this interface a plain old IUnknown? Or does it have 
its own IID? (Or does that not even matter, given that it's returned from that 
UnknownMethod1 above?) If it is nothing more than a plain IUnknown, why did 
you even add this, since it doesn't really extend the IUnknown interface with 
any new methods?

It's telling that its v-table is the very first v-table in the object. This 
probably indicates that this really is the canonical IUnknown (i.e. the one 
that gets returned when you QI for IUnknown), and the other interfaces' 
IUnknown methods just delegate to this one. If I haven't missed my guess about 
this object's QI implementation, then this is the IUnknown that should get 
returned from a QI for IUnknown.

This might actually be an object that supports aggregation; you may want to 
test for that, too ;).

Chip





Re: windowscodecs: Add support for IMILBitmapSource interface.

2013-05-20 Thread Dmitry Timoshkov
Charles Davis cdavi...@gmail.com wrote:

 You're violating COM rules here: when you QI an object for an interface
 pointer, the QI must always return the same result no matter what. Here,
 however, you're returning this interface pointer in response to a query
 for IUnknown, when the already-existing QueryInterface method on the
 IWICBitmap interface returns *itself* for IUnknown! Now when you QI for
 IUnknown on both of these interfaces, they won't compare equal. (I suspect
 you're not convinced. After all, native might violate COM rules here, too.
 I doubt that (see below), but it might. So, write a test that QI's for
 IUnknown from both interfaces, and checks if they're equal or not. Then
 we'll see who's right.)

This patch is a result of investigation done in the bug 33384. Even if it's
probably incomplete and violates COM rules sometimes. It's hard to tell
without additional investigation, join the club at the bug 33384 :)

  +STDMETHOD_(HRESULT,QueryInterface)(THIS_ REFIID,void **) PURE;
 You know you can just use the STDMETHOD() macro (no trailing underscore), 
 e.g.:
 
STDMETHOD(QueryInterface)(THIS_ REFIID,void **) PURE;
 
 for methods that return an HRESULT, right? Or do you prefer explicitly 
 declaring the return type like that, even if it is more typing?

I guess that I prefer the methods to be declared same way even it's more
typing.

  +#define INTERFACE IMILUnknown1
  +DECLARE_INTERFACE_(IMILUnknown1,IUnknown)
  +{
  +/*** IUnknown methods ***/
  +STDMETHOD_(HRESULT,QueryInterface)(THIS_ REFIID,void **) PURE;
  +STDMETHOD_(ULONG,AddRef)(THIS) PURE;
  +STDMETHOD_(ULONG,Release)(THIS) PURE;
  +};
  +#undef INTERFACE
 Wouldn't that just make this interface a plain old IUnknown? Or does it
 have its own IID? (Or does that not even matter, given that it's returned
 from that UnknownMethod1 above?) If it is nothing more than a plain
 IUnknown, why did you even add this, since it doesn't really extend
 the IUnknown interface with any new methods?

Most likely this interface has other methods besides IUnknown ones, but
it's hard to tell due to limited knowledge. Feel free to send a comment
with your findings to the bug 33384.

-- 
Dmitry.




Re: windowscodecs: Add support for IMILBitmapSource interface.

2013-05-19 Thread Marvin
Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?
Full results can be found at
http://testbot.winehq.org/JobDetails.pl?Key=25624

Your paranoid android.


=== WVISTAX64 (32 bit bitmap) ===
bitmap: unhandled exception c005 at 77370897

=== W2K8SE (32 bit bitmap) ===
bitmap: unhandled exception c005 at 778A81CB

=== W7PROX64 (32 bit bitmap) ===
bitmap: unhandled exception c005 at 77E035F2

=== TEST64_W7SP1 (32 bit bitmap) ===
bitmap: unhandled exception c005 at 77593772

=== W7PROX64 (64 bit bitmap) ===
No test summary line found

=== TEST64_W7SP1 (64 bit bitmap) ===
Timeout