November 2021 - Wine-Devel - list.winehq.org

Re: [PATCH 4/5] win32u: Move null user driver implementation from user32.

by Rémi Bernon

On 11/9/21 13:55, Jacek Caban wrote: > Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> > --- > dlls/user32/driver.c | 209 +---------------------------- > dlls/win32u/driver.c | 310 ++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 310 insertions(+), 209 deletions(-) > > A little bit related to my previous comment, this patch introduces a reset to "null" graphics driver on driver unload. I think it could also add the same __wine_set_display_driver( &null_driver, WINE_GDI_DRIVER_VERSION ); call when the "null" driver is explicitly requested and loaded, possibly addressing my previous comment too (as the pointer would not be lasy_load_driver anymore). -- Rémi Bernon <rbernon(a)codeweavers.com>

Nov. 9, 2021

Re: [PATCH 2/5] user32: Use user_driver_funcs to expose user driver function from drivers.

by Rémi Bernon

Hi Jacek, On 11/9/21 13:54, Jacek Caban wrote: > Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> > --- > The ultimate plan is for driver to register themselves by a direct Unix > call to win32u. > > dlls/user32/driver.c | 154 +++++++++-------- > dlls/user32/user32.spec | 1 + > dlls/wineandroid.drv/android.h | 27 +++ > dlls/wineandroid.drv/init.c | 132 ++++----------- > dlls/wineandroid.drv/wineandroid.drv.spec | 24 --- > dlls/winemac.drv/gdi.c | 148 ++++++----------- > dlls/winemac.drv/macdrv.h | 52 ++++++ > dlls/winemac.drv/mouse.c | 2 +- > dlls/winemac.drv/winemac.drv.spec | 39 ----- > dlls/winex11.drv/display.c | 1 - > dlls/winex11.drv/event.c | 2 - > dlls/winex11.drv/init.c | 194 +++++++++++----------- > dlls/winex11.drv/winex11.drv.spec | 41 ----- > dlls/winex11.drv/x11drv.h | 50 ++++++ > dlls/winex11.drv/x11drv_main.c | 1 + > dlls/winex11.drv/xim.c | 2 - > include/wine/gdi_driver.h | 2 + > 17 files changed, 393 insertions(+), 479 deletions(-) > > I think this patch changes the behavior with "null" graphics driver: as it doesn't register itself, the newly added USER_Driver == &lazy_load_driver condition now always pass and it now registers nodrv_CreateWindow for visible winstation. Currently when "null" graphics driver is explicitly requested, it uses nulldrv_CreateWindow, which succeeds creating windows (invisible of course). I think it'd be nice to keep it, and the check could be added with the LoadLibraryW call instead. Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>

Nov. 9, 2021

Re: [PATCH v5 2/2] mshtml: Populate the element props properly.

by Marvin

Hi, While running your changed tests, 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: https://testbot.winehq.org/JobDetails.pl?Key=101582 Your paranoid android. === w8adm (32 bit report) === mshtml: events.c:1089: Test failed: unexpected call img_onerror events: Timeout === w8 (32 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS === w8adm (32 bit report) === mshtml: htmldoc.c:3084: Test failed: Incorrect error code: -2146697211 htmldoc.c:3089: Test failed: Page address: L"http://test.winehq.org/tests/winehq_snapshot/" htmldoc.c:5861: Test failed: expected OnChanged_1012 htmldoc.c:5862: Test failed: expected Exec_HTTPEQUIV htmldoc.c:5864: Test failed: expected Exec_SETTITLE htmldoc.c:5905: Test failed: expected FireNavigateComplete2 === w7u_adm (32 bit report) === mshtml: script.c:624: Test failed: L"/index.html?es5.js:date_now: unexpected Date.now() result 1636498770036 expected 1636498770099"

Nov. 9, 2021

[PATCH] winecfg: Add the command line options to the man page.

by Floris Renaud

