June 2017 - Wine-Devel - list.winehq.org

Re: [PATCH] gdiplus: Scale widened dashes to the pen width.

by Vincent Povirk

If the dash_pos variable is relative to pen width, we need to fix the other references to it. It doesn't make sense to compare it directly with segment_length and segment_pos. Or we could scale the whole dash_pattern array by pen width at the start.

June 27, 2017

Fwd: Improvements to the heap allocator

by Andrea Canciani

The original message was apparently blocked by the mailing list rules/filter. Now I am subscribed to wine-devel, sorry if you get the message twice. Andrea ---------- Forwarded message ---------- From: Andrea Canciani <ranma42(a)gmail.com> Date: Fri, Jun 23, 2017 at 9:29 AM Subject: Improvements to the heap allocator To: wine-devel(a)winehq.org Yesterday I opened a bug report about a performance issue in the wine heap allocator: https://bugs.winehq.org/show_bug.cgi?id=43224 Since I was also asking for some suggestions about what the preferred fix would be, Sebastian Lackner pointed to this mailing list. The bug report contains the source code for a program that has an allocation pattern that is very bad (for the current wine allocator). This appears to be a known problem, but I was unable to find any test/benchmark that shows such degenerate behavior. So far none of the attempts at fixing it seem to have landed. Some possible approaches are: 1. ensure that the freelist sizes cover all (alignment rounded) sizes up to a certain value; 2. when looking up for a free block, start from the freelist whose smallest element is at least as big as the desired block; 3. implement more complex free block management. #1 would fix the issue for some allocations sizes, but for bigger allocations the problem would stay the same. #2 would fix the performance problem at the cost of increased memory usage: for some sizes it would be possible that after a block is allocated and freed, the following allocation for that size does not re-use that memory (if the free block is stored in a freelist together with smaller blocks). #3 would increase the complexity of the code; to keep it under control, it might be possible to re-use the red-black tree implementation already available in wine. To avoid regressing performance, the smallest blocks might still be handled with simple freelists (fixed as in #1), while larger blocks are managed in a tree. Some of the current/previous attempts at tackling this issue are: - https://github.com/wine-compholio/wine-staging/blob/ master/patches/ntdll-Heap_FreeLists/0001-ntdll-Improve- heap-allocation-performance-by-using-m.patch (implements #1) - https://github.com/laino/wine-patches (IIUC, a combination of #1 and #3, in which the code tries to keep the freelist length under control, but AFAICT there is no guaranteed bound) I would like to try and work on fixing this, but there are some decision to make that might depend on constraints I am not aware of. Would the solution I propose (#3, rbtree based, with some tuning for small allocation sizes) be acceptable? Are there any benchmarks that a new implementation should strive to improve or at least avoid regressing? Suggestions and alternative solutions are welcome :) Sorry for the long message, thank you for your patience Andrea

June 27, 2017

Re: [v5 2/3] d3dx9: Get rid of constant's length checking for matrix in set_constants().

by Matteo Bruni

2017-06-27 20:38 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 06/27/2017 09:34 PM, Matteo Bruni wrote: >> >> 2017-06-22 15:11 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >>> >>> When count in const_upload_info is calculated precisely (considering >>> actual >>> constant set length), boolean matrix setting fall in direct copy path, >>> unless >>> transpose is required. The case of incomplete last row for matrix should >>> be >>> handled in set_constants() for transposed boolean constant only. >>> >>> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> >>> --- >>> v5: >>> - added check for constant size not exceeding parameter size. The >>> check for >>> non array, non struct constants should be sufficient as the >>> dimensions of >>> arrays and structs are derived from const_set data and register >>> counts >>> from constant description is not used for them. >> >> I don't understand what you mean here. >> >>> @@ -1417,6 +1433,16 @@ static HRESULT init_set_constants_param(struct >>> d3dx_const_tab *const_tab, ID3DXC >>> && !info.transpose && info.minor == info.major_stride >>> && info.count == get_offset_reg(const_set.table, >>> const_set.register_count) >>> && info.count * sizeof(unsigned int) <= param->bytes; >>> + if (info.minor_remainder && !const_set.direct_copy && >>> !info.transpose) >>> + FIXME("Incomplete last row for not transposed matrix which >>> cannot be directly copied, parameter %s.\n", >>> + debugstr_a(param->name)); >>> + >>> + if (info.major_count > info.major >>> + || (info.major_count == info.major && info.minor_remainder)) >>> + { >>> + WARN("Constant dimensions exceed parameter size.\n"); >>> + return D3DERR_INVALIDCALL; >>> + } >> >> It seems to me that this catches all the problematic cases. >> >> > Yes, that what I mean above :) I tried to explain why I don't need a > separate check that for arrays and structs, but probably the explanation is > less clear than code itself. Ahh, I see. Indeed the code speaks for itself :D

June 27, 2017

