[PATCH 04/11] mfplat: Implement IMFAttributes::{SetItem,GetItem}.

[Jactry Zeng]

Signed-off-by: Jactry Zeng 
---
 dlls/mfplat/main.c            | 142 ++++++++++++++++++++++++++++++++--
 dlls/mfplat/tests/Makefile.in |   2 +-
 dlls/mfplat/tests/mfplat.c    |  69 +++++++++++++++++
 3 files changed, 207 insertions(+), 6 deletions(-)

diff --git a/dlls/mfplat/main.c b/dlls/mfplat/main.c
index c7c346512f..6e056b3609 100644
--- a/dlls/mfplat/main.c
+++ b/dlls/mfplat/main.c
@@ -35,6 +35,7 @@
 #include "wine/heap.h"
 #include "wine/debug.h"
 #include "wine/unicode.h"
+#include "wine/list.h"

WINE_DEFAULT_DEBUG_CHANNEL(mfplat);

@@ -448,10 +449,19 @@ HRESULT WINAPI MFCopyImage(BYTE *dest, LONG deststride, const BYTE *src, LONG sr
     return E_NOTIMPL;
 }

+struct mfattribute
+{
+    struct list entry;
+    GUID key;
+    PROPVARIANT value;
+};
+
 typedef struct _mfattributes
 {
     IMFAttributes IMFAttributes_iface;
     LONG ref;
+    CRITICAL_SECTION lock;
+    struct list attributes;
 } mfattributes;

static inline mfattributes *impl_from_IMFAttributes(IMFAttributes *iface)
@@ -491,6 +501,8 @@ static ULONG WINAPI mfattributes_AddRef(IMFAttributes *iface)
     return ref;
 }

+static void free_attribute_object(mfattributes *object);
+
 static ULONG WINAPI mfattributes_Release(IMFAttributes *iface)
 {
     mfattributes *This = impl_from_IMFAttributes(iface);
@@ -500,19 +512,50 @@ static ULONG WINAPI mfattributes_Release(IMFAttributes *iface)

if (!ref)
     {
+        free_attribute_object(This);
         HeapFree(GetProcessHeap(), 0, This);
     }

return ref;
 }

+static HRESULT mfattributes_getitem(mfattributes *object, REFGUID key, PROPVARIANT *value, BOOL verify_type)
+{
+    struct mfattribute *attribute, *attribute2;
+    HRESULT hres;
+
+    EnterCriticalSection(&object->lock);
+
+    LIST_FOR_EACH_ENTRY_SAFE(attribute, attribute2, &object->attributes, struct mfattribute, entry)
+    {
+        if(IsEqualGUID(key, &attribute->key))
+        {
+            if(verify_type && ((value->vt && attribute->value.vt != value->vt) ||
+                              (value->vt == VT_UNKNOWN && !attribute->value.punkVal)))
+                hres = MF_E_INVALIDTYPE;
+            else
+            {
+                PropVariantCopy(value, &attribute->value);
+                hres = S_OK;
+            }
+            break;
+        }
+        else
+            hres = MF_E_ATTRIBUTENOTFOUND;
+    }
+
+    LeaveCriticalSection(&object->lock);
+
+    return hres;
+}
+
 static HRESULT WINAPI mfattributes_GetItem(IMFAttributes *iface, REFGUID key, PROPVARIANT *value)
 {
     mfattributes *This = impl_from_IMFAttributes(iface);

-    FIXME("%p, %s, %p\n", This, debugstr_guid(key), value);
+    TRACE("(%p, %s, %p)\n", This, debugstr_guid(key), value);

-    return E_NOTIMPL;
+    return mfattributes_getitem(This, key, value, FALSE);
 }

static HRESULT WINAPI mfattributes_GetItemType(IMFAttributes *iface, REFGUID key, MF_ATTRIBUTE_TYPE *type)
@@ -645,13 +688,71 @@ static HRESULT WINAPI mfattributes_GetUnknown(IMFAttributes *iface, REFGUID key,
     return E_NOTIMPL;
 }

