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.