Signed-off-by: Floris Renaud <jkfloris(a)dds.nl> --- programs/winecfg/winecfg.man.in | 48 +++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/programs/winecfg/winecfg.man.in b/programs/winecfg/winecfg.man.in index 522d2a0e812..2d2e89dfc9b 100644 --- a/programs/winecfg/winecfg.man.in +++ b/programs/winecfg/winecfg.man.in @@ -1,14 +1,58 @@ -.TH WINECFG 1 "November 2010" "@PACKAGE_STRING@" "Wine Programs" +.TH WINECFG 1 "November 2021" "@PACKAGE_STRING@" "Wine Programs" .SH NAME winecfg \- Wine Configuration Editor .SH SYNOPSIS -.BR "winecfg" +.B winecfg +.RI [ OPTION ] .SH DESCRIPTION .B winecfg is the Wine configuration editor. It allows you to change several settings, such as DLL load order (native versus builtin), enable a virtual desktop, setup disk drives, and change the Wine audio driver, among others. Many of these settings can be made on a per application basis, for example, preferring native riched20.dll for wordpad.exe, but not for notepad.exe. +.SH OPTIONS +.RS +.IP "\fB[no option]\fR" 16 +Launch the graphical version of this program. +.IP "\fB/v\fR, \fB-v\fR" +Display the current global Windows version. +.IP "\fB/v \fIversion\fR, \fB-v \fIversion\fR" +Set global Windows version to +.IR version . +.IP "\fB/?\fR, \fB-?\fR" +Display valid versions for +.I version +and available options. +.RE +.PP +Valid versions for +.I version +are: +.TS +l l r +--- +l l r. +version Mimic Windows version WINEARCH +win10 Windows 10 Ultimate N, Build 17763 win32 & win64 +win81 Windows 8.1 Ultimate N, Build 9600 win32 & win64 +win8 Windows 8 Ultimate N, Build 9200 win32 & win64 +win2008r2 Windows Server 2008 R2 Standard, Build 7601, Service Pack 1 win32 & win64 +win7 Windows 7 Ultimate N, Build 7601, Service Pack 1 win32 & win64 +win2008 Windows Server 2008 Standard, Build 6002, Service Pack 2 win32 & win64 +vista Windows Vista Ultimate N, Build 6002, Service Pack 2 win32 & win64 +win2003 Windows Server 2003 Standard Edition, Build 3790, Service Pack 2 win32 & win64 +winxp64 Windows XP Professional, Build 3790, Service Pack 2 win64 +winxp Windows XP Professional, Build 2600, Service Pack 3 win32 +win2k Windows 2000 Professional, Build 2195, Service Pack 4 win32 +winme Windows ME, V4.90, Build 3000 win32 +win98 Windows 98 Second Edition, V4.10, Build 2222 win32 +win95 Windows 95, V4.00, Build 950 win32 +nt40 Windows NT Workstation, V4.0, Build 1381, Service Pack 6a win32 +nt351 Windows NT Workstation, V3.51, Build 1057, Service pack 5 win32 +win31 Windows 3.1 win32 +win30 Windows 3.0 win32 +win20 Windows 2.0 win32 +.TE .SH BUGS Bugs can be reported on the .UR https://bugs.winehq.org -- 2.33.1

Nov. 9, 2021

[PATCH] po: Update Dutch translation

by Floris Renaud

Signed-off-by: Floris Renaud <jkfloris(a)dds.nl> --- po/nl.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/nl.po b/po/nl.po index cfc51dc296f..c9be849321c 100644 --- a/po/nl.po +++ b/po/nl.po @@ -5,7 +5,7 @@ msgstr "" "Project-Id-Version: Wine\n" "Report-Msgid-Bugs-To: https://bugs.winehq.org\n" "POT-Creation-Date: N/A\n" -"PO-Revision-Date: 2021-09-25 22:40+0200\n" +"PO-Revision-Date: 2021-11-10 00:03+0100\n" "Last-Translator: Floris Renaud <jkfloris(a)dds.nl>\n" "Language-Team: Dutch\n" "Language: nl\n" @@ -14442,7 +14442,7 @@ msgstr "IPv6-adres" #: programs/ipconfig/ipconfig.rc:38 msgid "Primary DNS suffix" -msgstr "" +msgstr "Primair DNS-achtervoegsel" #: programs/msinfo32/msinfo32.rc:28 msgid "System Information" -- 2.33.1

Nov. 9, 2021

[PATCH] combase: add stub for RoOriginateError

by Louis Lenders

