[PATCH v11 0/3] MR1164: Draft: gdiplus: add initial CompoundLine support to fix application crash

List overview All Threads

newer

older

[PATCH v12 0/3] MR1164: gdiplus:...

[PATCH v6 0/5] MR741: ntdll:...

Bartosz Kosiorek (＠gang65)

27 Oct 2022 27 Oct '22

8:35 a.m.

Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=52196

-- v11: gdiplus: add GdipGetPenCompoundArray implementation gdiplus: add GdipGetPenCompoundCount implementation gdiplus: add GdipSetPenCompoundArray implementation

https://gitlab.winehq.org/wine/wine/-/merge_requests/1164

Show replies by date

Bartosz Kosiorek

27 Oct 27 Oct

8:35 a.m.

New subject: [PATCH v11 1/3] gdiplus: add GdipSetPenCompoundArray implementation

From: Bartosz Kosiorek gang65@poczta.onet.pl

Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=52196 --- dlls/gdiplus/gdiplus_private.h | 2 ++ dlls/gdiplus/graphicspath.c | 3 +++ dlls/gdiplus/pen.c | 29 +++++++++++++++++++++++++---- dlls/gdiplus/tests/pen.c | 12 +++++++++++- 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h index 03bfa5513cb..559944a770c 100644 --- a/dlls/gdiplus/gdiplus_private.h +++ b/dlls/gdiplus/gdiplus_private.h @@ -235,6 +235,8 @@ struct GpPen{ GpBrush *brush; GpPenAlignment align; GpMatrix transform; + REAL *compound_array; + UINT compound_array_size; };

