Bug 213742 - Move Color blending related functions to their own files
Summary: Move Color blending related functions to their own files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-29 10:16 PDT by Sam Weinig
Modified: 2020-06-30 15:50 PDT (History)
17 users (show)

See Also:


Attachments
Patch (39.24 KB, patch)
2020-06-29 10:20 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2020-06-29 18:25 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (26.25 KB, patch)
2020-06-30 13:01 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-06-29 10:16:07 PDT
Move Color blending related functions to their own files
Comment 1 Sam Weinig 2020-06-29 10:20:54 PDT Comment hidden (obsolete)
Comment 2 Dean Jackson 2020-06-29 15:15:47 PDT
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
Comment 3 Sam Weinig 2020-06-29 18:25:08 PDT Comment hidden (obsolete)
Comment 4 EWS 2020-06-29 18:33:35 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2020-06-30 09:11:20 PDT Comment hidden (obsolete)
Comment 6 Radar WebKit Bug Importer 2020-06-30 09:12:17 PDT
<rdar://problem/64941518>
Comment 7 Jason Lawrence 2020-06-30 09:54:48 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2020-06-30 10:24:56 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2020-06-30 10:54:28 PDT
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.
Comment 10 Sam Weinig 2020-06-30 11:28:55 PDT
(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).
Comment 11 Sam Weinig 2020-06-30 11:32:12 PDT
(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&);
Comment 12 Sam Weinig 2020-06-30 13:01:46 PDT
Created attachment 403230 [details]
Patch
Comment 13 EWS 2020-06-30 15:50:38 PDT
Committed r263776: <https://trac.webkit.org/changeset/263776>

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