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

Re: [PATCH 5/5] ddraw/tests: Properly declare matrices.

by Henri Verbeet

On 16 June 2014 15:09, Marvin <testbot(a)winehq.org> wrote: > === w8 (32 bit visual) === > visual.c:2619: Test failed: unexpected colortable on offscreen surface > > === w2000pro (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > === wxppro (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > === wvistau64 (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > === w2008s64 (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > === w7u (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > === w8 (32 bit d3d) === > d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag > > These are existing failures, for what it's worth.

June 16, 2014

Re: [PATCH 5/5] ddraw/tests: Properly declare matrices.

by Marvin

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=7524 Your paranoid android. === w8 (32 bit visual) === visual.c:2619: Test failed: unexpected colortable on offscreen surface === w2000pro (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag === wxppro (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag === wvistau64 (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag === w2008s64 (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag === w7u (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag === w8 (32 bit d3d) === d3d.c:1261: Test failed: Homogeneous output was generated despite UNCLIPPED flag

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Henri Verbeet

On 16 June 2014 14:15, Jacek Caban <jacek(a)codeweavers.com> wrote: > I don't know this how will this be used in the future, so my ideas may > not be best fit. For the code that you sent so far, a separated struct As far as the API is concerned, brushes are mostly used for drawing commands like e.g. ID2D1RenderTarget::FillRectangle() and the like. These all take ID2D1Brush pointers. The way the implementation is likely to work in the future is that brush types will have an associated fragment shader, and some constant buffer data. I.e., a typical drawing function implementation would do something along the lines of the following: struct d2d_brush *b = unsafe_impl_from_ID2DBrush(brush); ... ID3D10Device_PSSetShader(device, b->shader); ID3D10Device_PSSetConstantBuffers(device, 0, 1, &b->cb); ... ID3D10Device_DrawIndexed(device, ...); > for all brush types would do the job. I guess that different brush types > will store different data in the future, so separated structs will be > needed anyway. If having some sort of shared struct is desired, this > could be introduced as separated struct: > > struct d2d1_brash { > /* shared stuff, maybe even refcount */ > }; > > struct d2d1_solid_brush { > ID2D1SolidColorBrush ID2D1SolidColorBrush_iface > LONG refcount; > struct brush base_brush; > /* Stuff specific to solid brush */ > }; > > This has some drawbacks like the fact that d2d1_brush can't be easily > casted to ID2D1Brush. This may be avoided by design or, with some It's mostly the ID2D1Brush -> struct d2d_brush conversion that's the issue. If you do the above, you either need to do multiple QueryInterface() calls in each drawing function, or introduce a private interface to get at the embedded d2d_brush structure, and then implement that for every single brush. I think the two casts, while not very pretty, are worth it in comparison. > runtime overhead, d2d1_brush could store a pointer to ID2D1Brush (which > would be initialized to solid_brush->ID2D1SoludBrush_iface in this case) That would only help for getting an ID2D1Brush from struct d2d_brush, but even then, you'd just reintroduce the casts you were trying to avoid.

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Jacek Caban

On 06/16/14 13:59, Henri Verbeet wrote: > On 16 June 2014 13:57, Jacek Caban <jacek(a)codeweavers.com> wrote: >> Yup, and that patch has the same problem with parent to child interface >> casts. >> > What are you suggesting instead? I don't know this how will this be used in the future, so my ideas may not be best fit. For the code that you sent so far, a separated struct for all brush types would do the job. I guess that different brush types will store different data in the future, so separated structs will be needed anyway. If having some sort of shared struct is desired, this could be introduced as separated struct: struct d2d1_brash { /* shared stuff, maybe even refcount */ }; struct d2d1_solid_brush { ID2D1SolidColorBrush ID2D1SolidColorBrush_iface LONG refcount; struct brush base_brush; /* Stuff specific to solid brush */ }; This has some drawbacks like the fact that d2d1_brush can't be easily casted to ID2D1Brush. This may be avoided by design or, with some runtime overhead, d2d1_brush could store a pointer to ID2D1Brush (which would be initialized to solid_brush->ID2D1SoludBrush_iface in this case) or have some internal vtbl-like casting functions. Jacek

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Henri Verbeet

On 16 June 2014 13:57, Jacek Caban <jacek(a)codeweavers.com> wrote: > Yup, and that patch has the same problem with parent to child interface > casts. > What are you suggesting instead?

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Jacek Caban

On 06/16/14 13:53, Henri Verbeet wrote: > On 16 June 2014 13:44, Jacek Caban <jacek(a)codeweavers.com> wrote: >> Given how you use this object, this should be ID2D1SolidColorBrush. > No, struct d2d_brush is also used for e.g. 105100. Yup, and that patch has the same problem with parent to child interface casts. Jacek

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Henri Verbeet

On 16 June 2014 13:44, Jacek Caban <jacek(a)codeweavers.com> wrote: > Given how you use this object, this should be ID2D1SolidColorBrush. No, struct d2d_brush is also used for e.g. 105100.

June 16, 2014

Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().

by Jacek Caban

Hi Henri, On 06/16/14 13:13, Henri Verbeet wrote: > diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h > index 5609db6..9e91728 100644 > --- a/dlls/d2d1/d2d1_private.h > +++ b/dlls/d2d1/d2d1_private.h > @@ -33,4 +33,13 @@ struct d2d_d3d_render_target > void d2d_d3d_render_target_init(struct d2d_d3d_render_target *render_target, ID2D1Factory *factory, > IDXGISurface *surface, const D2D1_RENDER_TARGET_PROPERTIES *desc) DECLSPEC_HIDDEN; > > +struct d2d_brush > +{ > + ID2D1Brush ID2D1Brush_iface; Given how you use this object, this should be ID2D1SolidColorBrush. I guess you want to somehow reuse this struct for other brush types, but there should be better ways for this. Then: +void d2d_solid_color_brush_init(struct d2d_brush *brush, ID2D1RenderTarget *render_target, + const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc) +{ + FIXME("Ignoring brush properties.\n"); + + brush->ID2D1Brush_iface.lpVtbl = (ID2D1BrushVtbl *)&d2d_solid_color_brush_vtbl; + brush->refcount = 1; +} This cast should not be needed. > + LONG refcount; > +}; > + > +void d2d_solid_color_brush_init(struct d2d_brush *brush, ID2D1RenderTarget *render_target, > + const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc) DECLSPEC_HIDDEN; > + > #endif /* __WINE_D2D1_PRIVATE_H */ > diff --git a/dlls/d2d1/render_target.c b/dlls/d2d1/render_target.c > index be9c1b6..e8c56a0 100644 > --- a/dlls/d2d1/render_target.c > +++ b/dlls/d2d1/render_target.c > @@ -117,9 +117,19 @@ static HRESULT STDMETHODCALLTYPE d2d_d3d_render_target_CreateBitmapBrush(ID2D1Re > static HRESULT STDMETHODCALLTYPE d2d_d3d_render_target_CreateSolidColorBrush(ID2D1RenderTarget *iface, > const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc, ID2D1SolidColorBrush **brush) > { > - FIXME("iface %p, color %p, desc %p, brush %p stub!\n", iface, color, desc, brush); > + struct d2d_brush *object; > > - return E_NOTIMPL; > + TRACE("iface %p, color %p, desc %p, brush %p.\n", iface, color, desc, brush); > + > + if (!(object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*object)))) > + return E_OUTOFMEMORY; > + > + d2d_solid_color_brush_init(object, iface, color, desc); > + > + TRACE("Created brush %p.\n", object); > + *brush = (ID2D1SolidColorBrush *)&object->ID2D1Brush_iface; Same here, the cast should not be needed. Also allocating memory in brush.c seems more logical to me (although that's a matter of taste). Cheers, Jacek

June 16, 2014

Re: Fix for PlgBlt, the XFORM matrix was calculated incorrectly

by Sebastian Lackner

Hi guys, by comparing the "new" equations (http://www.winehq.org/pipermail/wine-devel/2009-December/080391.html) and the old ones (http://source.winehq.org/source/dlls/gdi32/bitblt.c) you'll find the following differences: eM11: identical eM21: identical eDx: identical eM12: identical eM22: plg <-> rect swapped eDy: identical It isn't obvious at the first look, but the only difference is that in eM22 plg and rect are swapped. By taking a bit closer look at ... http://source.winehq.org/source/dlls/gdi32/bitblt.c#L1040 ... and comparing the equations for the X-component (eM11, eM21) and the Y-component (eM12, eM22) you'll notice that this must be right, the current equation violate the symmetry - no need to calculate, you can see that with the naked eye ;) The correct equation for eM22 should be: xf.eM22 = (rect[1].x*(plg[2].y - plg[0].y) - rect[2].x*(plg[1].y - plg[0].y) - rect[0].x*(plg[2].y - plg[1].y)) / det; Could someone please test if this change alone is sufficient to fix the issue? Should be way easier to get that upstream than all the other "no-op" changes. Regards, Sebastian Am 16.06.2014 04:07, schrieb Michael Mc Donnell: > Hi Alexander > > On Wed, Jun 11, 2014 at 5:36 AM, Alexander Almaleh <sashoalm(a)gmail.com> wrote: >> >> I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows. > > I do not understand how that would stop you from writing an automated > test? You can use todo_wine to show how the code is supposed to work. > You can then cross compile the test and upload it to the Wine TestBot. > The Wine TestBot will run your test on Windows and report back if it > is correct. The automated test can then be applied to the wine source > tree. Once your patch with the fix is ready you can submit it together > with a patch that removes the todo_wine. > >> Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix? > > I think you will need to be able to explain why the old code is wrong > and explain why yours is better. Explaining what the formula is might > be a good way to *help* explain it. It is important that your code > does not break anything. You have found one example where your code > works better but it might break other examples. > > I see Nikolay Sivov, the author of the function, has already given you > feedback earlier [1]. He will probably be one of the people you need > to convince. When he said: > > "P.S. I don't see your point on inlining determinant expression." > > He probably means you should rewrite your equation to use the > determinant, unless you have a very good reason, in which case you > need to explain it. I personally think you might be able to fix his > code instead of replacing it. > > Good luck, > Michael > > [1] http://www.winehq.org/pipermail/wine-devel/2009-December/080392.html > >

June 16, 2014

Re: Fix for PlgBlt, the XFORM matrix was calculated incorrectly

by Michael Mc Donnell

Hi Alexander On Wed, Jun 11, 2014 at 5:36 AM, Alexander Almaleh <sashoalm(a)gmail.com> wrote: > > I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows. I do not understand how that would stop you from writing an automated test? You can use todo_wine to show how the code is supposed to work. You can then cross compile the test and upload it to the Wine TestBot. The Wine TestBot will run your test on Windows and report back if it is correct. The automated test can then be applied to the wine source tree. Once your patch with the fix is ready you can submit it together with a patch that removes the todo_wine. > Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix? I think you will need to be able to explain why the old code is wrong and explain why yours is better. Explaining what the formula is might be a good way to *help* explain it. It is important that your code does not break anything. You have found one example where your code works better but it might break other examples. I see Nikolay Sivov, the author of the function, has already given you feedback earlier [1]. He will probably be one of the people you need to convince. When he said: "P.S. I don't see your point on inlining determinant expression." He probably means you should rewrite your equation to use the determinant, unless you have a very good reason, in which case you need to explain it. I personally think you might be able to fix his code instead of replacing it. Good luck, Michael [1] http://www.winehq.org/pipermail/wine-devel/2009-December/080392.html

June 16, 2014