Re: [PATCH 05/10] wined3d: Try to avoid glGetTexImage() for 2D array textures.
On 19 April 2016 at 00:01, Józef Kucia <jkucia(a)codeweavers.com> wrote:
Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- It's inefficient to download a complete miplevel to copy just a single layer from it. --- Does this happen much in practice? Do you have performance numbers? The patch doesn't really make sense in its current form. surface_download_data() is meant for downloading the data from the texture. If that's not what you want, don't call it. (Also, does this actually work? It looks like it's just reading from the current drawable.)
On Tue, Apr 19, 2016 at 11:24 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 19 April 2016 at 00:01, Józef Kucia <jkucia(a)codeweavers.com> wrote:
Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- It's inefficient to download a complete miplevel to copy just a single layer from it. --- Does this happen much in practice? Do you have performance numbers? The patch doesn't really make sense in its current form. surface_download_data() is meant for downloading the data from the texture. If that's not what you want, don't call it. (Also, does this actually work? It looks like it's just reading from the current drawable.)
The issue is if we do not have ARB_get_texture_sub_image we use glGet[Compressed]TexImage() to download the data from texture. This type of download is implemented in the patch 6. I want to avoid glGet[Compressed]TexImage() for array textures because these functions don't allow to download the data for a single wined3d surface. They allow to download data for the whole miplevel slice. In the patch 6 a block of memory is allocated, the whole miplevel slice is downloaded and the data for a single layer is copied to the current wined3d surface. The idea in this patch is to simulate glGetTextureSubImage() using reads from the framebuffer. An array array texture is attached to framebuffer using glFramebufferTextureLayer() and the data for a single wined3d surface is downloaded using glReadPixels(). This works in my d3d11 tests which I plan to submit in the following patchset.
On 19 April 2016 at 12:17, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
The issue is if we do not have ARB_get_texture_sub_image we use glGet[Compressed]TexImage() to download the data from texture. This type of download is implemented in the patch 6. I want to avoid glGet[Compressed]TexImage() for array textures because these functions don't allow to download the data for a single wined3d surface. They allow to download data for the whole miplevel slice. In the patch 6 a block of memory is allocated, the whole miplevel slice is downloaded and the data for a single layer is copied to the current wined3d surface.
The idea in this patch is to simulate glGetTextureSubImage() using reads from the framebuffer. An array array texture is attached to framebuffer using glFramebufferTextureLayer() and the data for a single wined3d surface is downloaded using glReadPixels(). This works in my d3d11 tests which I plan to submit in the following patchset.
Well, yes. But it's not up to surface_download_data() to decide whether to use GetTexImage() or ReadPixels(). surface_load_sysmem() should decide if it's more appropriate to use surface_download_data() or read_from_framebuffer(). That said, I'm not sure read_from_framebuffer() is necessarily an improvement in terms of (overall) performance. True, GetTexImage() reads more data than it needs to, but I don't think it's a given that that's much more expensive than reading a smaller amount of data unless the texture is particularly large. In cases where you're going to access the other sub-resources afterwards anyway it may even be an improvement, provided resource locations etc. are properly updated. On the other hand, read_from_framebuffer() calls context_apply_blit_state(), which does a lot of state invalidation. If multiple of those are batched together that may be ok (but in that case perhaps surface_download_data() would be too), but if they're interleaved with draws it may not be an improvement. I.e., it would be a lot easier to justify this if you could show an application where it helps.
On Wed, Apr 20, 2016 at 10:34 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
Well, yes. But it's not up to surface_download_data() to decide whether to use GetTexImage() or ReadPixels(). surface_load_sysmem() should decide if it's more appropriate to use surface_download_data() or read_from_framebuffer().
That said, I'm not sure read_from_framebuffer() is necessarily an improvement in terms of (overall) performance. True, GetTexImage() reads more data than it needs to, but I don't think it's a given that that's much more expensive than reading a smaller amount of data unless the texture is particularly large. In cases where you're going to access the other sub-resources afterwards anyway it may even be an improvement, provided resource locations etc. are properly updated. On the other hand, read_from_framebuffer() calls context_apply_blit_state(), which does a lot of state invalidation. If multiple of those are batched together that may be ok (but in that case perhaps surface_download_data() would be too), but if they're interleaved with draws it may not be an improvement.
I.e., it would be a lot easier to justify this if you could show an application where it helps.
I don't have any application to show that this helps. I'll drop the patch. Is patch 121528 ok in its current form? It doesn't feel like a good idea messing with other surfaces in surface_download_data().
On 20 April 2016 at 10:54, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
Is patch 121528 ok in its current form? It doesn't feel like a good idea messing with other surfaces in surface_download_data(). It seems ok, but I'd have to review it properly.
participants (2)
-
Henri Verbeet -
Józef Kucia