On 31 March 2010 15:07, Roderick Colenbrander thunderbird2k@gmail.com wrote:
I have worked on some more parts of the blit_shader code. Before I clean it up and submit it, I'm posting it here to see if the direction is ok.
I already commented on some issues with 0007 and 0008 since you already submitted those. As for the general direction, I think you're still trying to take fairly large steps. E.g., it's much easier to first introduce things like arbfp_blit_supported() and arbfp_blit_surface() as a separate functions, work them out properly for each implementation, and only then add them to the blitter interface. (I think I mentioned that before.) That's much nicer to review too, since I wouldn't be reviewing 3 or 4 different things at the same time.
0008 - Adds a blit method to arbfp_blit and limits the arbfp shader to complex blits (yuv/p8); for now this disables hw accelerated 8-bit color keying but that can easily be readded once the transition is over by modifying the p8 shader. I think this patch is ready. (I'm not that happy with the name arbfp_blit_surface but I had to use something else because arbfp_blit won't work and arbfp_blit_blit is a bit silly I think)
I didn't look too close at this one due to the reasons mentioned above, but my general feeling is that it may need some more work. (Beyond what I already mentioned in the reply to that patch.)
0009 - This moves the remaining 'offscreen -> render target' code to ffp_blit. I have thought long about whether to move the color key flag voodoo over as well. You can reason that it is up to the blit_shader how and if it implements color keying. If you move the code over, you would need some of the DDBLTFX information in the blit_shader (either through a parameter or some surface variable). I'm not sure if we want to go that way. I think it is better to leave the voodoo in one place.
You could also try something like introducing a function to the blitter interface to set color key information. I don't think this is the most important thing to worry about at this point though, so it's ok to leave it where it is for the moment. Note that that does imply that you can't have BLIT_OP_COLOR_KEYED_BLIT either yet.
0010 - Adds an initial fbo_blit implementation which supports blitting. I think this patch is ready but I'm only not sure where we want to have the fbo_blit code. Right now stretch_rect_fbo is in device.c (and the same for fbo color fill but Henri said that we shouldn't use it). I would say that the code should be in surface.c unless it is fine to use stretch_rect_fbo (some types have to be unified to be able to get rid of casts) as a helper function.
I think surface.c is ok. The reason it's in device.c is mostly historic.