struct GpGraphics{ diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c index ce2666eedab..d98a5ae4bf6 100644 --- a/dlls/gdiplus/graphicspath.c +++ b/dlls/gdiplus/graphicspath.c @@ -2356,6 +2356,9 @@ GpStatus WINGDIPAPI GdipWidenPath(GpPath *path, GpPen *pen, GpMatrix *matrix, if (pen->align != PenAlignmentCenter) FIXME("unimplemented pen alignment %d\n", pen->align);

+ if (pen->compound_array_size != 0) + FIXME("unimplemented pen compoundline. Solid line will be drawn instead: %d\n", pen->compound_array_size); + for (i=0; i < flat_path->pathdata.Count; i++) { if ((types[i]&PathPointTypePathTypeMask) == PathPointTypeStart) diff --git a/dlls/gdiplus/pen.c b/dlls/gdiplus/pen.c index 32579adc484..46d95bcec22 100644 --- a/dlls/gdiplus/pen.c +++ b/dlls/gdiplus/pen.c @@ -171,6 +171,8 @@ GpStatus WINGDIPAPI GdipCreatePen2(GpBrush *brush, REAL width, GpUnit unit, gp_pen->offset = 0.0; gp_pen->customstart = NULL; gp_pen->customend = NULL; + gp_pen->compound_array = NULL; + gp_pen->compound_array_size = 0; GdipSetMatrixElements(&gp_pen->transform, 1.0, 0.0, 0.0, 1.0, 0.0, 0.0);

if(!((gp_pen->unit == UnitWorld) || (gp_pen->unit == UnitPixel))) { @@ -198,6 +200,7 @@ GpStatus WINGDIPAPI GdipDeletePen(GpPen *pen) GdipDeleteBrush(pen->brush); GdipDeleteCustomLineCap(pen->customstart); GdipDeleteCustomLineCap(pen->customend); + heap_free(pen->compound_array); heap_free(pen->dashes); heap_free(pen);

@@ -533,15 +536,33 @@ GpStatus WINGDIPAPI GdipGetPenCompoundCount(GpPen *pen, INT *count) return NotImplemented; }

-GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *dash, +GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *compoundarray, INT count) { - FIXME("(%p, %p, %i): stub\n", pen, dash, count); + REAL previousValue; + TRACE("(%p, %p, %i)\n", pen, compoundarray, count);

- if (!pen || !dash || count < 2 || count%2 == 1) + if(!pen || !compoundarray || count < 2 || count%2 == 1 || *compoundarray < 0.0 || *compoundarray > 1.0) return InvalidParameter; + previousValue = *compoundarray;

- return NotImplemented; + for(size_t i = 1; i<count; i++) + { + if((compoundarray[i] < previousValue) || (compoundarray[i] > 1.0)) + return InvalidParameter; + previousValue = compoundarray[i]; + } + + heap_free(pen->compound_array); + pen->compound_array = heap_alloc_zero(count * sizeof(REAL)); + if(!pen->compound_array) + { + pen->compound_array_size = 0; + return OutOfMemory; + } + memcpy(pen->compound_array, compoundarray, count * sizeof(REAL)); + pen->compound_array_size = count; + return Ok; }

GpStatus WINGDIPAPI GdipSetPenCustomEndCap(GpPen *pen, GpCustomLineCap* customCap) diff --git a/dlls/gdiplus/tests/pen.c b/dlls/gdiplus/tests/pen.c index cd3e45f9b41..d1d19312878 100644 --- a/dlls/gdiplus/tests/pen.c +++ b/dlls/gdiplus/tests/pen.c @@ -351,6 +351,9 @@ static void test_compoundarray(void) GpStatus status; GpPen *pen; static const REAL testvalues[] = {0.2, 0.4, 0.6, 0.8}; + static const REAL notSortedValues[] = {0.2, 0.6, 0.4, 0.8}; + static const REAL negativeValues[] = {-1.2, 0.4, 0.6, 0.8}; + static const REAL tooLargeValues[] = {0.2, 0.4, 0.6, 2.8}; INT count;

status = GdipSetPenCompoundArray(NULL, testvalues, 4); @@ -382,8 +385,15 @@ todo_wine { status = GdipSetPenCompoundArray(pen, testvalues, -2); expect(InvalidParameter, status);

+ status = GdipSetPenCompoundArray(pen, notSortedValues, 4); + expect(InvalidParameter, status); + status = GdipSetPenCompoundArray(pen, negativeValues, 4); + expect(InvalidParameter, status); + status = GdipSetPenCompoundArray(pen, tooLargeValues, 4); + expect(InvalidParameter, status); + status = GdipSetPenCompoundArray(pen, testvalues, 4); - todo_wine expect(Ok, status); + expect(Ok, status); status = GdipSetPenCompoundArray(pen, NULL, 0); expect(InvalidParameter, status);

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1164

Bartosz Kosiorek

8:35 a.m.

New subject: [PATCH v11 2/3] gdiplus: add GdipGetPenCompoundCount implementation

From: Bartosz Kosiorek gang65@poczta.onet.pl

Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=52196 --- dlls/gdiplus/pen.c | 6 +++--- dlls/gdiplus/tests/pen.c | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/dlls/gdiplus/pen.c b/dlls/gdiplus/pen.c index 46d95bcec22..52f0db5d069 100644 --- a/dlls/gdiplus/pen.c +++ b/dlls/gdiplus/pen.c @@ -528,12 +528,12 @@ GpStatus WINGDIPAPI GdipSetPenColor(GpPen *pen, ARGB argb)

GpStatus WINGDIPAPI GdipGetPenCompoundCount(GpPen *pen, INT *count) { - FIXME("(%p, %p): stub\n", pen, count); + TRACE("(%p, %p)\n", pen, count);

if (!pen || !count) return InvalidParameter; - - return NotImplemented; + *count = pen->compound_array_size; + return Ok; }

GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *compoundarray, diff --git a/dlls/gdiplus/tests/pen.c b/dlls/gdiplus/tests/pen.c index d1d19312878..9305cf058ab 100644 --- a/dlls/gdiplus/tests/pen.c +++ b/dlls/gdiplus/tests/pen.c @@ -370,10 +370,9 @@ static void test_compoundarray(void)

count = 10; status = GdipGetPenCompoundCount(pen, &count); -todo_wine { expect(Ok, status); ok(count == 0, "Unexpected compound count %d\n", count); -} + status = GdipSetPenCompoundArray(pen, NULL, 0); expect(InvalidParameter, status); status = GdipSetPenCompoundArray(pen, NULL, 4);

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1164

Bartosz Kosiorek

8:35 a.m.

New subject: [PATCH v11 3/3] gdiplus: add GdipGetPenCompoundArray implementation

From: Bartosz Kosiorek gang65@poczta.onet.pl

Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=52196 --- dlls/gdiplus/gdiplus.spec | 2 +- dlls/gdiplus/pen.c | 14 +++++++++++++- dlls/gdiplus/tests/pen.c | 20 ++++++++++++++++++-- include/gdiplusflat.h | 1 + 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/dlls/gdiplus/gdiplus.spec b/dlls/gdiplus/gdiplus.spec index 15d9b5d81e8..d4545abf319 100644 --- a/dlls/gdiplus/gdiplus.spec +++ b/dlls/gdiplus/gdiplus.spec @@ -334,7 +334,7 @@ 334 stdcall GdipGetPathWorldBoundsI(ptr ptr ptr ptr) 335 stdcall GdipGetPenBrushFill(ptr ptr) 336 stdcall GdipGetPenColor(ptr ptr) -337 stub GdipGetPenCompoundArray +337 stdcall GdipGetPenCompoundArray(ptr ptr long) 338 stdcall GdipGetPenCompoundCount(ptr ptr) 339 stdcall GdipGetPenCustomEndCap(ptr ptr) 340 stdcall GdipGetPenCustomStartCap(ptr ptr) diff --git a/dlls/gdiplus/pen.c b/dlls/gdiplus/pen.c index 52f0db5d069..f5c5d13382a 100644 --- a/dlls/gdiplus/pen.c +++ b/dlls/gdiplus/pen.c @@ -195,7 +195,8 @@ GpStatus WINGDIPAPI GdipDeletePen(GpPen *pen) { TRACE("(%p)\n", pen);

- if(!pen) return InvalidParameter; + if(!pen) + return InvalidParameter;

GdipDeleteBrush(pen->brush); GdipDeleteCustomLineCap(pen->customstart); @@ -526,6 +527,17 @@ GpStatus WINGDIPAPI GdipSetPenColor(GpPen *pen, ARGB argb) return GdipSetSolidFillColor(((GpSolidFill*)pen->brush), argb); }

+GpStatus WINGDIPAPI GdipGetPenCompoundArray(GpPen *pen, REAL *compoundarray, INT count) +{ + TRACE("(%p, %p, %i)\n", pen, compoundarray, count); + + if (!pen || !compoundarray || count > pen->compound_array_size) + return InvalidParameter; + if (pen->compound_array && count) + memcpy(compoundarray, pen->compound_array, count * sizeof(REAL)); + return Ok; +} + GpStatus WINGDIPAPI GdipGetPenCompoundCount(GpPen *pen, INT *count) { TRACE("(%p, %p)\n", pen, count); diff --git a/dlls/gdiplus/tests/pen.c b/dlls/gdiplus/tests/pen.c index 9305cf058ab..2e0da329604 100644 --- a/dlls/gdiplus/tests/pen.c +++ b/dlls/gdiplus/tests/pen.c @@ -350,6 +350,7 @@ static void test_compoundarray(void) { GpStatus status; GpPen *pen; + REAL *returnvalues; static const REAL testvalues[] = {0.2, 0.4, 0.6, 0.8}; static const REAL notSortedValues[] = {0.2, 0.6, 0.4, 0.8}; static const REAL negativeValues[] = {-1.2, 0.4, 0.6, 0.8}; @@ -398,10 +399,25 @@ static void test_compoundarray(void)

count = 0; status = GdipGetPenCompoundCount(pen, &count); -todo_wine { expect(Ok, status); ok(count == 4, "Unexpected compound count %d\n", count); -} + + returnvalues = calloc(5, sizeof(REAL)); + // When count larger than stored array return error + status = GdipGetPenCompoundArray(pen, returnvalues, 40); + expect(InvalidParameter, status); + status = GdipGetPenCompoundArray(NULL, returnvalues, 4); + expect(InvalidParameter, status); + // When count is zero, it should do nothing + status = GdipGetPenCompoundArray(pen, returnvalues, 0); + expect(Ok, status); + ok(returnvalues[0] == 0.0, "Unexpected compound array %f\n", returnvalues[0]); + + status = GdipGetPenCompoundArray(pen, returnvalues, 4); + expect(Ok, status); + ok(memcmp(returnvalues, testvalues, 4 * sizeof(REAL)) == 0, "Unexpected compound array\n"); + + free(returnvalues); GdipDeletePen(pen); }

diff --git a/include/gdiplusflat.h b/include/gdiplusflat.h index 5c9600ed30b..10f4d435a85 100644 --- a/include/gdiplusflat.h +++ b/include/gdiplusflat.h @@ -629,6 +629,7 @@ GpStatus WINGDIPAPI GdipCreatePen2(GpBrush*,REAL,GpUnit,GpPen**); GpStatus WINGDIPAPI GdipDeletePen(GpPen*); GpStatus WINGDIPAPI GdipGetPenBrushFill(GpPen*,GpBrush**); GpStatus WINGDIPAPI GdipGetPenColor(GpPen*,ARGB*); +GpStatus WINGDIPAPI GdipGetPenCompoundArray(GpPen*,REAL*,INT); GpStatus WINGDIPAPI GdipGetPenCompoundCount(GpPen*,INT*); GpStatus WINGDIPAPI GdipGetPenCustomStartCap(GpPen*,GpCustomLineCap**); GpStatus WINGDIPAPI GdipGetPenCustomEndCap(GpPen*,GpCustomLineCap**);

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1164

Marvin

8:57 a.m.

New subject: [PATCH v11 3/3] gdiplus: add GdipGetPenCompoundArray implementation

Hi,

It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.

23 TestBot errors prevented a full analysis of your patch. If the test caused the operating system (e.g. Windows) to crash or reboot you will probably have to modify it to avoid that. Other issues should be reported to the TestBot administrators.

The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125479

Your paranoid android.

=== w7u_2qxl (testbot log) ===

WineRunTask.pl:error: Could not copy the test executable to the VM: Unable to open '/home/testbot/var/jobs/125479/gdiplus_test.exe' for reading: No such file or directory WineRunTask.pl:error: BotError: The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) WineRunTask.pl:error: Giving up after 3 run(s)

=== w7u_adm (testbot log) ===

=== w7u_el (testbot log) ===

=== w8 (testbot log) ===

=== w8adm (testbot log) ===

=== w864 (testbot log) ===

=== w1064v1507 (testbot log) ===

=== w1064v1809 (testbot log) ===

=== w1064 (testbot log) ===

=== w1064_tsign (testbot log) ===

=== w10pro64 (testbot log) ===

=== w864 (testbot log) ===

WineRunTask.pl:error: Could not copy the test executable to the VM: Unable to open '/home/testbot/var/jobs/125479/gdiplus_test64.exe' for reading: No such file or directory WineRunTask.pl:error: BotError: The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) WineRunTask.pl:error: Giving up after 3 run(s)

=== w1064v1507 (testbot log) ===

=== w1064v1809 (testbot log) ===

=== w1064 (testbot log) ===

=== w1064_2qxl (testbot log) ===

=== w1064_adm (testbot log) ===

=== w1064_tsign (testbot log) ===

=== w10pro64 (testbot log) ===

=== w10pro64_en_AE_u8 (testbot log) ===

=== w10pro64_ar (testbot log) ===

=== w10pro64_ja (testbot log) ===

=== w10pro64_zh_CN (testbot log) ===

=== debian11 (build log) ===

Task: WineTest did not produce the win32 report

=== debian11 (build log) ===

Task: WineTest did not produce the wow32 report

Esme Povirk (＠madewokherd)

28 Oct 28 Oct

6:30 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/gdiplus_private.h:

...

 GpBrush *brush;
 GpPenAlignment align;
 GpMatrix transform;
REAL *compound_array;

UINT compound_array_size;

This should probably match the type passed into `GdipSetPenCompoundArray`. Things can get weird in C when you compare between signed and unsigned types.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12391

Esme Povirk (＠madewokherd)

6:31 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pen.c:

...

-GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *dash, +GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *compoundarray, INT count) {

FIXME("(%p, %p, %i): stub\n", pen, dash, count);

REAL previousValue;

TRACE("(%p, %p, %i)\n", pen, compoundarray, count);

if (!pen || !dash || count < 2 || count%2 == 1)

if(!pen || !compoundarray || count < 2 || count%2 == 1 || *compoundarray < 0.0 || *compoundarray > 1.0) return InvalidParameter;

previousValue = *compoundarray;

return NotImplemented;

for(size_t i = 1; i<count; i++)

It's not typical Wine style to declare a variable inside a for statement. I think it's because of C89 compatibility.

The type of the variable should match what it's comparing to, in this case count. As-is, this would be a signed/unsigned comparison.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12392

Esme Povirk (＠madewokherd)

6:31 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pen.c:

...

 return NotImplemented;
}

-GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *dash, +GpStatus WINGDIPAPI GdipSetPenCompoundArray(GpPen *pen, GDIPCONST REAL *compoundarray, INT count) {

FIXME("(%p, %p, %i): stub\n", pen, dash, count);

REAL previousValue;

TRACE("(%p, %p, %i)\n", pen, compoundarray, count);

if (!pen || !dash || count < 2 || count%2 == 1)

if(!pen || !compoundarray || count < 2 || count%2 == 1 || *compoundarray < 0.0 || *compoundarray > 1.0) return InvalidParameter;

previousValue = *compoundarray;

This is more complicated than accessing compoundarray[i-1] in the loop. I think it makes the code harder to read.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12393

Esme Povirk (＠madewokherd)

6:31 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pen.c:

...

TRACE("(%p, %p, %i)\n", pen, compoundarray, count);

if (!pen || !dash || count < 2 || count%2 == 1)

if(!pen || !compoundarray || count < 2 || count%2 == 1 || *compoundarray < 0.0 || *compoundarray > 1.0) return InvalidParameter;

previousValue = *compoundarray;

return NotImplemented;
for(size_t i = 1; i<count; i++)

{
   if((compoundarray[i] < previousValue) || (compoundarray[i] > 1.0))
       return InvalidParameter;
   previousValue = compoundarray[i];
}

heap_free(pen->compound_array);

I've been told in the past that an old array shouldn't be freed until we have successfully allocated a new array to replace it.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12394

Esme Povirk (＠madewokherd)

6:31 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pen.c:

...

{ TRACE("(%p)\n", pen);

if(!pen) return InvalidParameter;
if(!pen)
   return InvalidParameter;

Please don't include unrelated changes.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12395

Esme Povirk (＠madewokherd)

6:31 p.m.

New subject: [PATCH v11 0/3] MR1164: gdiplus: add initial CompoundLine support to fix application crash

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pen.c:

...

 return GdipSetSolidFillColor(((GpSolidFill*)pen->brush), argb);
}

+GpStatus WINGDIPAPI GdipGetPenCompoundArray(GpPen *pen, REAL *compoundarray, INT count) +{

TRACE("(%p, %p, %i)\n", pen, compoundarray, count);

if (!pen || !compoundarray || count > pen->compound_array_size)

We should probably also check for count being negative.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1164#note_12396