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

Re: [PATCH v2 5/5] 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=101184 Your paranoid android. === w8adm (32 bit report) === mshtml: events.c:1089: Test failed: unexpected call img_onerror events: Timeout === 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 === w10pro64 (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE

Nov. 2, 2021

Re: [PATCH vkd3d 5/5] vkd3d-shader/hlsl: Parse the Sample() method.

by Zebediah Figura (she/her)

On 11/2/21 09:04, Giovanni Mascellani wrote: > Hi, > > On 02/11/21 01:43, Zebediah Figura wrote: >> + if (!(coords = add_implicit_conversion(ctx, instrs, >> params->args[1], >> + >> ctx->builtin_types.vector[HLSL_TYPE_FLOAT][sampler_dim - 1], loc))) >> + coords = params->args[1]; > > I would suggest to use the hlsl_get_vector_type helper. Right, thanks. Most of these patches I've been submitting were written before that helper was...

Nov. 2, 2021

Re: [PATCH vkd3d 3/5] tests: Add a shader test for texture sampling.

by Zebediah Figura (she/her)

On 11/2/21 08:46, Giovanni Mascellani wrote: > Hi, > > On 02/11/21 01:43, Zebediah Figura wrote: >> +static D3D12_TEXTURE_ADDRESS_MODE parse_sampler_address_mode(const >> char *line, const char **rest) >> +{ >> + if (match_string(line, "border", rest)) >> + return D3D12_TEXTURE_ADDRESS_MODE_BORDER; >> + if (match_string(line, "clamp", rest)) >> + return D3D12_TEXTURE_ADDRESS_MODE_CLAMP; >> + if (match_string(line, "mirror once", rest)) >> + return D3D12_TEXTURE_ADDRESS_MODE_MIRROR_ONCE; >> + if (match_string(line, "mirror", rest)) >> + return D3D12_TEXTURE_ADDRESS_MODE_MIRROR; >> + if (match_string(line, "wrap", rest)) >> + return D3D12_TEXTURE_ADDRESS_MODE_WRAP; >> + fprintf(stderr, "Malformed address mode '%s'.\n", line); >> + return D3D12_TEXTURE_ADDRESS_MODE_WRAP; >> +} > > I don't like very much the "mirror once" thing: the structure of the > file hints that spaces are used for token separation, and here we would > be using two tokens for an option that seems to be completely similar to > the other options using just one token. Also, the hypothetical line > "mirror_once mirror mirror_once" would be easier to mentally scan than > "mirror once mirror mirror once". Can we change to "mirror_once"? Sure, seems reasonable to me. > Also, the validation fundamentalist that is in me would happily swap the > last return statement with a call to abort(), but I guess I can silence > him for a test program. It probably would be a good idea to fail more noisily in general, honestly. I'll send a follow-up patch. >> +static void parse_sampler_directive(struct sampler *sampler, const >> char *line) >> +{ >> + const char *const orig_line = line; >> + >> + if (match_string(line, "address", &line)) >> + { >> + sampler->u_address = parse_sampler_address_mode(line, &line); >> + sampler->v_address = parse_sampler_address_mode(line, &line); >> + sampler->w_address = parse_sampler_address_mode(line, &line); >> + } >> + else if (match_string(line, "filter", &line)) >> + { >> + static const struct >> + { >> + const char *string; >> + D3D12_FILTER filter; >> + } >> + filters[] = >> + { >> + {"point point point", D3D12_FILTER_MIN_MAG_MIP_POINT}, >> + {"point point linear", >> D3D12_FILTER_MIN_MAG_POINT_MIP_LINEAR}, >> + {"point linear point", >> D3D12_FILTER_MIN_POINT_MAG_LINEAR_MIP_POINT}, >> + {"point linear linear", >> D3D12_FILTER_MIN_POINT_MAG_MIP_LINEAR}, >> + {"linear point point", >> D3D12_FILTER_MIN_LINEAR_MAG_MIP_POINT}, >> + {"linear point linear", >> D3D12_FILTER_MIN_LINEAR_MAG_POINT_MIP_LINEAR}, >> + {"linear linear point", >> D3D12_FILTER_MIN_MAG_LINEAR_MIP_POINT}, >> + {"linear linear linear", >> D3D12_FILTER_MIN_MAG_MIP_LINEAR}, >> + }; >> + unsigned int i; >> + >> + for (i = 0; i < ARRAY_SIZE(filters); ++i) >> + { >> + if (match_string(line, filters[i].string, &line)) >> + sampler->filter = filters[i].filter; >> + } >> + } >> + else >> + { >> + goto err; >> + } >> + >> + return; >> + >> +err: >> + fprintf(stderr, "Ignoring malformed line '%s'.\n", orig_line); >> +} > > I don't find the goto statement very useful: you could just print the > error in the "else" branch. Unless you have plans to extend the function > in ways I don't see now, I think this is just complicating the control > flow. Not for parse_sampler_directive() specifically, but in general yes. But something like a fatal_error(...) function would also obviate the need for a goto. > >> @@ -305,21 +383,40 @@ static void parse_test_directive(struct >> shader_context *context, const char *lin >> range->RegisterSpace = 0; >> range->OffsetInDescriptorsFromTableStart = 0; >> - texture->heap = >> create_gpu_descriptor_heap(context->c.device, >> D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, 1); >> - texture->resource = >> create_default_texture(context->c.device, texture->width, >> texture->height, >> - texture->format, 0, D3D12_RESOURCE_STATE_COPY_DEST); >> - resource_data.pData = texture->data; >> - resource_data.SlicePitch = resource_data.RowPitch = >> texture->width * texture->texel_size; >> - upload_texture_data(texture->resource, &resource_data, 1, >> context->c.queue, command_list); >> - reset_command_list(command_list, context->c.allocator); >> - transition_resource_state(command_list, >> texture->resource, D3D12_RESOURCE_STATE_COPY_DEST, >> - D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | >> D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE); >> - ID3D12Device_CreateShaderResourceView(context->c.device, >> texture->resource, >> - NULL, get_cpu_descriptor_handle(&context->c, >> texture->heap, 0)); >> + if (!texture->resource) >> + { >> + texture->heap = >> create_gpu_descriptor_heap(context->c.device, >> + D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, 1); >> + texture->resource = >> create_default_texture(context->c.device, texture->width, >> texture->height, >> + texture->format, 0, >> D3D12_RESOURCE_STATE_COPY_DEST); >> + resource_data.pData = texture->data; >> + resource_data.SlicePitch = resource_data.RowPitch = >> texture->width * texture->texel_size; >> + upload_texture_data(texture->resource, >> &resource_data, 1, context->c.queue, command_list); >> + reset_command_list(command_list, context->c.allocator); >> + transition_resource_state(command_list, >> texture->resource, D3D12_RESOURCE_STATE_COPY_DEST, >> + >> D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | >> D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE); >> + >> ID3D12Device_CreateShaderResourceView(context->c.device, >> texture->resource, >> + NULL, get_cpu_descriptor_handle(&context->c, >> texture->heap, 0)); >> + } > > Is this hunk related to the patch? AFAIU, it just prevents recreating > the texture resource if it's already there, which is a good thing, but > probably worthy of another patch. Mostly because this is the first shader_test file that includes more than one valid [test] directive. But yes, it could use a separate patch. Same for the hunk quoted below. > >> @@ -650,11 +765,36 @@ START_TEST(shader_runner_d3d12) >> if (!strcmp(line, "[pixel shader]\n")) >> { >> state = STATE_SHADER_PIXEL; >> + >> + if (context.ps_code) >> + ID3D10Blob_Release(context.ps_code); >> + context.ps_code = NULL; >> } > > This hunk too seems unrelated to this patch. > > Thanks, Giovanni. >

Nov. 2, 2021

Re: [v2 PATCH 7/7] d3d10/effect: Add initial support for indexing expressions.

by Matteo Bruni

On Mon, Nov 1, 2021 at 11:52 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/d3d10/effect.c | 556 +++++++++++++++++++++++++++++++++++++- > dlls/d3d10/tests/effect.c | 114 ++++++++ > 2 files changed, 669 insertions(+), 1 deletion(-) Still looking into this one. For the time being I have a single nitpick, probably not worth acting on it just yet: > diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c > index 78a3d0c386a..3ae2ad4f481 100644 > --- a/dlls/d3d10/effect.c > +++ b/dlls/d3d10/effect.c > @@ -30,6 +30,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(d3d10); > ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 )) > #define TAG_DXBC MAKE_TAG('D', 'X', 'B', 'C') > #define TAG_FX10 MAKE_TAG('F', 'X', '1', '0') > +#define TAG_FXLC MAKE_TAG('F', 'X', 'L', 'C') > +#define TAG_CLI4 MAKE_TAG('C', 'L', 'I', '4') > +#define TAG_CTAB MAKE_TAG('C', 'T', 'A', 'B') > > #define D3D10_FX10_TYPE_COLUMN_SHIFT 11 > #define D3D10_FX10_TYPE_COLUMN_MASK (0x7 << D3D10_FX10_TYPE_COLUMN_SHIFT) > @@ -180,6 +183,89 @@ static enum d3d10_effect_container_type get_var_container_type(const struct d3d1 > } > } > > +struct preshader_instr > +{ > + unsigned int comp_count : 16; > + unsigned int reserved : 4; > + unsigned int opcode : 11; > + unsigned int scalar : 1; > +}; > + > +typedef float (*pres_op_func)(float **args, unsigned int n); > + > +static float pres_ftou(float **args, unsigned int n) > +{ > + unsigned int u = *args[0]; > + return *(float *)&u; > +} > + > +static float pres_add(float **args, unsigned int n) > +{ > + return *args[0] + *args[1]; > +} > + > +enum preshader_op > +{ > + D3D10_PRESHADER_OP_FTOU = 0, > + D3D10_PRESHADER_OP_ADD, > + D3D10_PRESHADER_OP_LAST_OP, > +}; > + > +struct preshader_op_info > +{ > + unsigned short idx; > + unsigned short opcode; > + char name[8]; > + pres_op_func func; > +}; idx is an enum preshader_op, right? Any reason you're not using that type? The field name seems to have room for improvement too (although I realize the trouble with "opcode" also there).

Nov. 2, 2021