On 25 March 2010 23:04, Roderick Colenbrander thunderbird2k@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.