-static HRESULT WINAPI mfattributes_SetItem(IMFAttributes *iface, REFGUID key, REFPROPVARIANT Value)
+static HRESULT mfattribute_setitem(mfattributes *object, REFGUID key, REFPROPVARIANT value)
+{
+    struct mfattribute *attribute, *attribute2, *new_attribute = NULL;
+
+    EnterCriticalSection(&object->lock);
+
+    LIST_FOR_EACH_ENTRY_SAFE(attribute, attribute2, &object->attributes, struct mfattribute, entry)
+        if(IsEqualGUID(key, &attribute->key))
+        {
+            new_attribute = attribute;
+            PropVariantClear(&new_attribute->value);
+            break;
+        }
+
+    if(!new_attribute)
+    {
+        new_attribute = heap_alloc(sizeof(struct mfattribute));
+        if(!new_attribute)
+        {
+            LeaveCriticalSection(&object->lock);
+            return E_OUTOFMEMORY;
+        }
+        new_attribute->key = *key;
+        list_add_tail(&object->attributes, &new_attribute->entry);
+    }
+    PropVariantCopy(&new_attribute->value, value);
+
+    LeaveCriticalSection(&object->lock);
+
+    return S_OK;
+ }
+
+static BOOL is_type_valid(DWORD type)
+{
+    switch(type)
+    {
+        case MF_ATTRIBUTE_UINT32:
+        case MF_ATTRIBUTE_UINT64:
+        case MF_ATTRIBUTE_DOUBLE:
+        case MF_ATTRIBUTE_GUID:
+        case MF_ATTRIBUTE_STRING:
+        case MF_ATTRIBUTE_BLOB:
+        case MF_ATTRIBUTE_IUNKNOWN:
+            return TRUE;
+        default:
+            return FALSE;
+    }
+}
+
+static HRESULT WINAPI mfattributes_SetItem(IMFAttributes *iface, REFGUID key, REFPROPVARIANT value)
 {
     mfattributes *This = impl_from_IMFAttributes(iface);

-    FIXME("%p, %s, %p\n", This, debugstr_guid(key), Value);
+    TRACE("(%p, %s, %p)\n", This, debugstr_guid(key), value);

-    return E_NOTIMPL;
+    if(!is_type_valid(value->vt))
+    {
+        PROPVARIANT empty_value;
+
+        PropVariantClear(&empty_value);
+        mfattribute_setitem(This, key, &empty_value);
+        return MF_E_INVALIDTYPE;
+    }
+    else
+        return mfattribute_setitem(This, key, value);
 }

static HRESULT WINAPI mfattributes_DeleteItem(IMFAttributes *iface, REFGUID key)
@@ -824,6 +925,31 @@ static void init_attribute_object(mfattributes *object, UINT32 size)
 {
     object->ref = 1;
     object->IMFAttributes_iface.lpVtbl = &mfattributes_vtbl;
+    InitializeCriticalSection(&object->lock);
+    object->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": IMFAttributes.lock");
+    list_init(&object->attributes);
+}
+
+static void free_attribute_object(mfattributes *object)
+{
+    EnterCriticalSection(&object->lock);
+
+    if (!list_empty(&object->attributes))
+    {
+        struct mfattribute *attribute, *attribute2;
+
+        LIST_FOR_EACH_ENTRY_SAFE(attribute, attribute2, &object->attributes, struct mfattribute, entry)
+        {
+            PropVariantClear(&attribute->value);
+            list_remove(&attribute->entry);
+            heap_free(attribute);
+        }
+    }
+
+    LeaveCriticalSection(&object->lock);
+
+    object->lock.DebugInfo->Spare[0] = 0;
+    DeleteCriticalSection(&object->lock);
 }

