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

[Giovanni Mascellani]

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"?

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.

> +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.

> @@ -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.

> @@ -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.