On 3 November 2015 at 23:55, Stefan Dösinger stefan@codeweavers.com wrote:
Instead of its sub-functions. See comments in patch 3 for details about surface_blt_fbo.
This fixes bug 39536. surface_load_location didn't prepare the multisample renderbuffer.
The next patches will consistently move the responsibility to the caller of surface_load_location.
This isn't necessarily wrong on its own, although I wonder if it's really worth it. I don't think it makes the API easier to use correctly, and I don't think there's going to be much of a performance impact.
For fixing bug 39536 this is wrong though. If I'm reading the terminal output there correctly, the issue is that load_location() is called with WINED3D_LOCATION_RB_MULTISAMPLE, which it doesn't know how to handle. This patch will hide the issue by simply always calling prepare_location(). I'd have to check if multisample surfaces are supposed to have defined content after creation and presents, but perhaps what you need to do here is to add proper WINED3D_LOCATION_DISCARDED handling for multisample surfaces, similar to depth stencil surfaces.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Am 2015-11-04 um 12:30 schrieb Henri Verbeet:
The next patches will consistently move the responsibility to the caller of surface_load_location.
This isn't necessarily wrong on its own, although I wonder if it's really worth it. I don't think it makes the API easier to use correctly, and I don't think there's going to be much of a performance impact.
I'm also happy to keep the prepare call inside load_location, which would just mean dropping patch 3. Right now it's a mix of the caller being responsible (sysmem, renderbuffers) and surface_load_location being responsible (texture). I don't think that's what we want.
The argument for making the callers responsible is that some places like map and clear need to call prepare_location manually anyway.
For fixing bug 39536 this is wrong though. If I'm reading the terminal output there correctly, the issue is that load_location() is called with WINED3D_LOCATION_RB_MULTISAMPLE, which it doesn't know how to handle. This patch will hide the issue by simply always calling prepare_location(). I'd have to check if multisample surfaces are supposed to have defined content after creation and presents, but perhaps what you need to do here is to add proper WINED3D_LOCATION_DISCARDED handling for multisample surfaces, similar to depth stencil surfaces.
Your reading of the logs is correct, although the surface is in sysmem, not discarded. But the renderbuffer init problem existed before 1ca9dfc8, and as you say it still exists after my patches and surface_load_location writes the same FIXME after my patches.
I could implement uploads from sysmem (going through a non-multisampled texture or using glDrawPixels), but what we probably really want is some sort of WINED3D_LOCATION_ZEROED location that will init whatever location is used first to zero (probably using GL_ARB_clear_texture where applicable), and also better WINED3D_LOCATION_DISCARD handling (presents, discard rendertargets). All those need more thought and aren't necessarily related to creating the storage, so I decided to fix the creation problem and leave the initial upload the way it was / is.
On 4 November 2015 at 13:10, Stefan Dösinger stefandoesinger@gmail.com wrote:
I'm also happy to keep the prepare call inside load_location, which would just mean dropping patch 3. Right now it's a mix of the caller being responsible (sysmem, renderbuffers) and surface_load_location being responsible (texture). I don't think that's what we want.
The caller is not responsible in case of renderbuffers. The general rule is that if you're going to be reading/writing from/to a location you need to call either load_location() or prepare_location(). The sysmem handling does indeed violate that in some places, those are bugs.
Your reading of the logs is correct, although the surface is in sysmem, not discarded. But the renderbuffer init problem existed
Well yes, but perhaps it should be in WINED3D_LOCATION_DISCARDED.
I could implement uploads from sysmem (going through a non-multisampled texture or using glDrawPixels), but what we probably really want is some sort of WINED3D_LOCATION_ZEROED location that will init whatever location is used first to zero (probably using
We'll want that at some point, but I seem to recall that the initial contents of multisample surfaces is undefined. If it isn't it should only be a couple of lines to add support for WINED3D_LOCATION_RB_MULTISAMPLE to surface_load_location().
-
Henri Verbeet -
Stefan Dösinger