Re: [v5 2/3] d3dx9: Get rid of constant's length checking for matrix in set_constants().

by Paul Gofman

On 06/27/2017 09:34 PM, Matteo Bruni wrote: > 2017-06-22 15:11 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> When count in const_upload_info is calculated precisely (considering actual >> constant set length), boolean matrix setting fall in direct copy path, unless >> transpose is required. The case of incomplete last row for matrix should be >> handled in set_constants() for transposed boolean constant only. >> >> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> >> --- >> v5: >> - added check for constant size not exceeding parameter size. The check for >> non array, non struct constants should be sufficient as the dimensions of >> arrays and structs are derived from const_set data and register counts >> from constant description is not used for them. > I don't understand what you mean here. > >> @@ -1417,6 +1433,16 @@ static HRESULT init_set_constants_param(struct d3dx_const_tab *const_tab, ID3DXC >> && !info.transpose && info.minor == info.major_stride >> && info.count == get_offset_reg(const_set.table, const_set.register_count) >> && info.count * sizeof(unsigned int) <= param->bytes; >> + if (info.minor_remainder && !const_set.direct_copy && !info.transpose) >> + FIXME("Incomplete last row for not transposed matrix which cannot be directly copied, parameter %s.\n", >> + debugstr_a(param->name)); >> + >> + if (info.major_count > info.major >> + || (info.major_count == info.major && info.minor_remainder)) >> + { >> + WARN("Constant dimensions exceed parameter size.\n"); >> + return D3DERR_INVALIDCALL; >> + } > It seems to me that this catches all the problematic cases. > > Yes, that what I mean above :) I tried to explain why I don't need a separate check that for arrays and structs, but probably the explanation is less clear than code itself.

June 27, 2017

Re: [v5 2/3] d3dx9: Get rid of constant's length checking for matrix in set_constants().

by Matteo Bruni

2017-06-22 15:11 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > When count in const_upload_info is calculated precisely (considering actual > constant set length), boolean matrix setting fall in direct copy path, unless > transpose is required. The case of incomplete last row for matrix should be > handled in set_constants() for transposed boolean constant only. > > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > v5: > - added check for constant size not exceeding parameter size. The check for > non array, non struct constants should be sufficient as the dimensions of > arrays and structs are derived from const_set data and register counts > from constant description is not used for them. I don't understand what you mean here. > @@ -1417,6 +1433,16 @@ static HRESULT init_set_constants_param(struct d3dx_const_tab *const_tab, ID3DXC > && !info.transpose && info.minor == info.major_stride > && info.count == get_offset_reg(const_set.table, const_set.register_count) > && info.count * sizeof(unsigned int) <= param->bytes; > + if (info.minor_remainder && !const_set.direct_copy && !info.transpose) > + FIXME("Incomplete last row for not transposed matrix which cannot be directly copied, parameter %s.\n", > + debugstr_a(param->name)); > + > + if (info.major_count > info.major > + || (info.major_count == info.major && info.minor_remainder)) > + { > + WARN("Constant dimensions exceed parameter size.\n"); > + return D3DERR_INVALIDCALL; > + } It seems to me that this catches all the problematic cases.

June 27, 2017

Re: [v5 1/3] d3dx9: Remove redundant parameter size check in set_constants().

by Matteo Bruni

2017-06-22 15:11 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > v5: > - Removed assert for parameter data length. The check for constant size not exceeding parameter size > is in the next patch. Otherwise the code would be changed right in the next patch after modifying > get_const_upload_info(), while before that the check is redundant. Yeah, it would have been slightly preferable with the check in a separate patch before this one. The check would have been redundant in practice but that's a bit of an "implementation detail". Certainly not a big deal though.

June 27, 2017

Re: [PATCH 2/2] d3drm: Store animation options

by Nikolay Sivov

On 27.06.2017 17:43, Henri Verbeet wrote: > On 27 June 2017 at 16:33, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> On 27.06.2017 15:43, Henri Verbeet wrote: >>> On 27 June 2017 at 10:17, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >>>> +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options) >>>> +{ >>>> + struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface); >>>> + static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION >>>> + | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION; >>>> + >>>> + TRACE("iface %p, options %#x.\n", iface, options); >>>> + >>>> + if (options && !(options & supported_options)) >>>> + return D3DRMERR_BADVALUE; >>> Should that be "if (options & ~supported_options)"? The test is >>> ambiguous about that. >> >> No, they both are wrong. It should be just "if (!(options & >> supported_options)", because 0 is not allowed. >> > What about unrecognised options? E.g., would 0x80000001 be allowed or not? Yes, it is allowed, and stored as is - unsupported bits are not filtered out. So patch v2 works correctly in that regard, only additional test is missing. > >> Bottom line is spline and linear mode bits are bad design, >> > Yeah, but that goes for much of ddraw/d3d up to about version 9 or 10. >

