Re: [PATCH 5/6] wined3d: Use PBOs for dynamic volumes

[Henri Verbeet]

On 22 August 2013 23:22, Stefan Dösinger  wrote:
> @@ -145,6 +158,9 @@ static DWORD volume_access_from_location(DWORD location)
>          case WINED3D_LOCATION_TEXTURE_RGB:
>              return WINED3D_RESOURCE_ACCESS_GPU;
> -
>    case WINED3D_LOCATION_BUFFER:
> -
>        return WINED3D_RESOURCE_ACCESS_CPU | WINED3D_RESOURCE_ACCESS_GPU;
> -

Why does loading into a PBO require CPU access?
> -
>    case WINED3D_LOCATION_BUFFER:
> -
>        if (!volume->pbo || !(volume->flags & WINED3D_VFLAG_PBO))
> -
>        {
> -
>            ERR("Trying to load WINED3D_LOCATION_BUFFER without setting it up first.\n");
> -
>            return;
> -
>        }

I think there should never be a PBO without WINED3D_VFLAG_PBO being
set. There's also something to be said for leaving out the return,
since that would at least in theory allow the compiler to drop the
extra checks when compiled without debug messages. That probably
applies to ERRs in general, since they're either never supposed to
happen, or not supposed to be an ERR.
> +/* Context activation is done by the caller. */
> +static void wined3d_volume_prepare_pbo(struct wined3d_volume *volume, struct wined3d_context *context)
> +{
> - const struct wined3d_gl_info *gl_info = context->gl_info;
> -
> - if (volume->pbo)
> -
>    return;
> -
> - GL_EXTCALL(glGenBuffersARB(1, &volume->pbo));
> - checkGLcall("glGenBuffersARB(1, &volume->pbo)");
> - GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo));
> - checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo)");
> - GL_EXTCALL(glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB));
> - checkGLcall("glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB");
> - GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0));
> - checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0)");
> -
> - TRACE("Created PBO %u for volume %p.\n", volume->pbo, volume);
>
> +}

I think one checkGLcall() per function is enough in general. That's
even more true these days with ARB_debug_output that will usually give
more specific error messages than checkGLcall() anyway. Applies to a
couple of other places as well.