Hi Masanori,
Thank you for your patches! I have a few comments:
Regarding the big picture I think it would be great if we had a scheme that also worked for partially mapping resources. It's also necessary to make the D3DLOCK_NO_DIRTY_UPDATE test on managed textures work. However, doing this is a lot more work than your patchset and complicates location management a lot.
If Henri tells you something different follow his advice. He is the authority for decisions in d3d :-) .
Patch 1: Am 2017-04-17 um 15:47 schrieb Masanori Kakura:
+struct wined3d_dirty_regions +{
- UINT left;
- UINT top;
- UINT right;
- UINT bottom;
- UINT front;
- UINT back;
- struct wined3d_dirty_regions *next;
+};
A standard wine list would be better IMHO.
Patch 2:
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 785105d7b9..711f98a0a6 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -2765,6 +2765,27 @@ struct wined3d_dirty_regions struct wined3d_dirty_regions *next; };
+static inline BOOL wined3d_dirty_regions_add(struct wined3d_dirty_regions **regions,
UINT left, UINT top, UINT right, UINT bottom, UINT front, UINT back)
I don't think we want this in the header file, I think it'd be better placed in resource.c. Also note that buffers already have a related mechanism (buffer->modified_areas).
You can predeclare the functions in the header if you need to call them from multiple files.
+{
- struct wined3d_dirty_regions *new_region;
- if (!(new_region = HeapAlloc(GetProcessHeap(), 0, sizeof(struct wined3d_dirty_regions))))
return FALSE;
HeapAlloc is expensive. I think it is OK for the start, but in the long run a way to keep some old dirty region elements around and reuse them instead of always calling Alloc / Free.
Patch 5:
- if (!wined3d_dirty_regions_add(&resource->dirty_regions, 0, 0, width, height, 0, depth))
Why not use wined3d_resource_maximize_dirty_region here?
wined3d_resource_acquire(resource);
- wined3d_dirty_regions_clear(&resource->dirty_regions); wined3d_cs_destroy_object(resource->device->cs, wined3d_resource_destroy_object, resource);
Regions need to be cleared from inside the cs thread in case there are pending update_texture commands.
Similarly patch 6 needs to put the region modifying into wined3d_cs_exec_add_dirty_texture_region.
Patch 7:
if (box->left < box->right && box->top < box->bottom && box->front < box->back){
You don't need to check the box, texture_resource_sub_resource_map already does it before it reaches this place of the code. See wined3d_texture_check_box_dimensions.
Patch 8: I am not sure if this is necessary at all. In d3d9 it doesn't matter because a UpdateSurface destination can never be an UpdateTexture source.
Patch 9/10: These should be merged, and I don't think patch 10 looks right. I think dirty regions should be honored on sublevels as well, just adjusted (i.e., divide by 2 for every level).
- if (src_texture->resource.width <= dst_texture->resource.width
&& src_texture->resource.height <= dst_texture->resource.height&& src_texture->resource.depth <= dst_texture->resource.depth)
Why are you adding this check? Is it because the src_skip_levels broke the if (i == 0) root level detection? If you apply the dirty region to all levels this problem should go away.