/***********************************************************************
@@ -901,6 +1027,7 @@ static ULONG WINAPI mfbytestream_Release(IMFByteStream *iface)

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         HeapFree(GetProcessHeap(), 0, This);
     }

@@ -1368,6 +1495,7 @@ static ULONG WINAPI mfpresentationdescriptor_Release(IMFPresentationDescriptor *

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         HeapFree(GetProcessHeap(), 0, This);
     }

@@ -2064,6 +2192,7 @@ static ULONG WINAPI mediatype_Release(IMFMediaType *iface)

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         HeapFree(GetProcessHeap(), 0, This);
     }

@@ -2426,6 +2555,7 @@ static ULONG WINAPI mfmediaevent_Release(IMFMediaEvent *iface)

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         HeapFree(GetProcessHeap(), 0, This);
     }

@@ -2940,6 +3070,7 @@ static ULONG WINAPI mfdescriptor_Release(IMFStreamDescriptor *iface)

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         heap_free(This);
     }

@@ -3437,6 +3568,7 @@ static ULONG WINAPI mfsample_Release(IMFSample *iface)

if (!ref)
     {
+        free_attribute_object(&This->attributes);
         heap_free(This);
     }

diff --git a/dlls/mfplat/tests/Makefile.in b/dlls/mfplat/tests/Makefile.in
index 07cf328ad2..e80a3b7cfc 100644
--- a/dlls/mfplat/tests/Makefile.in
+++ b/dlls/mfplat/tests/Makefile.in
@@ -1,5 +1,5 @@
 TESTDLL   = mfplat.dll
-IMPORTS   = ole32 mfplat
+IMPORTS   = ole32 mfplat propsys

C_SRCS = \
    mfplat.c
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c
index 3fa59f4816..fff424e798 100644
--- a/dlls/mfplat/tests/mfplat.c
+++ b/dlls/mfplat/tests/mfplat.c
@@ -33,6 +33,7 @@
 #include "mfidl.h"
 #include "mferror.h"
 #include "mfreadwrite.h"
+#include "propvarutil.h"

#include "wine/test.h"

@@ -693,6 +694,73 @@ static void test_MFSample(void)
     IMFSample_Release(sample);
 }

+static void test_IMFAttributes_item(void)
+{
+    IMFAttributes *attributes;
+    HRESULT hr;
+    PROPVARIANT propvar, ret_propvar;
+
+    hr = MFCreateAttributes(&attributes, 2);
+    ok(hr == S_OK, "MFCreateAttributes failed: 0x%08x.\n", hr);
+
+    PropVariantInit(&propvar);
+    propvar.vt = MF_ATTRIBUTE_UINT32;
+    U(propvar).ulVal = 123;
+    hr = IMFAttributes_SetItem(attributes, &DUMMY_GUID1, &propvar);
+    ok(hr == S_OK, "IMFAttributes_SetItem failed: 0x%08x.\n", hr);
+    PropVariantInit(&ret_propvar);
+    ret_propvar.vt = MF_ATTRIBUTE_UINT32;
+    U(ret_propvar).ulVal = 0xdeadbeef;
+    hr = IMFAttributes_GetItem(attributes, &DUMMY_GUID1, &ret_propvar);
+    ok(hr == S_OK, "IMFAttributes_GetItem failed: 0x%08x.\n", hr);
+    ok(!PropVariantCompareEx(&propvar, &ret_propvar, 0, 0), "Returned wrong property: (vt: %x, ulVal: %d).\n",
+       ret_propvar.vt, U(ret_propvar).ulVal);
+    PropVariantClear(&ret_propvar);
+
+    PropVariantInit(&ret_propvar);
+    ret_propvar.vt = MF_ATTRIBUTE_STRING;
+    U(ret_propvar).pwszVal = NULL;
+    hr = IMFAttributes_GetItem(attributes, &DUMMY_GUID1, &ret_propvar);
+    ok(hr == S_OK, "IMFAttributes_GetItem failed: 0x%08x.\n", hr);
+    ok(!PropVariantCompareEx(&propvar, &ret_propvar, 0, 0), "Returned wrong property: (vt: %x, ulVal: %d).\n",
+       ret_propvar.vt, U(ret_propvar).ulVal);
+    PropVariantClear(&ret_propvar);
+
+    PropVariantClear(&propvar);
+
+    PropVariantInit(&propvar);
+    propvar.vt = MF_ATTRIBUTE_UINT64;
+    U(propvar).uhVal.QuadPart = 65536;
+    hr = IMFAttributes_SetItem(attributes, &DUMMY_GUID1, &propvar);
+    ok(hr == S_OK, "IMFAttributes_SetItem failed: 0x%08x.\n", hr);
+    PropVariantInit(&ret_propvar);
+    ret_propvar.vt = MF_ATTRIBUTE_UINT32;
+    U(ret_propvar).ulVal = 0xdeadbeef;
+    hr = IMFAttributes_GetItem(attributes, &DUMMY_GUID1, &ret_propvar);
+    ok(hr == S_OK, "IMFAttributes_GetItem failed: 0x%08x.\n", hr);
+    ok(!PropVariantCompareEx(&propvar, &ret_propvar, 0, 0), "Returned wrong property: (vt: %x, ulVal: %d).\n",
+       ret_propvar.vt, U(ret_propvar).ulVal);
+    PropVariantClear(&ret_propvar);
+    PropVariantClear(&propvar);
+
+    PropVariantInit(&propvar);
+    propvar.vt = VT_I4;
+    U(propvar).lVal = 123;
+    hr = IMFAttributes_SetItem(attributes, &DUMMY_GUID2, &propvar);
+    ok(hr == MF_E_INVALIDTYPE, "IMFAttributes_SetItem should failed: 0x%08x.\n", hr);
+    PropVariantInit(&ret_propvar);
+    ret_propvar.vt = MF_ATTRIBUTE_UINT32;
+    U(ret_propvar).lVal = 0xdeadbeef;
+    hr = IMFAttributes_GetItem(attributes, &DUMMY_GUID2, &ret_propvar);
+    ok(hr == S_OK, "IMFAttributes_GetItem failed: 0x%08x.\n", hr);
+    PropVariantClear(&propvar);
+    ok(!PropVariantCompareEx(&propvar, &ret_propvar, 0, 0), "Returned wrong property: (vt: %x, ulVal: %d).\n",
+       ret_propvar.vt, U(ret_propvar).ulVal);
+    PropVariantClear(&ret_propvar);
+
+    IMFAttributes_Release(attributes);
+}
+
 START_TEST(mfplat)
 {
     CoInitialize(NULL);
@@ -708,6 +776,7 @@ START_TEST(mfplat)
     test_MFCreateMFByteStreamOnStream();
     test_MFCreateMemoryBuffer();
     test_source_resolver();
+    test_IMFAttributes_item();

CoUninitialize();
 }

[PATCH 04/11] mfplat: Implement IMFAttributes::{SetItem, GetItem}.

[Nikolay Sivov]

I don't think you need SAFE iterators for Get/Set.
> - EnterCriticalSection(&object->lock);
> -
> - if (!list_empty(&object->attributes))
> - {
> -
>    struct mfattribute *attribute, *attribute2;
> -
> -
>    LIST_FOR_EACH_ENTRY_SAFE(attribute, attribute2, &object->attributes, struct mfattribute, entry)
> -
>    {
> -
>        PropVariantClear(&attribute->value);
> -
>        list_remove(&attribute->entry);
> -
>        heap_free(attribute);
> -
>    }
> - }
> -
> - LeaveCriticalSection(&object->lock);
> -
> - object->lock.DebugInfo->Spare[0] = 0;
> - DeleteCriticalSection(&object->lock);

Empty list check is redundant.

[PATCH 04/11] mfplat: Implement IMFAttributes::{SetItem, GetItem}.

[Nikolay Sivov]

On 12/28/18 6:42 PM, Jactry Zeng wrote:
> +struct mfattribute
> +{
> - struct list entry;
> - GUID key;
> - PROPVARIANT value;
>
> +};
> - typedef struct _mfattributes
> {
>     IMFAttributes IMFAttributes_iface;
>     LONG ref;
> - CRITICAL_SECTION lock;
> - struct list attributes;
> } mfattributes;

Also GetItemByIndex() suggests array would be more appropriate.

[PATCH 04/11] mfplat: Implement IMFAttributes::{SetItem, GetItem}.

[Jactry Zeng]

Hi Nikolay,

Thanks for review!

Nikolay Sivov  于2018年12月29日周六 上午1:09写道：
> On 12/28/18 6:42 PM, Jactry Zeng wrote:
> > +struct mfattribute
> > +{
> > - struct list entry;
> > - GUID key;
> > - PROPVARIANT value;
> >
> > +};
> > - typedef struct _mfattributes
> > {
> >     IMFAttributes IMFAttributes_iface;
> >     LONG ref;
> > - CRITICAL_SECTION lock;
> > - struct list attributes;
> > } mfattributes;
>
> Also GetItemByIndex() suggests array would be more appropriate.

Please correct me if I misunderstand anything.
Since it has IMFAttributes::DeleteItem which can delete any attribute
from that list, it seems that list can make thing more simple?
Or it will need to reorder the array every time an attribute is deleted.

[PATCH 04/11] mfplat: Implement IMFAttributes::{SetItem, GetItem}.

[Nikolay Sivov]

On 12/29/18 6:50 PM, Jactry Zeng wrote:
> Hi Nikolay,
>
> Thanks for review!
>
> Nikolay Sivov  于2018年12月29日周六 上午1:09写道：
> > On 12/28/18 6:42 PM, Jactry Zeng wrote:
> > > +struct mfattribute
> > > +{
> > > - struct list entry;
> > > - GUID key;
> > > - PROPVARIANT value;
> > >
> > > +};
> > > - typedef struct _mfattributes
> > >  {
> > >      IMFAttributes IMFAttributes_iface;
> > >      LONG ref;
> > > - CRITICAL_SECTION lock;
> > > - struct list attributes;
> > > } mfattributes;
> >
> > Also GetItemByIndex() suggests array would be more appropriate.
>
> Please correct me if I misunderstand anything.
> Since it has IMFAttributes::DeleteItem which can delete any attribute
> from that list, it seems that list can make thing more simple?
> Or it will need to reorder the array every time an attribute is deleted.

Ideally it should be a hash table I suppose, additionally indexed to 
make GetItemByIndex() work, assuming it maintains a stable order when 
you add/remove items.

If it's not stable and then it would be just a hash table, choosing 
between list and array in this case depends on GetItemByIndex() 
behavior, and "normal" use case, e.g. if applications don't normally 
remove attributes, and index is stable then array would be better.