https://bugs.winehq.org/show_bug.cgi?id=51983 Signed-off-by: Louis Lenders <xerox.xerox2000x(a)gmail.com> --- .../api-ms-win-core-winrt-error-l1-1-0.spec | 2 +- .../api-ms-win-core-winrt-error-l1-1-1.spec | 2 +- dlls/combase/combase.spec | 2 +- dlls/combase/roapi.c | 9 +++++++++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/dlls/api-ms-win-core-winrt-error-l1-1-0/api-ms-win-core-winrt-error-l1-1-0.spec b/dlls/api-ms-win-core-winrt-error-l1-1-0/api-ms-win-core-winrt-error-l1-1-0.spec index 99f1ca357cf..cf410136631 100644 --- a/dlls/api-ms-win-core-winrt-error-l1-1-0/api-ms-win-core-winrt-error-l1-1-0.spec +++ b/dlls/api-ms-win-core-winrt-error-l1-1-0/api-ms-win-core-winrt-error-l1-1-0.spec @@ -2,7 +2,7 @@ @ stub RoCaptureErrorContext @ stub RoFailFastWithErrorContext @ stub RoGetErrorReportingFlags -@ stub RoOriginateError +@ stdcall RoOriginateError(long ptr) combase.RoOriginateError @ stub RoOriginateErrorW @ stub RoResolveRestrictedErrorInfoReference @ stub RoSetErrorReportingFlags diff --git a/dlls/api-ms-win-core-winrt-error-l1-1-1/api-ms-win-core-winrt-error-l1-1-1.spec b/dlls/api-ms-win-core-winrt-error-l1-1-1/api-ms-win-core-winrt-error-l1-1-1.spec index 0b390f1f80a..d969540e94d 100644 --- a/dlls/api-ms-win-core-winrt-error-l1-1-1/api-ms-win-core-winrt-error-l1-1-1.spec +++ b/dlls/api-ms-win-core-winrt-error-l1-1-1/api-ms-win-core-winrt-error-l1-1-1.spec @@ -7,7 +7,7 @@ @ stub RoGetMatchingRestrictedErrorInfo @ stub RoInspectCapturedStackBackTrace @ stub RoInspectThreadErrorInfo -@ stub RoOriginateError +@ stdcall RoOriginateError(long ptr) combase.RoOriginateError @ stub RoOriginateErrorW @ stdcall RoOriginateLanguageException(long ptr ptr) combase.RoOriginateLanguageException @ stub RoReportFailedDelegate diff --git a/dlls/combase/combase.spec b/dlls/combase/combase.spec index 7343da6a0ef..c2fafacdea3 100644 --- a/dlls/combase/combase.spec +++ b/dlls/combase/combase.spec @@ -301,7 +301,7 @@ @ stdcall RoInitialize(long) @ stub RoInspectCapturedStackBackTrace @ stub RoInspectThreadErrorInfo -@ stub RoOriginateError +@ stdcall RoOriginateError(long ptr) @ stub RoOriginateErrorW @ stdcall RoOriginateLanguageException(long ptr ptr) @ stub RoParameterizedTypeExtraGetTypeSignature diff --git a/dlls/combase/roapi.c b/dlls/combase/roapi.c index 53da979d681..7d871fb885c 100644 --- a/dlls/combase/roapi.c +++ b/dlls/combase/roapi.c @@ -290,6 +290,15 @@ BOOL WINAPI RoOriginateLanguageException(HRESULT error, HSTRING message, IUnknow return FALSE; } +/*********************************************************************** + * RoOriginateError (combase.@) + */ +BOOL WINAPI RoOriginateError(HRESULT error, HSTRING message) +{ + FIXME("(%x %s) stub\n", error, debugstr_hstring(message)); + return FALSE; +} + /*********************************************************************** * CleanupTlsOleState (combase.@) */ -- 2.33.1

Nov. 9, 2021

Re: [PATCH vkd3d 5/5] vkd3d-shader/hlsl: Remove trivial swizzles.

by Zebediah Figura (she/her)

This patch looks good, and is valuable on its own, but it depends on the previous ones...

Nov. 9, 2021

Re: [PATCH vkd3d 2/5] vkd3d-shader/hlsl: Perform a copy propagation pass.

by Zebediah Figura

