Re: gdiplus: Create a GDI brush only when needed.

[Vincent Povirk]

This won't work. We create gdi32 brushes for hatch brushes based on
temporary bitmaps that won't exist at the time brush_fill_path is
called.

On Wed, Feb 15, 2012 at 2:44 AM, Dmitry Timoshkov  wrote:
> This patch fixes the problem reported in the bug 29894.
>
> dlls/gdiplus/brush.c           |   19 -------------------
>  dlls/gdiplus/gdiplus_private.h |    1 -
>  dlls/gdiplus/graphics.c        |   13 ++++++++++++-
>  3 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/dlls/gdiplus/brush.c b/dlls/gdiplus/brush.c
> index d1af903..e0c400e 100644
> --- a/dlls/gdiplus/brush.c
> +++ b/dlls/gdiplus/brush.c
> @@ -55,7 +55,6 @@ GpStatus WINGDIPAPI GdipCloneBrush(GpBrush *brush, GpBrush **clone)
>
> memcpy(*clone, brush, sizeof(GpSolidFill));
> - (*clone)->gdibrush = CreateBrushIndirect(&(*clone)->lb);
>
> fill->bmp = ARGB2BMP(fill->color);
>             break;
>         }
> @@ -123,8 +122,6 @@ GpStatus WINGDIPAPI GdipCloneBrush(GpBrush *brush, GpBrush **clone)
>
> memcpy(dest, src, sizeof(GpLineGradient));
> - dest->brush.gdibrush = CreateSolidBrush(dest->brush.lb.lbColor);
> -
>
> count = dest->blendcount;
>             dest->blendfac = GdipAlloc(count * sizeof(REAL));
>             dest->blendpos = GdipAlloc(count * sizeof(REAL));
> @@ -142,7 +139,6 @@ GpStatus WINGDIPAPI GdipCloneBrush(GpBrush *brush, GpBrush **clone)
>                 GdipFree(dest->blendpos);
>                 GdipFree(dest->pblendcolor);
>                 GdipFree(dest->pblendpos);
> - DeleteObject(dest->brush.gdibrush);
>
> GdipFree(dest);
>                 return OutOfMemory;
>             }
> @@ -295,7 +291,6 @@ GpStatus WINGDIPAPI GdipCreateHatchBrush(HatchStyle hatchstyle, ARGB forecol, AR
>             (*brush)->brush.lb.lbStyle = BS_PATTERN;
>             (*brush)->brush.lb.lbColor = 0;
>             (*brush)->brush.lb.lbHatch = (ULONG_PTR)hbmp;
> - (*brush)->brush.gdibrush = CreateBrushIndirect(&(*brush)->brush.lb);
>
> DeleteObject(hbmp);
>         }
> @@ -307,7 +302,6 @@ GpStatus WINGDIPAPI GdipCreateHatchBrush(HatchStyle hatchstyle, ARGB forecol, AR
>         (*brush)->brush.lb.lbStyle = BS_SOLID;
>         (*brush)->brush.lb.lbColor = fgcol;
>         (*brush)->brush.lb.lbHatch = 0;
> - (*brush)->brush.gdibrush = CreateBrushIndirect(&(*brush)->brush.lb);
>
> }
>
> if (stat == Ok)
> @@ -351,7 +345,6 @@ GpStatus WINGDIPAPI GdipCreateLineBrush(GDIPCONST GpPointF* startpoint,
>     (*line)->brush.lb.lbStyle = BS_SOLID;
>     (*line)->brush.lb.lbColor = col;
>     (*line)->brush.lb.lbHatch = 0;
> - (*line)->brush.gdibrush = CreateSolidBrush(col);
>
> (*line)->brush.bt = BrushTypeLinearGradient;
>
> (*line)->startpoint.X = startpoint->X;
> @@ -387,7 +380,6 @@ GpStatus WINGDIPAPI GdipCreateLineBrush(GDIPCONST GpPointF* startpoint,
>     {
>         GdipFree((*line)->blendfac);
>         GdipFree((*line)->blendpos);
> - DeleteObject((*line)->brush.gdibrush);
>
> GdipFree(*line);
>         *line = NULL;
>         return OutOfMemory;
> @@ -624,7 +616,6 @@ GpStatus WINGDIPAPI GdipCreatePathGradient(GDIPCONST GpPointF* points,
>     (*grad)->brush.lb.lbColor = col;
>     (*grad)->brush.lb.lbHatch = 0;
> - (*grad)->brush.gdibrush = CreateSolidBrush(col);
>
> (*grad)->brush.bt = BrushTypePathGradient;
>     (*grad)->centercolor = 0xffffffff;
>     (*grad)->wrap = wrap;
> @@ -719,7 +710,6 @@ GpStatus WINGDIPAPI GdipCreatePathGradientFromPath(GDIPCONST GpPath* path,
>     (*grad)->brush.lb.lbColor = col;
>     (*grad)->brush.lb.lbHatch = 0;
> - (*grad)->brush.gdibrush = CreateSolidBrush(col);
>
> (*grad)->brush.bt = BrushTypePathGradient;
>     (*grad)->centercolor = 0xffffffff;
>     (*grad)->wrap = WrapModeClamp;
> @@ -753,7 +743,6 @@ GpStatus WINGDIPAPI GdipCreateSolidFill(ARGB color, GpSolidFill **sf)
>     (*sf)->brush.lb.lbColor = col;
>     (*sf)->brush.lb.lbHatch = 0;
> - (*sf)->brush.gdibrush = CreateSolidBrush(col);
>
> (*sf)->brush.bt = BrushTypeSolidColor;
>     (*sf)->color = color;
>     (*sf)->bmp = ARGB2BMP(color);
> @@ -898,7 +887,6 @@ GpStatus WINGDIPAPI GdipCreateTextureIA(GpImage *image,
>     (*texture)->brush.lb.lbColor = 0;
>     (*texture)->brush.lb.lbHatch = (ULONG_PTR)hbm;
> - (*texture)->brush.gdibrush = CreateBrushIndirect(&(*texture)->brush.lb);
>
> (*texture)->brush.bt = BrushTypeTextureFill;
>     (*texture)->image = new_image;
>
> @@ -1036,7 +1024,6 @@ GpStatus WINGDIPAPI GdipDeleteBrush(GpBrush *brush)
>             break;
>     }
> - DeleteObject(brush->gdibrush);
>
> GdipFree(brush);
>
> return Ok;
> @@ -1578,9 +1565,6 @@ GpStatus WINGDIPAPI GdipSetPathGradientCenterColor(GpPathGradient *grad,
>     grad->centercolor = argb;
>     grad->brush.lb.lbColor = ARGB2COLORREF(argb);
> - DeleteObject(grad->brush.gdibrush);
> - grad->brush.gdibrush = CreateSolidBrush(grad->brush.lb.lbColor);
> -
>
> return Ok;
>  }
>
> @@ -1775,9 +1759,6 @@ GpStatus WINGDIPAPI GdipSetSolidFillColor(GpSolidFill *sf, ARGB argb)
>     sf->color = argb;
>     sf->brush.lb.lbColor = ARGB2COLORREF(argb);
> - DeleteObject(sf->brush.gdibrush);
> - sf->brush.gdibrush = CreateSolidBrush(sf->brush.lb.lbColor);
> -
>
> return Ok;
>  }
>
> diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
> index 9afe15b..b597511 100644
> --- a/dlls/gdiplus/gdiplus_private.h
> +++ b/dlls/gdiplus/gdiplus_private.h
> @@ -170,7 +170,6 @@ struct GpGraphics{
>  };
>
> struct GpBrush{
> - HBRUSH gdibrush;
>
> GpBrushType bt;
>     LOGBRUSH lb;
>  };
> diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c
> index 65a3815..35aa504 100644
> --- a/dlls/gdiplus/graphics.c
> +++ b/dlls/gdiplus/graphics.c
> @@ -723,10 +723,21 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush)
>         /* else fall through */
>     }
>     default:
> - SelectObject(graphics->hdc, brush->gdibrush);
> - {
> - HBRUSH gdibrush, old_brush;
> -
> - if (brush->lb.lbStyle == BS_SOLID)
> - gdibrush = CreateSolidBrush(brush->lb.lbColor);
> - else
> - gdibrush = CreateBrushIndirect(&brush->lb);
> -
> - old_brush = SelectObject(graphics->hdc, gdibrush);
>
> FillPath(graphics->hdc);
> - SelectObject(graphics->hdc, old_brush);
> - DeleteObject(gdibrush);
>
> break;
>     }
> - }
>
> }
> static INT brush_can_fill_pixels(GpBrush *brush)
>
> 1.7.8.4

