Re: [PATCH 6/6] wined3d: Introduce a scratch string buffers framework.
On 22 April 2015 at 19:30, Matteo Bruni <mbruni(a)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.
+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?
+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.
struct wined3d_shader_buffer { At some point we'll probably want to rename this to something like "wined3d_string_buffer".
2015-04-23 14:45 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 22 April 2015 at 19:30, Matteo Bruni <mbruni(a)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).
2015-04-23 16:12 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>:
2015-04-23 14:45 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 22 April 2015 at 19:30, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
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).
I thought that probably it would be even better if I do the rename before introducing those changes. The resulting patch is still quite large but I think it should be okay. Initially I also renamed shader_addline into string_buffer_addline. Not a good idea... I can certainly wait for your shader_vaddline() patch before sending this one (and then the v2 of the patch in subject).
participants (2)
-
Henri Verbeet -
Matteo Bruni