On 11/9/21 3:44 AM, Giovanni Mascellani wrote: > Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> > --- > Pretty sure this (and the following ones) will require some more > tweaking before being accepted! :-) > > libs/vkd3d-shader/hlsl_codegen.c | 254 ++++++++++++++++++++++++++++++- > 1 file changed, 253 insertions(+), 1 deletion(-) > Overall this patch looks a lot better than I was afraid of. It's a lot of code that's intimidating to review, but once you ignore the rbtree boilerplate it's simple enough and seems about in line with what I expect. There's quite a few things that I think can be simplified, but the basic structure seems sound, so nice work. The CF part is probably going to be a lot trickier, but fortunately I think that this patch alone is the one that really matters. Frankly, if we were to take this patch, and then a second patch that does copy-prop on interior CF blocks with a fresh copy_propagation_state(), we might even cover enough that it's not even worrying about CF... > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c > index 0ee8ab55..8db8bfc9 100644 > --- a/libs/vkd3d-shader/hlsl_codegen.c > +++ b/libs/vkd3d-shader/hlsl_codegen.c > @@ -237,6 +237,253 @@ static void replace_node(struct hlsl_ir_node *old, struct hlsl_ir_node *new) > hlsl_free_instr(old); > } > > +/* struct copy_propagation_state represents the accumulated knowledge > + * of the copy propagation pass while it scans through the code. Field > + * "variables" is a tree whose elements have type struct > + * copy_propagation_varible, and represent each of the variables the > + * pass has already encountered (except those with special semantics, > + * which are ignored). For each variable, the array "values" (whose > + * length is the register size of the variable) represent which node > + * and which index inside that node (i.e., which of the at most four > + * entries of a vector) provided that value last time. Field "node" > + * can be NULL, meaning that the pass was not able to statically > + * determine the node. > + */ This comment feels a bit too low-level? I dunno, comments like this are hard to review, but my inclination is to describe what you're doing at a high level, along the lines of "we track the last known value of each component of each variable", and let the code itself explain the details. > + > +struct copy_propagation_value > +{ > + struct hlsl_ir_node *node; > + unsigned int index; I think the term we want is "component", not "index". > +}; > + > +struct copy_propagation_variable > +{ > + struct rb_entry entry; > + struct hlsl_ir_var *var; > + struct copy_propagation_value *values; > +}; > + > +struct copy_propagation_state > +{ > + struct rb_tree variables; > +}; > + > +static int copy_propagation_variable_compare(const void *key, const struct rb_entry *entry) > +{ > + struct copy_propagation_variable *variable = RB_ENTRY_VALUE(entry, struct copy_propagation_variable, entry); > + uintptr_t key_int = (uintptr_t)key, entry_int = (uintptr_t)variable->var; > + > + if (key_int < entry_int) > + return -1; > + else if (key_int > entry_int) > + return 1; > + else > + return 0; Could we just modify the rbtree implementation to use uintptr_t instead, and then do a direct subtraction? > +} > + > +static void copy_propagation_variable_destroy(struct rb_entry *entry, void *context) > +{ > + struct copy_propagation_variable *variable = RB_ENTRY_VALUE(entry, struct copy_propagation_variable, entry); > + > + vkd3d_free(variable); > +} > + > +static struct copy_propagation_variable *copy_propagation_get_variable(struct hlsl_ctx *ctx, > + struct copy_propagation_state *state, struct hlsl_ir_var *var) > +{ > + struct rb_entry *entry = rb_get(&state->variables, var); > + struct copy_propagation_variable *variable; > + int res; > + > + if (entry) > + return RB_ENTRY_VALUE(entry, struct copy_propagation_variable, entry); > + > + variable = hlsl_alloc(ctx, sizeof(*variable)); > + if (!variable) > + return NULL; > + > + variable->var = var; > + variable->values = hlsl_alloc(ctx, sizeof(*variable->values) * var->data_type->reg_size); > + if (!variable->values) > + { > + vkd3d_free(variable); > + return NULL; > + } > + > + res = rb_put(&state->variables, var, &variable->entry); > + assert(!res); Although this is a bit awkward because most of the function is only relevant for stores, not loads, and I think it would actually be better to reflect that in the code, one way or another (so we can do less effort). > + > + return variable; > +} > + > +static void copy_propagation_set_value(struct copy_propagation_variable *variable, unsigned int offset, > + unsigned char writemask, struct hlsl_ir_node *node) > +{ > + unsigned int index; > + > + for (index = 0; index < 4; ++index) > + { > + if (writemask & (1u << index)) > + { > + if (TRACE_ON()) > + { > + char buf[32]; > + if (!node) > + sprintf(buf, "(nil)"); Can that happen? > + else if (node->index) > + sprintf(buf, "@%u", node->index); > + else > + sprintf(buf, "%p", node); Can that happen? > + TRACE("variable %s[%d] is written by %p[%d]\n", variable->var->name, offset + index, buf, index); This trace doesn't really match the usual format, but more saliently, the hardcoded buffer is ugly. Assuming the other two cases really can happen, I'd rather see individual traces spelled out. Same thing below. > + } > + variable->values[offset + index].node = node; > + variable->values[offset + index].index = index; > + } > + } > +} > + > +/* Check if locations [offset, offset+count) in variable were all > + * written from the same node. If so return the node the corresponding > + * indices, otherwise return NULL (and undefined indices). */ > +static struct hlsl_ir_node *copy_propagation_reconstruct_node(struct copy_propagation_variable *variable, > + unsigned int offset, unsigned int count, unsigned int indices[4]) "indices" is really just a swizzle; can we return it as such? > +{ > + struct hlsl_ir_node *node = NULL; > + unsigned int i; > + > + assert(offset + count <= variable->var->data_type->reg_size); > + > + for (i = 0; i < count; ++i) > + { > + if (!node) > + node = variable->values[offset + i].node; > + else if (node != variable->values[offset + i].node) > + return NULL; > + indices[i] = variable->values[offset + i].index; > + } > + > + return node; > +} This is really obviously the right thing to do, yet it still confused me a *lot* when trying to review it. That might just be me, but if not, I guess the function name and comment could use work. Explaining *why* the function is necessary would probably help. > + > +static bool copy_propagation_load(struct hlsl_ctx *ctx, struct hlsl_ir_load *load, Can you please try to name functions using a verb? There's three more examples below. > + struct copy_propagation_state *state) > +{ > + struct hlsl_ir_node *node = &load->node, *new_node; > + struct copy_propagation_variable *variable; > + struct hlsl_type *type = node->data_type; > + unsigned int offset, indices[4] = {}; {} isn't portable, unfortunately. > + struct hlsl_deref *src = &load->src; > + struct hlsl_ir_var *var = src->var; > + struct hlsl_ir_swizzle *swizzle; > + DWORD s; > + > + if (var->is_input_semantic || var->is_output_semantic || var->is_uniform) > + return false; For input semantics and uniforms: yeah, but do we need to? The important question is "can we reconstruct a store for this variable", and this should already be false. For output semantics, we should never get here in the first place. > + > + if (type->type != HLSL_CLASS_SCALAR && type->type != HLSL_CLASS_VECTOR) > + return false; > + > + offset = hlsl_offset_from_deref(src); The problem with hlsl_offset_from_deref(), and the reason I probably should have fought against 62b25bc52b in its current form, is that it not only doesn't deal with non-constant offsets, but doesn't really give the caller a way to bail either. We should probably be returning bool from it, and then aborting here. > + > + variable = copy_propagation_get_variable(ctx, state, var); > + if (!variable) > + return false; > + > + new_node = copy_propagation_reconstruct_node(variable, offset, type->dimx, indices); > + > + if (TRACE_ON()) > + { > + char buf[32]; > + if (!new_node) > + sprintf(buf, "(nil)"); Is this useful to trace? > + else if (new_node->index) > + sprintf(buf, "@%u", new_node->index); > + else > + sprintf(buf, "%p", new_node); Can this happen? > + TRACE("load from %s[%d-%d] reconstructed to %s[%d %d %d %d]\n", var->name, offset, > + offset + type->dimx, buf, indices[0], indices[1], indices[2], indices[3]); > + } > + > + if (!new_node) > + return false; > + > + s = indices[0] | indices[1] << 2 | indices[2] << 4 | indices[3] << 6; > + if (!(swizzle = hlsl_new_swizzle(ctx, s, type->dimx, new_node, &node->loc))) > + return false; > + list_add_before(&node->entry, &swizzle->node.entry); > + > + replace_node(node, &swizzle->node); > + > + return true; > +} > + > +static bool copy_propagation_store(struct hlsl_ctx *ctx, struct hlsl_ir_store *store, > + struct copy_propagation_state *state) > +{ > + struct copy_propagation_variable *variable; > + struct hlsl_deref *lhs = &store->lhs; > + struct hlsl_ir_var *var = lhs->var; > + > + if (var->is_input_semantic || var->is_output_semantic || var->is_uniform) > + return false; Same deal as in copy_propagation_load(), but inverted. > + > + variable = copy_propagation_get_variable(ctx, state, var); > + if (!variable) > + return false; > + > + copy_propagation_set_value(variable, hlsl_offset_from_deref(lhs), store->writemask, store->rhs.node); > + > + return false; > +} Shouldn't this function just return void? > + > +static bool copy_propagation_recursive(struct hlsl_ctx *ctx, struct hlsl_block *block, > + struct copy_propagation_state *state) > +{ > + struct hlsl_ir_node *instr, *next; > + bool progress = false; > + > + LIST_FOR_EACH_ENTRY_SAFE(instr, next, &block->instrs, struct hlsl_ir_node, entry) > + { > + switch (instr->type) > + { > + case HLSL_IR_LOAD: > + progress |= copy_propagation_load(ctx, hlsl_ir_load(instr), state); > + break; > + > + case HLSL_IR_STORE: > + progress |= copy_propagation_store(ctx, hlsl_ir_store(instr), state); > + break; > + > + case HLSL_IR_IF: > + FIXME("Copy propagation doesn't support conditionals yet, leaving.\n"); > + return progress; > + > + case HLSL_IR_LOOP: > + FIXME("Copy propagation doesn't support loops yet, leaving.\n"); > + return progress; > + > + default: > + break; > + } > + } > + > + return progress; > +} > + > +static bool copy_propagation_pass(struct hlsl_ctx *ctx, struct hlsl_block *block) > +{ > + struct copy_propagation_state state; > + bool progress; > + > + rb_init(&state.variables, copy_propagation_variable_compare); > + > + progress = copy_propagation_recursive(ctx, block, &state); > + > + rb_destroy(&state.variables, copy_propagation_variable_destroy, NULL); > + > + return progress; > +} > + > static bool is_vec1(const struct hlsl_type *type) > { > return (type->type == HLSL_CLASS_SCALAR) || (type->type == HLSL_CLASS_VECTOR && type->dimx == 1); > @@ -1354,7 +1601,12 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun > progress |= transform_ir(ctx, split_struct_copies, body, NULL); > } > while (progress); > - while (transform_ir(ctx, fold_constants, body, NULL)); > + do > + { > + progress = transform_ir(ctx, fold_constants, body, NULL); > + progress |= copy_propagation_pass(ctx, body); This probably isn't worth examining, but I'm curious why constant folding is useful after copy-prop. > + } > + while (progress); > > if (ctx->profile->major_version < 4) > transform_ir(ctx, lower_division, body, NULL); >

