On 28 April 2010 14:29, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- /* Early sort out of cases where no render target is used */
- if (!dstSwapchain && !srcSwapchain
- && src_surface != device->render_targets[0]
- && dst_surface != device->render_targets[0])
- {
- if (fbo_blit_supported(&device->adapter->gl_info, BLIT_OP_BLIT,
- &src_rect, src_surface->resource.usage, src_surface->resource.pool, src_surface->resource.format_desc,
- &dst_rect, dst_surface->resource.usage, dst_surface->resource.pool, dst_surface->resource.format_desc))
- {
- stretch_rect_fbo(device, src_surface, &src_rect, dst_surface, &dst_rect, Filter);
- return WINED3D_OK;
- }
- TRACE("No surface is render target, not using hardware blit.\n");
- return WINED3DERR_INVALIDCALL;
- }
You might as well try FBO blits first, before doing most of the other checks. That's how it's eventually supposed to work anyway, and you could remove the other strecth_rect_fbo() calls in that case. That does depend on fbo_blit_supported() working properly, of course.
On Wed, Apr 28, 2010 at 2:56 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 28 April 2010 14:29, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- /* Early sort out of cases where no render target is used */
- if (!dstSwapchain && !srcSwapchain
- && src_surface != device->render_targets[0]
- && dst_surface != device->render_targets[0])
- {
- if (fbo_blit_supported(&device->adapter->gl_info, BLIT_OP_BLIT,
- &src_rect, src_surface->resource.usage, src_surface->resource.pool, src_surface->resource.format_desc,
- &dst_rect, dst_surface->resource.usage, dst_surface->resource.pool, dst_surface->resource.format_desc))
- {
- stretch_rect_fbo(device, src_surface, &src_rect, dst_surface, &dst_rect, Filter);
- return WINED3D_OK;
- }
- TRACE("No surface is render target, not using hardware blit.\n");
- return WINED3DERR_INVALIDCALL;
- }
You might as well try FBO blits first, before doing most of the other checks. That's how it's eventually supposed to work anyway, and you could remove the other strecth_rect_fbo() calls in that case. That does depend on fbo_blit_supported() working properly, of course.
I had a look at the code and it looks like all the swapchain related calls don't affect FBOs but some questions though. First of I will write tests before I attempt such a risky patch which would improve test coverage of untested parts of BltOverride and which also test Blt/StretchRect more thoroughly.
} else if(dstSwapchain && srcSwapchain) { FIXME("Implement hardware blit between two different swapchains\n"); It looks like this case would work fine using stretch_rect_fbo. In the end it comes down to binding two textures to an FBO and a blit which works because the different swapchains are sharing lists. Corect me if I'm wrong since I don't know the subtle details.
} else if(dstSwapchain && dstSwapchain == srcSwapchain) { FIXME("Implement hardware blit between two surfaces on the same swapchain\n"); return WINED3DERR_INVALIDCALL; This case is for swapchain blits other than back->front. stretch_rect_fbo should be able to handle this properly except for overlaps because BlitFramebuffer doesn't support overlaps. This is no issue for StretchRect which doesn't support this either but Blt surely does (at least in some cases, so more tests are needed). Such a case could be supported by drawing in multiple passes or by just disallowing this but for that I need swapchain information in 'blit_supported'.
Information about swapchains might be needed in blit_supported anyway for ffp_blit_supported. A lot of swapchain related cases (two were shown some lines up) are not supported in the ffp backend (and perhaps also in the arbfp one later on ). Some of the cases can be fixed but I fear some cases can't be. What do you think? I hope we don't have to pass variables telling that src / dst are on a swapchain and whether they are on the same.
Roderick
On 29 April 2010 15:38, Roderick Colenbrander thunderbird2k@gmail.com wrote:
} else if(dstSwapchain && srcSwapchain) { FIXME("Implement hardware blit between two different swapchains\n"); It looks like this case would work fine using stretch_rect_fbo. In the end it comes down to binding two textures to an FBO and a blit which works because the different swapchains are sharing lists. Corect me if I'm wrong since I don't know the subtle details.
In principle surfaces that are on a swapchain are onscreen buffers like GL_FRONT and GL_BACK. You can make that work between different GL contexts, but it involves reading back at least one of them into a texture. The exception is when we're rendering the backbuffer offscreen, because in that case at least the backbuffers are textures as well.
} else if(dstSwapchain && dstSwapchain == srcSwapchain) { FIXME("Implement hardware blit between two surfaces on the same swapchain\n"); return WINED3DERR_INVALIDCALL; This case is for swapchain blits other than back->front. stretch_rect_fbo should be able to handle this properly except for overlaps because BlitFramebuffer doesn't support overlaps. This is no issue for StretchRect which doesn't support this either but Blt surely does (at least in some cases, so more tests are needed). Such a case could be supported by drawing in multiple passes or by just disallowing this but for that I need swapchain information in 'blit_supported'.
Well, overlaps can only happen if the source and destination surface are the same, that doesn't have much to do with being on the same swapchain. A case where you can have different surfaces on the same swapchain that aren't the front and back buffers would be a blit between different backbuffers. Multiple backbuffers is something that's currently unimplemented though.
Information about swapchains might be needed in blit_supported anyway for ffp_blit_supported. A lot of swapchain related cases (two were shown some lines up) are not supported in the ffp backend (and perhaps also in the arbfp one later on ). Some of the cases can be fixed but I fear some cases can't be. What do you think? I hope we don't have to pass variables telling that src / dst are on a swapchain and whether they are on the same.
Multiple swapchains is something that you can probably implement by switching contexts and downloading one of the surfaces into a texture in the worst case. You can probably avoid overlapped blits by checking for them in the caller and making them a separate blit operation. Worst case you can do those by downloading into a separate texture first as well, although there may be a performance consideration to make as well.
On Wed, Apr 28, 2010 at 2:56 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 28 April 2010 14:29, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- /* Early sort out of cases where no render target is used */
- if (!dstSwapchain && !srcSwapchain
- && src_surface != device->render_targets[0]
- && dst_surface != device->render_targets[0])
- {
- if (fbo_blit_supported(&device->adapter->gl_info, BLIT_OP_BLIT,
- &src_rect, src_surface->resource.usage, src_surface->resource.pool, src_surface->resource.format_desc,
- &dst_rect, dst_surface->resource.usage, dst_surface->resource.pool, dst_surface->resource.format_desc))
- {
- stretch_rect_fbo(device, src_surface, &src_rect, dst_surface, &dst_rect, Filter);
- return WINED3D_OK;
- }
- TRACE("No surface is render target, not using hardware blit.\n");
- return WINED3DERR_INVALIDCALL;
- }
You might as well try FBO blits first, before doing most of the other checks. That's how it's eventually supposed to work anyway, and you could remove the other strecth_rect_fbo() calls in that case. That does depend on fbo_blit_supported() working properly, of course.
I'm investigating how I want to continue with this code after the code freeze is over. In the mean time I plan to write a few tests (if needed) to hopefully prevent regressions later on. I have a few questions left regarding this reply from a while back.
In the end we want some mechanism to select the right blit_shader using 'blit_supported()' and use that to perform the requested blit. At this point I think that fbo_blit_supported() works properly for the cases we call stretch_rect_fbo (though there is still some unneeded if-statement in fbo_blit_supported which I still have to take out). Regarding BltOverride in what way did you want to 'try the FBO blit first'. Did you want fbo_blit_supported sorting out the 'unsupported swapchain cases' (it lacks surface info and only has flags) itself or was all you wanted just: if ( !unhandled_swapchain_cases && !color_keying && fbo_blit_supported(..)) stretch_rect_fbo(..);
continue with all other checks
I'm planning to write some more DirectDraw Blt tests to stress this code some more. Are there any specific tests (or perhaps some D3D9 tests) you want to see as well when I'm at it?
Thanks, Roderick
On 22 June 2010 21:38, Roderick Colenbrander thunderbird2k@gmail.com wrote:
Regarding BltOverride in what way did you want to 'try the FBO blit first'. Did you want fbo_blit_supported sorting out the 'unsupported swapchain cases' (it lacks surface info and only has flags) itself or was all you wanted just: if ( !unhandled_swapchain_cases && !color_keying && fbo_blit_supported(..)) stretch_rect_fbo(..);
continue with all other checks
In principle inside fbo_blit_supported() should be fine, but for a part that's also what you're supposed to figure out. I.e., which of those checks are generic restrictions, which of those are fixed function blitter restrictions, which are FBO blitter restrictions, etc.
-
Henri Verbeet -
Roderick Colenbrander