Bug 213742 – Move Color blending related functions to their own files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Move Color blending related functions to their own files

[Sam Weinig]

Created attachment 403078 [details]
Patch

[Dean Jackson]

Comment on attachment 403078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403078&action=review

> Source/WebCore/ChangeLog:25
> +        Move declarations / implemenatations from Color.h/cpp to ColorBlending.h/cpp.

Typo

[Sam Weinig]

Created attachment 403152 [details]
Patch

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 403152 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Sam Weinig]

Committed r263753: <https://trac.webkit.org/changeset/263753>

[Radar WebKit Bug Importer]

<rdar://problem/64941518>

[Jason Lawrence]

Reverted r263753 for reason:

This commit caused build failures across multiple platforms.

Committed r263757: <https://trac.webkit.org/changeset/263757>

[Sam Weinig]

(In reply to Jason Lawrence from comment #7)
> Reverted r263753 for reason:
> 
> This commit caused build failures across multiple platforms.
> 
> Committed r263757: <https://trac.webkit.org/changeset/263757>

Crap, sorry. Apparently, our tools really don't working with svn cp any more. :(

[Darin Adler]

Comment on attachment 403152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403152&action=review

> Source/WebCore/platform/graphics/ColorBlending.h:39
> +// This is an implementation of Porter-Duff's "source-over" equation
> +Color blendSourceOver(const Color&, const Color&);
> +
> +// Bespoke "whitening" algorithm used by RenderTheme::transformSelectionBackgroundColor.
> +Color blendWithWhite(const Color&);
> +
> +Color blend(const Color& from, const Color& to, double progress);
> +Color blendWithoutPremultiply(const Color& from, const Color& to, double progress);

All of these functions also transform extended colors into simple colors. Would be nice if this was clearer.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 403152 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403152&action=review
> 
> > Source/WebCore/platform/graphics/ColorBlending.h:39
> > +// This is an implementation of Porter-Duff's "source-over" equation
> > +Color blendSourceOver(const Color&, const Color&);
> > +
> > +// Bespoke "whitening" algorithm used by RenderTheme::transformSelectionBackgroundColor.
> > +Color blendWithWhite(const Color&);
> > +
> > +Color blend(const Color& from, const Color& to, double progress);
> > +Color blendWithoutPremultiply(const Color& from, const Color& to, double progress);
> 
> All of these functions also transform extended colors into simple colors.
> Would be nice if this was clearer.

Hm, indeed. 

Color blendSourceOverSRGBASimpleColorLossy(const Color&, const Color&);

That seems a bit wordy and very unclear, so let's not do that :). 

I wonder what if we made them take a typed color, and make the caller do the conversion (I tend to like this approach, as it makes it easy to find all the lossy conversions easily).


SRGBA<uint8_t> blendSourceOver(const SRGBA<uint8_t>&, const const SRGBA<uint8_t>&);
SRGBA<uint8_t> blendWithWhite(const SRGBA<uint8_t>&);
SRGBA<uint8_t> blend(const SRGBA<uint8_t>&, const SRGBA<uint8_t>&, double progress);
SRGBA<uint8_t> blendWithoutPremultiply(const SRGBA<uint8_t>&, const SRGBA<uint8_t>&, double progress);


This has a few initial problems I can think of.

1) blendWithWhite for some reason preserves the Semantic bit. This is easily solvable, since blendWithWhite has only one caller, by just making it the callers responsibility to preserve it. (Honestly, I have not looked into why we still have that bit or why its ok for it to only work for SimpleColors. I know it's used only by editing and filter code, but not entirely sure why).

2) blend and blendWithoutPremultiply preserve the isValid bit in certain circumstances (when progress == 1).

[Sam Weinig]

(In reply to Sam Weinig from comment #10)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 403152 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403152&action=review
> > 
> > > Source/WebCore/platform/graphics/ColorBlending.h:39
> > > +// This is an implementation of Porter-Duff's "source-over" equation
> > > +Color blendSourceOver(const Color&, const Color&);
> > > +
> > > +// Bespoke "whitening" algorithm used by RenderTheme::transformSelectionBackgroundColor.
> > > +Color blendWithWhite(const Color&);
> > > +
> > > +Color blend(const Color& from, const Color& to, double progress);
> > > +Color blendWithoutPremultiply(const Color& from, const Color& to, double progress);
> > 
> > All of these functions also transform extended colors into simple colors.
> > Would be nice if this was clearer.
> 
> Hm, indeed. 
> 
> Color blendSourceOverSRGBASimpleColorLossy(const Color&, const Color&);
> 
> That seems a bit wordy and very unclear, so let's not do that :). 

A shorter name idea, just use Lossy suffix and add a comment:

// NOTE: These function will do a lossy conversion to 8-bit sRGBA before blending.
Color blendSourceOverLossy(const Color&, const Color&);

[Sam Weinig]

Created attachment 403230 [details]
Patch

[EWS]

Committed r263776: <https://trac.webkit.org/changeset/263776>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403230 [details].