[PATCH 6/6] wined3d: Introduce a scratch string buffers framework.

[Matteo Bruni]

2015-04-23 14:45 GMT+02:00 Henri Verbeet <hverbeet at gmail.com>:
> On 22 April 2015 at 19:30, Matteo Bruni <mbruni at codeweavers.com> wrote:
>> It's somewhat of an RFC at this point. In particular I'd like some
>> feedback about the API.
> In general, I think it goes in the right direction.
>
>> An obvious variant would be to store the pointer to the parent
>> wined3d_shader_buffer structure right before the string and just
>> return the string itself (thus avoiding having to add ->buffer to
>> all the uses of the string).
> Personally, I don't particularly mind the couple of extra characters,
> and it seems like a good thing to explicitly distinguish between
> string buffers and regular strings.
>
>> +        tex_reg_name = shader_sprintf_unsafe(&priv->string_buffers, "tex%u", stage);
>>          shader_glsl_color_correction_ext(buffer, tex_reg_name, WINED3DSP_WRITEMASK_ALL,
>>                  settings->op[stage].color_fixup);
> The problem with this construction is that it is likely to break as
> soon as shader_glsl_color_correction_ext() does anything that ends up
> calling shader_get_buffer(). To make it worse,
> shader_glsl_color_correction_ext() isn't aware of this because it just
> gets a const char *, and shader_glsl_generate_ffp_fragment_shader()
> isn't supposed to care about the internals of
> shader_glsl_color_correction_ext().
>
>> @@ -276,13 +276,17 @@ int shader_vaddline(struct wined3d_shader_buffer *buffer, const char *format, va
>>      char *base = buffer->buffer + buffer->content_size;
>>      int rc;
>>      char *new_buffer;
>> +    unsigned int new_buffer_size;
>>
>>      while(1)
>>      {
>>          rc = vsnprintf(base, buffer->buffer_size - buffer->content_size, format, args);
>>          if (rc < 0 /* C89 */ || (unsigned int)rc >= buffer->buffer_size - buffer->content_size /* C99 */)
>>          {
>> -            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, buffer->buffer_size * 2);
>> +            new_buffer_size = buffer->buffer_size * 2;
>> +            while (rc > 0 && (unsigned int)rc >= new_buffer_size - buffer->content_size)
>> +                new_buffer_size *= 2;
>> +            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, new_buffer_size);
>>              if (!new_buffer)
>>              {
>>                  ERR("The buffer allocated for the shader program string is too small at %d bytes.\n", buffer->buffer_size);
>> @@ -290,7 +294,7 @@ int shader_vaddline(struct wined3d_shader_buffer *buffer, const char *format, va
>>                  return -1;
>>              }
>>              buffer->buffer = new_buffer;
>> -            buffer->buffer_size = buffer->buffer_size * 2;
>> +            buffer->buffer_size = new_buffer_size;
>>              base = buffer->buffer + buffer->content_size;
>>          }
>>          else
> This is a bit of an unrelated change, but probably the right thing to
> do. Note that I have an unsent patch that separates tracing the shader
> source from shader_vaddline() though, so it's going to look a bit
> different fairly soon.

That was prompted by the change of the initial size of the
wined3d_shader_buffer to 32. They should both go into a separate patch
indeed.

>> +static char shader_empty_string[] = "";
> This should be const, right?
>
>> +static void shader_sprintf_va_list(struct wined3d_shader_buffer *buffer, const char *format, va_list args)
> I probably would have called that vprintf/vsprintf instead of sprintf_va_list.
>
>> +{
>> +    int ret;
>> +    char *new_buffer;
>> +    unsigned int new_buffer_size;
>> +
>> +    if (!buffer)
>> +        return;
>> +    while (1)
>> +    {
>> +        ret = vsnprintf(buffer->buffer, buffer->buffer_size, format, args);
>> +        if (ret < 0 || (unsigned int)ret >= buffer->buffer_size)
>> +        {
>> +            new_buffer_size = buffer->buffer_size * 2;
>> +            while (ret > 0 && (unsigned int)ret >= new_buffer_size)
>> +                new_buffer_size *= 2;
>> +            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, new_buffer_size);
>> +            if (!new_buffer)
>> +            {
>> +                ERR("Couldn't allocate buffer for temporary string.\n");
>> +                buffer->buffer[0] = 0;
>> +                return;
>> +            }
>> +            buffer->buffer = new_buffer;
>> +            buffer->buffer_size = new_buffer_size;
>> +        }
>> +        else
>> +        {
>> +            break;
>> +        }
>> +    }
>> +}
> Once tracing the shader source is removed from shader_vaddline(), this
> comes down to a shader_buffer_clear() followed by shader_vaddline().
> And arguably shader_get_buffer() should always shader_buffer_clear()
> the buffers it returns.
>
>> +/* This function immediately puts the buffer back into the free list (it's
>> + * intended to be used when the string is to be used and discarded immediately,
>> + * mostly useful to avoid having to move the string pointer out of the block). */
>> +const char *shader_sprintf_unsafe(struct list *buffer_list, const char *format, ...)
> Not a fan, for the reasons mentioned at
> shader_glsl_generate_ffp_fragment_shader() above.
>
>> +void shader_release_buffer(struct list *list, struct wined3d_shader_buffer *buffer)
>> +{
>> +    if (!buffer)
>> +        return;
> Can buffer ever legitimately be NULL?

Only on allocation failure. At that point though the caller of
shader_get_buffer() will probably crash, unless it checks the returned
value. In that case the caller could avoid calling
shader_release_buffer() altogether but I though to be consistent with
free() and HeapFree() and allow a NULL parameter too.
I'm not sure there will be any such user in practice though.

>> +void shader_buffer_list_free(struct list *list)
>> +{
>> +    struct wined3d_shader_buffer *buffer, *buffer_next;
>> +
>> +    LIST_FOR_EACH_ENTRY_SAFE(buffer, buffer_next, list, struct wined3d_shader_buffer, entry)
>> +    {
>> +        shader_buffer_free(buffer);
>> +        HeapFree(GetProcessHeap(), 0, buffer);
>> +    }
>> +}
>> +
> This doesn't actually remove the buffers from the list. There may also
> be an argument for wrapping the list itself in a structure, similar to
> struct wined3d_private_store.

That would only matter if one tries to use the list after calling
shader_buffer_list_free() and before calling list_init() again. Yes,
that's somewhat asymmetric. I'm going to go with the separate
structure, that should indirectly help.

>>  struct wined3d_shader_buffer
>>  {
> At some point we'll probably want to rename this to something like
> "wined3d_string_buffer".

Right, and probably that should happen in the patch introducing this
stuff to limit the amount of churn (while moving some of the hunks
making use of the new functions to separate patches).