[PATCH 3/6] mfreadwrite/tests: Add some audio media type attributes tests.

[Rémi Bernon]

On 11/9/21 21:50, Nikolay Sivov wrote:
> 
> 
> On 11/8/21 5:08 PM, Rémi Bernon wrote:
>> +    for (i = 0; i < count; i++)
>> +    {
>> +        PropVariantInit(&value);
>> +        hr = IMFMediaType_GetItemByIndex(media_type, i, &key, &value);
>> +        ok(hr == S_OK, "GetItemByIndex returned hr %#x\n", hr);
>> +
>> +        for (j = 0; expect->items[j].key; j++) if (IsEqualGUID(expect->items[j].key, &key)) break;
>> +        if (!expect->items[j].key)
>> +        {
>> +            todo_wine_if(expect->todo_spurious > spurious_count)
>> +            ok(0, "spurious attribute %s\n", debugstr_guid(&key));
>> +            spurious_count++;
>> +            continue;
>> +        }
> So this basically ignores "extra" attributes a type might have, that are
> not accounted for in "expect"?
> A rather arbitrary number of attributes we don't know about will be ignored.
> 

No, "expect" should contain the exact list of attributes native MF type 
has, anything extra will cause a test failure.

On Wine, there's some types with attributes they should not have (either 
for instance channel mask, or because we report uncompressed native 
types where native report compressed). In order to keep things simpler 
we just leave a given number of spurious attrs, marked as todo.

The todo and spurious_count can be removed once Wine stops reporting 
additional attributes.

> Will it work if you checked for expected attributes only, doing a loop
> over "expect" array,
> and simply checking with CompareItem(expect.key, expect.value) ? This
> way you don't know secondary search,
> and we'll know that all of expected attributes matched.
> 

This would only check the attributes we expect to be present, and not 
attributes that native MF reports but that we aren't looking for yet.

If native starts reporting more attributes, and games start using them, 
they may slip through the tests if we only check the ones we expect.

>> +
>> +        ok(!found[j], "duplicate attribute %s\n", debugstr_guid(&key));
>> +        found[j] = TRUE;
> I don't understand this duplicate detection. If you're iterating once
> through all attributes with zeroed "found[]",
> how is it possible to get duplicated keys?

Well, we're only enumerating items by index, it's just making sure an 
expected item isn't matched twice. Of course it's probably never going 
to happen if the keys are unique.

>> +
>> +        if (!strcmp(winetest_platform, "wine"))
>> +            ok(!expect->items[j].todo_missing, "attribute not missing %s\n", debugstr_guid(&key));
>> +        ok(!PropVariantCompareEx(&value, &expect->items[j].value, 0, 0), "got %s, expected %s.\n",
>> +                debugstr_propvariant(&value), debugstr_propvariant(&expect->items[j].value));
>> +        PropVariantClear(&value);
>> +    }
> Like I meantioned CompareItem() should probably work?
> 

I guess, but then we would ignore anything we don't expect.

I'm actually trying to add these tests to validate that some attributes 
we don't have yet should be present. More specifically, average bytes 
per sec and block alignment, which are actually pretty useless as they 
can be deduced from the other attributes.

As I see it it's the kind of situation where there would be little 
reason to have them in an "expected" list as they aren't really useful, 
but where checking the whole attribute list would have shown that they 
should be present nonetheless.
-- 
Rémi Bernon <rbernon at codeweavers.com>