June 27, 2017

Re: [PATCH 2/2] d3drm: Store animation options

by Henri Verbeet

On 27 June 2017 at 16:33, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On 27.06.2017 15:43, Henri Verbeet wrote: >> On 27 June 2017 at 10:17, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >>> +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options) >>> +{ >>> + struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface); >>> + static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION >>> + | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION; >>> + >>> + TRACE("iface %p, options %#x.\n", iface, options); >>> + >>> + if (options && !(options & supported_options)) >>> + return D3DRMERR_BADVALUE; >> Should that be "if (options & ~supported_options)"? The test is >> ambiguous about that. > > No, they both are wrong. It should be just "if (!(options & > supported_options)", because 0 is not allowed. > What about unrecognised options? E.g., would 0x80000001 be allowed or not? > Bottom line is spline and linear mode bits are bad design, > Yeah, but that goes for much of ddraw/d3d up to about version 9 or 10.

June 27, 2017

Re: [PATCH 1/2] d3drm: Store animated frame pointer in animation object

by Nikolay Sivov

On 27.06.2017 15:43, Henri Verbeet wrote: > On 27 June 2017 at 10:17, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> static HRESULT WINAPI d3drm_animation1_SetFrame(IDirect3DRMAnimation *iface, IDirect3DRMFrame *frame) >> { >> - FIXME("iface %p, frame %p.\n", iface, frame); >> + struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation(iface); >> + HRESULT hr = D3DRM_OK; >> >> - return E_NOTIMPL; >> + TRACE("iface %p, frame %p.\n", iface, frame); >> + >> + if (frame) >> + { >> + hr = IDirect3DRMFrame_QueryInterface(frame, &IID_IDirect3DRMFrame3, (void **)&animation->frame); >> + if (SUCCEEDED(hr)) >> + IDirect3DRMFrame3_Release(animation->frame); >> + } >> + else >> + animation->frame = NULL; >> + >> + return hr; >> } > There's nothing necessarily wrong with this, but I feel the following > alternative is worth pointing out: > > struct d3drm_frame *f = unsafe_impl_from_IDirect3DRMFrame(frame); > ... > animation->frame = f ? &f->IDirect3DRMFrame3_iface : NULL; > > return D3DRM_OK; > > or even > > ... > return d3drm_animation2_SetFrame(&animation->IDirect3DRMAnimation2_iface, > f ? &f->IDirect3DRMFrame3_iface : NULL); > > which is a pattern that's used in ddraw for this kind of thing. > > That makes sense, I usually avoid unsafe_* stuff when public methods are enough. I'll resend as is, to send updated patch 2/2, but will probably patch it later in a way you suggested.

June 27, 2017

Re: [PATCH 2/2] d3drm: Store animation options

by Nikolay Sivov

On 27.06.2017 15:43, Henri Verbeet wrote: > On 27 June 2017 at 10:17, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options) >> +{ >> + struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface); >> + static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION >> + | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION; >> + >> + TRACE("iface %p, options %#x.\n", iface, options); >> + >> + if (options && !(options & supported_options)) >> + return D3DRMERR_BADVALUE; > Should that be "if (options & ~supported_options)"? The test is > ambiguous about that. No, they both are wrong. It should be just "if (!(options & supported_options)", because 0 is not allowed. > >> @@ -6776,6 +6777,42 @@ static void test_animation(void) >> IDirect3DRMFrame3_Release(frame3); >> IDirect3DRMFrame_Release(frame); >> >> + /* Animation options. */ >> + options = IDirect3DRMAnimation_GetOptions(animation); >> + ok(options == (D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION), >> + "Unexpected default options %#x.\n", options); >> + >> + /* Undefined mask value */ >> + hr = IDirect3DRMAnimation_SetOptions(animation, 0xdeadbeef); >> + ok(hr == D3DRMERR_BADVALUE, "Unexpected hr %#x.\n", hr); > 0xdeadbeef is a bit of a poor test value here, since it will trigger > both the "D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED" and > "D3DRMANIMATION_LINEARPOSITION | D3DRMANIMATION_SPLINEPOSITION" paths > as well. Right. > > Is it valid to set 0 as an option? Is it valid to set e.g. > D3DRMANIMATION_OPEN, but not one of D3DRMANIMATION_LINEARPOSITION and > D3DRMANIMATION_SPLINEPOSITION? I.e., is it just invalid to set > conflicting options, or should always exactly one of OPEN/CLOSED and > LINEARPOSITION/SPLINEPOSITION be set? > Setting 0 is invalid. For the rest there's already a test setting only _OPEN. So yes, only conflicting combinations are checked, and I found another one actually - when both _SCALEANDROTATION and _POSITION are set. Bottom line is spline and linear mode bits are bad design, there's no need in two flags when you'll need one of two modes anyway; and you can disable both whatever this means.

June 27, 2017