3/4 wined3d: add color_fill to blit_shader

[Henri Verbeet]

On 25 March 2010 23:04, Roderick Colenbrander <thunderbird2k at gmail.com> wrote:
> This patch adds a first part of the new blit_shader infrastructure. In
> a few days I plan to submit more parts of it which will extend the
> blit_shader and rewrite parts in the end.
>
This is patch is pretty hacky.

> +static const struct blit_shader *blitters[] =
> +{
> +    &ffp_blit,
> +    &cpu_blit
> +};
This doesn't make much sense here. You'll probably want to make it a
variable local to surface_select_blitter() as a start, but eventually
it should be stored in the device / adapter.

> +const struct blit_shader *surface_select_blitter(const struct wined3d_gl_info *gl_info, IWineD3DSurfaceImpl *src, IWineD3DSurfaceImpl *dst, enum blit_operation op)
Why do we have both this and select_blit_implementation() now? Also, static.

> +    switch(op)
> +    {
> +        case BLIT_OP_COLOR_FILL:
> +            return TRUE;
> +        default:
> +            return FALSE;
> +    }
This is a silly way to write "return op == BLIT_OP_COLOR_FILL;".

> +const struct blit_shader cpu_blit =  {
> +    NULL, /* alloc_private */
> +    NULL, /* free_private */
> +    NULL, /* set_shader */
> +    NULL, /* unset_shader */
> +    NULL, /* color_fixup_supported */
> +    cpu_blit_can_blit,
> +    cpu_blit_color_fill
>  };
You can't do that, callers don't check these for NULL.

> +typedef enum blit_operation
> +{
> +    BLIT_OP_COLOR_FILL=1,
> +    BLIT_OP_BLIT_SYSMEM_TO_DRAWABLE=2,
> +    BLIT_OP_BLIT_DRAWABLE_TO_TEXTURE=3
> +} blit_operation;
Besides being unused, the distinction between "SYSMEM_TO_DRAWABLE",
"DRAWABLE_TO_TEXTURE", and other potential variants is an
implementation detail callers shouldn't care about. Also, we don't
care about the exact values of the enum elements, and the typedef only
obscures the type of "blit_operation".

>      BOOL (*color_fixup_supported)(const struct wined3d_gl_info *gl_info, struct color_fixup_desc fixup);
> +    BOOL (*can_blit)(const struct wined3d_gl_info *gl_info, blit_operation op, IWineD3DSurfaceImpl *src_surface, IWineD3DSurfaceImpl *dst_surface);
The functionality of "can_blit" is a superset of "color_fixup_supported".

> +    IWineD3DSurface_GetContainer((IWineD3DSurface*)dst_surface, &IID_IWineD3DSwapChain, (void **)&dst_swapchain);
> +    if(dst_swapchain) IWineD3DSwapChain_Release((IWineD3DSwapChain *) dst_swapchain);
That's three different styles of doing a cast, in two lines of code.
And code that's supposedly being rewritten at that. At this point I
don't even care about the exact style you're using (although I do have
a preference), but try to at least care about consistency. Same goes
for these:
> +    if(dst_swapchain) IWineD3DSwapChain_Release((IWineD3DSwapChain *) dst_swapchain);
> +    switch(op)
> +                if ((IWineD3DSurface*)dst_surface != device->render_targets[0] &&

> +const struct blit_shader *surface_select_blitter(const struct wined3d_gl_info *gl_info, IWineD3DSurfaceImpl *src, IWineD3DSurfaceImpl *dst, enum blit_operation op)
This line is a bit long for my taste. I think I have relatively large
screens, but this takes up over half a screen. Personally I think 120
characters is a reasonable upper limit, but there are probably plenty
of people that would be happier with 80.

Essentially, I think you're trying to take too large steps. E.g.
splitting of functionality into cpu_blit_color_fill(),
ffp_blit_color_fill(), etc. is ok, hacking it into struct blit_shader
isn't. Extending color_fixup_supported() into something like
blit_supported() could also be a reasonable patch, on its own, but
trying to do it all at the same time becomes hacky pretty quickly.