gdiplus: Create a GDI brush only when needed.

[Vincent Povirk]

The right solution is to remove the gdibrush and logbrush fields from
GpBrush, and generate the HBRUSH as needed in brush_fill_path. Note
that brush_fill_path will only be called for solid and hatch brushes
(those for which brush_can_fill_path returns true), so we don't need
the code to create a gdi brush for the other types.

gdiplus: Create a GDI brush only when needed.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> The right solution is to remove the gdibrush and logbrush fields from
> GpBrush, and generate the HBRUSH as needed in brush_fill_path. Note
> that brush_fill_path will only be called for solid and hatch brushes
> (those for which brush_can_fill_path returns true), so we don't need
> the code to create a gdi brush for the other types.

Does the attached patch look better? Next step would be to get rid of bmp
in GpSolidFill structure.

gdiplus: Create a GDI brush only when needed.

[Vincent Povirk]

Looks better.

Maybe create_gdi_logbrush should fall back to creating a solid brush,
so that pens with weird brushes set will do something vaguely
sensible. Eventually, we will want to use a different implementation
of pens that does not involve gdi32 in those cases.

On Wed, Feb 29, 2012 at 2:29 AM, Dmitry Timoshkov  wrote:
> Vincent Povirk  wrote:
> > The right solution is to remove the gdibrush and logbrush fields from
> > GpBrush, and generate the HBRUSH as needed in brush_fill_path. Note
> > that brush_fill_path will only be called for solid and hatch brushes
> > (those for which brush_can_fill_path returns true), so we don't need
> > the code to create a gdi brush for the other types.
>
> Does the attached patch look better? Next step would be to get rid of bmp
> in GpSolidFill structure.
>
> --
> Dmitry.