Nov. 9, 2021

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

by Rémi Bernon

On 11/9/21 21:53, Nikolay Sivov wrote: > > > On 11/8/21 5:08 PM, Rémi Bernon wrote: >> + static const struct media_type_desc audio_44100_s8_desc = >> + { >> + .items = >> + { >> + {.key = &MF_MT_AUDIO_AVG_BYTES_PER_SECOND, .value = {.vt = VT_UI4, .ulVal = 44100}, .todo_missing = TRUE}, >> + {.key = &MF_MT_AUDIO_BLOCK_ALIGNMENT, .value = {.vt = VT_UI4, .ulVal = 1}, .todo_missing = TRUE}, >> + {.key = &MF_MT_AUDIO_NUM_CHANNELS, .value = {.vt = VT_UI4, .ulVal = 1}}, >> + {.key = &MF_MT_MAJOR_TYPE, .value = {.vt = VT_CLSID, .puuid = (GUID *)&MFMediaType_Audio}}, >> + {.key = &MF_MT_AUDIO_SAMPLES_PER_SECOND, .value = {.vt = VT_UI4, .ulVal = 44100}}, >> + {.key = &MF_MT_AUDIO_PREFER_WAVEFORMATEX, .value = {.vt = VT_UI4, .ulVal = 1}, .todo_missing = TRUE}, >> + {.key = &MF_MT_ALL_SAMPLES_INDEPENDENT, .value = {.vt = VT_UI4, .ulVal = 1}}, >> + {.key = &MF_MT_AUDIO_BITS_PER_SAMPLE, .value = {.vt = VT_UI4, .ulVal = 8}}, >> + {.key = &MF_MT_SUBTYPE, .value = {.vt = VT_CLSID, .puuid = (GUID *)&MFAudioFormat_PCM}}, >> + }, >> + .todo_spurious = 1, >> + }; > When you explicitly checking audio types, you can drop MAJOR_TYPE, > PREFER_WAVEFORMATEX, ALL_SAMPLES_INDEPENDENT, or maybe even subtype from > static data, because it will always be the same. You can check for these > still right in the loop, for all data entries. > Sure but that would be a specific code path in the generic comparison method. I feel like it's just simpler to treat them the same way as the other items and not add any special case. -- Rémi Bernon <rbernon(a)codeweavers.com>

Nov. 9, 2021

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

by 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(a)codeweavers.com>

Nov. 9, 2021