Re: [PATCH] gdi32: Support the subpixel rendering gamma correction.

[Huw Davies]

On Sat, Oct 13, 2018 at 01:08:06AM +0900, Byeongsik Jeon wrote:
> Signed-off-by: Byeongsik Jeon
>
> By setting the appropriate gamma value, subpixel color fringing reduced.
> https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT...
>
> dlls/gdi32/dc.c                |  1 +
>  dlls/gdi32/dibdrv/dibdrv.h     |  9 ++++++-
>  dlls/gdi32/dibdrv/graphics.c   | 38 ++++++++++++++++++++++----
>  dlls/gdi32/dibdrv/primitives.c | 49 +++++++++++++++++++++++++---------
>  dlls/gdi32/font.c              | 28 +++++++++++++++++++
>  dlls/gdi32/gdi_private.h       |  1 +
>  6 files changed, 108 insertions(+), 18 deletions(-)
>
> diff --git a/dlls/gdi32/dc.c b/dlls/gdi32/dc.c
> index 892d3e9744..dd7a86d571 100644
> --- a/dlls/gdi32/dc.c
> +++ b/dlls/gdi32/dc.c
> @@ -87,6 +87,7 @@ static void set_initial_dc_state( DC *dc )
>      dc->miterLimit          = 10.0f; /* 10.0 is the default, from MSDN */
>      dc->layout              = 0;
>      dc->font_code_page      = CP_ACP;
> - dc->font_gamma          = 0;
> dc->ROPmode             = R2_COPYPEN;
> dc->polyFillMode        = ALTERNATE;
> dc->stretchBltMode      = BLACKONWHITE;
>
> diff --git a/dlls/gdi32/dibdrv/dibdrv.h b/dlls/gdi32/dibdrv/dibdrv.h
> index f79f5b0867..ea33386faf 100644
> --- a/dlls/gdi32/dibdrv/dibdrv.h
> +++ b/dlls/gdi32/dibdrv/dibdrv.h
> @@ -85,6 +85,13 @@ struct intensity_range
>      BYTE b_min, b_max;
>  };
>
> +struct font_gamma_lut
> +{
> - DWORD value;
> - BYTE  encode[256];
> - BYTE  decode[256];
>
> +};
> -
>
> typedef struct dibdrv_physdev
>  {
>      struct gdi_physdev dev;
> @@ -192,7 +199,7 @@ typedef struct primitive_funcs
>      void             (* draw_glyph)(const dib_info *dst, const RECT *rc, const dib_info *glyph,
>                                      const POINT *origin, DWORD text_pixel, const struct intensity_range *ranges);
>      void    (* draw_subpixel_glyph)(const dib_info *dst, const RECT *rc, const dib_info *glyph,
> -
>                                const POINT *origin, DWORD text_pixel );
> -
>                                const POINT *origin, DWORD text_pixel, const struct font_gamma_lut *gamma);
> DWORD             (* get_pixel)(const dib_info *dib, int x, int y);
>    DWORD     (* colorref_to_pixel)(const dib_info *dib, COLORREF color);
>    COLORREF  (* pixel_to_colorref)(const dib_info *dib, DWORD pixel);
>
> diff --git a/dlls/gdi32/dibdrv/graphics.c b/dlls/gdi32/dibdrv/graphics.c
> index 08f8c3d6d2..195866b92f 100644
> --- a/dlls/gdi32/dibdrv/graphics.c
> +++ b/dlls/gdi32/dibdrv/graphics.c
> @@ -683,7 +683,7 @@ static inline void get_text_bkgnd_masks( DC *dc, const dib_info *dib, rop_mask *
>
> static void draw_glyph( dib_info *dib, int x, int y, const GLYPHMETRICS *metrics,
>                          const dib_info *glyph_dib, DWORD text_color,
> -
>                    const struct intensity_range *ranges, const struct clipped_rects *clipped_rects,
> -
>                    const void *lut, const struct clipped_rects *clipped_rects,
>                    RECT *bounds )

Using a void ptr here is ugly, want you want is a union of the two sets of data.
> {
>      int i;
> @@ -705,10 +705,10 @@ static void draw_glyph( dib_info *dib, int x, int y, const GLYPHMETRICS *metrics
>          if (glyph_dib->bit_count == 32)
>              dib->funcs->draw_subpixel_glyph( dib, &clipped_rect, glyph_dib, &src_origin,
> -
>                                             text_color );
> -
>                                             text_color, lut );
>        else
>            dib->funcs->draw_glyph( dib, &clipped_rect, glyph_dib, &src_origin,
> -
>                                    text_color, ranges );
> -
>                                    text_color, lut );
>    }
> }
>
> }
> @@ -808,6 +808,19 @@ done:
>      return add_cached_glyph( font, index, flags, glyph );
>  }
>
> +static inline void update_font_gamma_lut( DWORD value, struct font_gamma_lut *gamma )
> +{
> - int i;
> -
> - for ( i = 0; i < 256; i++ )
> - {
> -
>    gamma->encode[i] = pow( i / 255., 1000. / value ) * 255. + .5;
> -
>    gamma->decode[i] = pow( i / 255., value / 1000. ) * 255. + .5;
> - }
> -
> - gamma->value = value;
>
> +}
> -
>
> static void render_string( DC *dc, dib_info *dib, struct cached_font *font, INT x, INT y,
>                             UINT flags, const WCHAR *str, UINT count, const INT *dx,
>                             const struct clipped_rects *clipped_rects, RECT *bounds )
> @@ -817,6 +830,8 @@ static void render_string( DC *dc, dib_info *dib, struct cached_font *font, INT
>      dib_info glyph_dib;
>      DWORD text_color;
>      struct intensity_range ranges[17];
> -
>
> static struct font_gamma_lut gamma;
> -
>
> void *lut = NULL;
>
> glyph_dib.bit_count    = get_glyph_depth( font->aa_flags );
> glyph_dib.rect.left    = 0;
>
> @@ -826,8 +841,21 @@ static void render_string( DC *dc, dib_info *dib, struct cached_font *font, INT
>  text_color = get_pixel_color( dc, dib, dc->textColor, TRUE );
>
> - if (glyph_dib.bit_count == 8)
> - switch ( glyph_dib.bit_count )
> - {
> - case 8:
>     get_aa_ranges( dib->funcs->pixel_to_colorref( dib, text_color ), ranges );
> -
>    lut = ranges;
> -
>    break;
> - case 32:
> -
>    if ( gamma.value != dc->font_gamma )

This isn't thread safe, you should store the computed values in the dc.
> -
>    {
> -
>        update_font_gamma_lut( dc->font_gamma, &gamma );
> -
>        TRACE( "font_contrast = %d\n", gamma.value );
> -
>    }
> -
>    lut = &gamma;
> -
>    break;
> -
>
> }
>
> for (i = 0; i < count; i++)
> {