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

Re: [v2 PATCH] oledb32: Support DBTYPE_DBTIMESTAMP to DBTYPE_WSTR in DataConvert
by Marvin 27 Jun '17

27 Jun '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31909 Your paranoid android. === wxppro (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w2003std (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === wvistau64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w2008s64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w7u (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w7pro64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w8 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w864 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w1064 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4

1 0

Re: [PATCH] oledb32: Support DBTYPE_DBTIMESTAMP to DBTYPE_WSTR in DataConvert
by Marvin 27 Jun '17

27 Jun '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31908 Your paranoid android. === wxppro (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w2003std (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === wvistau64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w2008s64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w7u (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w7pro64 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w8 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w864 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4 === w1064 (32 bit convert) === convert.c:1028: Test failed: got 4 convert.c:1037: Test failed: got 4 convert.c:1046: Test failed: got 4

1 0

Re: [PATCH] gdiplus: Scale widened dashes to the pen width.
by Vincent Povirk 27 Jun '17

27 Jun '17

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.

2 2

Fwd: Improvements to the heap allocator
by Andrea Canciani 27 Jun '17

27 Jun '17

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

1 0

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

27 Jun '17

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.

2 2

Re: [v5 1/3] d3dx9: Remove redundant parameter size check in set_constants().
by Matteo Bruni 27 Jun '17

27 Jun '17

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.

1 0

Re: [PATCH 2/2] d3drm: Store animation options
by Henri Verbeet 27 Jun '17

27 Jun '17

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. > @@ -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. 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?

2 3

Re: [PATCH 1/2] d3drm: Store animated frame pointer in animation object
by Henri Verbeet 27 Jun '17

27 Jun '17

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.

2 1

Re: [PATCH] gdiplus: Write API doc for GdipAddPathEllipse and GdipAddPathEllipseI
by Clemens Tamme 27 Jun '17

27 Jun '17

ok, i will rewrite so the same terms are used in the docs of GdipAddPathLine and GdipAddPathEllipse 2017-06-26 22:18 GMT+02:00 Marvin <testbot(a)winehq.org>: > Thank you for your contribution to Wine! > > This is an automated notification to let you know that your patch has > been reviewed and its status set to "Assigned". > > This means that the patch has been assigned to a reviewer. If the > reviewer signs off on the patch, it will be committed, otherwise > you'll receive advice on how to improve it. If you don't hear anything > after a couple of weeks, please resend the patch. >

1 0

Re: [PATCH v3 1/4] msi/tests: Add tests for external UI callback.
by Marvin 26 Jun '17

26 Jun '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31868 Your paranoid android. === wvistau64 (32 bit package) === package.c:9397: Test failed: MsiOpenPackage with valid db: in msg 0x0b000000 field 2: expected "", got "þþ" === wvistau64 (64 bit package) === package.c:9397: Test failed: MsiOpenPackage with valid db: in msg 0x0b000000 field 2: expected "", got "þþ"

1 0