Bug 220848 - CSS aspect-ratio interpolation
Summary: CSS aspect-ratio interpolation
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on: 189299
Blocks: 47738
  Show dependency treegraph
 
Reported: 2021-01-22 00:50 PST by Manuel Rego Casasnovas
Modified: 2021-03-15 06:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (28.09 KB, patch)
2021-03-13 11:38 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.60 KB, patch)
2021-03-14 01:58 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.60 KB, patch)
2021-03-14 03:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.71 KB, patch)
2021-03-14 08:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.44 KB, patch)
2021-03-15 03:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.43 KB, patch)
2021-03-15 03:19 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.58 KB, patch)
2021-03-15 04:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2021-01-22 00:50:46 PST
This was discussed on the CSSWG (https://github.com/w3c/csswg-drafts/issues/4953)
and it's now in the spec: https://drafts.csswg.org/css-values/#combine-ratio
Comment 1 Radar WebKit Bug Importer 2021-01-29 00:54:56 PST
<rdar://problem/73744914>
Comment 2 Antoine Quint 2021-01-29 01:44:34 PST
Should be pretty simple to add support for this using DiscretePropertyWrapper in CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap().
Comment 3 Antoine Quint 2021-01-29 02:08:33 PST
Actually, this might be trickier since this property is set across different RenderStyle methods.
Comment 4 Rob Buis 2021-03-13 11:38:08 PST
Created attachment 423106 [details]
Patch
Comment 5 EWS Watchlist 2021-03-13 11:39:03 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 6 Rob Buis 2021-03-14 01:58:14 PST
Created attachment 423121 [details]
Patch
Comment 7 Rob Buis 2021-03-14 03:46:35 PDT
Created attachment 423123 [details]
Patch
Comment 8 Rob Buis 2021-03-14 08:38:31 PDT
Created attachment 423129 [details]
Patch
Comment 9 Antoine Quint 2021-03-15 01:37:21 PDT
Comment on attachment 423129 [details]
Patch

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

Thanks for doing this, I've had it on my to-do list but I'm glad someone else did the work :) This looks good overall but I think this could be tightened up a bit before it gets r+.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1851
> +class PropertyWrapperAspectRatio : public AnimationPropertyWrapperBase {

I think this class can be marked "final"

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1869
> +    bool canInterpolate(const RenderStyle* a, const RenderStyle* b) const override

I've made a pass through this entire file in r274409 to use from/to instead of a/b for canInterpolate(). This can also be marked "final".

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1875
> +    void logBlend(const RenderStyle* a, const RenderStyle* b, const RenderStyle* result, double progress) const final

I've made a pass through this entire file in r274409 to use from/to instead of a/b for logBlend().

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1881
> +    void blend(const CSSPropertyBlendingClient*, RenderStyle* dst, const RenderStyle* a, const RenderStyle* b, double progress) const override

I've made a pass through this entire file in r274409 to use from/to instead of a/b for blend(), as well as "destination" instead of "dst". This can also be marked "final".

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1884
> +            || a->aspectRatioType() == AspectRatioType::Auto || b->aspectRatioType() == AspectRatioType::Auto) {

The code conditioned by this clause looks like it only applies to discrete animation, so should the clause use similar logic as canInterpolate()? 

I would switch the code in this method to have a canInterpolate() block where you do the regular case and return. And then have the rest of the function assert that progress is 0 or 1, as should be the case for a discrete animation (see CSSPropertyAnimation::blendProperties) and perform the discrete animation code.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1888
> +                dst->setAspectRatio(b->aspectRatioWidth(), b->aspectRatioHeight());

Maybe just:

auto* applicableStyle = progress < 0.5 ? from : to;
destination->setAspectRatio(applicableStyle->aspectRatioWidth(), applicableStyle->aspectRatioHeight());

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1892
> +        double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()), log(b->logicalAspectRatio()), progress);

"auto" instead of "double".

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1894
> +        dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() : b->aspectRatioType());

This line is duplicated in the if clause above and here. You should run it first before the code branches.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:2196
> +        new PropertyWrapperAspectRatio(),

Other wrappers that don't take arguments are created without the ().
Comment 10 Antoine Quint 2021-03-15 03:12:13 PDT
Actually, looking at the code again:

    double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()), log(b->logicalAspectRatio()), progress);
    dst->setAspectRatio(exp(aspectRatioDst), 1);
    dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() : b->aspectRatioType());

Since in the discrete case the progress is either 0 or 1, I think the blend function could be just those three lines. While it's a bit extra work to actually blend the values when we know it'll be one or the other, it simplifies the logic.

That assumes that canInterpolate() will correctly identify all cases where the animation should be discrete, which is its contract.
Comment 11 Rob Buis 2021-03-15 03:16:22 PDT
Created attachment 423150 [details]
Patch
Comment 12 Rob Buis 2021-03-15 03:18:04 PDT
Comment on attachment 423129 [details]
Patch

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

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1851
>> +class PropertyWrapperAspectRatio : public AnimationPropertyWrapperBase {
> 
> I think this class can be marked "final"

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1869
>> +    bool canInterpolate(const RenderStyle* a, const RenderStyle* b) const override
> 
> I've made a pass through this entire file in r274409 to use from/to instead of a/b for canInterpolate(). This can also be marked "final".

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1875
>> +    void logBlend(const RenderStyle* a, const RenderStyle* b, const RenderStyle* result, double progress) const final
> 
> I've made a pass through this entire file in r274409 to use from/to instead of a/b for logBlend().

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1881
>> +    void blend(const CSSPropertyBlendingClient*, RenderStyle* dst, const RenderStyle* a, const RenderStyle* b, double progress) const override
> 
> I've made a pass through this entire file in r274409 to use from/to instead of a/b for blend(), as well as "destination" instead of "dst". This can also be marked "final".

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1884
>> +            || a->aspectRatioType() == AspectRatioType::Auto || b->aspectRatioType() == AspectRatioType::Auto) {
> 
> The code conditioned by this clause looks like it only applies to discrete animation, so should the clause use similar logic as canInterpolate()? 
> 
> I would switch the code in this method to have a canInterpolate() block where you do the regular case and return. And then have the rest of the function assert that progress is 0 or 1, as should be the case for a discrete animation (see CSSPropertyAnimation::blendProperties) and perform the discrete animation code.

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1888
>> +                dst->setAspectRatio(b->aspectRatioWidth(), b->aspectRatioHeight());
> 
> Maybe just:
> 
> auto* applicableStyle = progress < 0.5 ? from : to;
> destination->setAspectRatio(applicableStyle->aspectRatioWidth(), applicableStyle->aspectRatioHeight());

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1892
>> +        double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()), log(b->logicalAspectRatio()), progress);
> 
> "auto" instead of "double".

Done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1894
>> +        dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() : b->aspectRatioType());
> 
> This line is duplicated in the if clause above and here. You should run it first before the code branches.

Good point, done.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:2196
>> +        new PropertyWrapperAspectRatio(),
> 
> Other wrappers that don't take arguments are created without the ().

Sure, done.
Comment 13 Rob Buis 2021-03-15 03:19:17 PDT
Created attachment 423151 [details]
Patch
Comment 14 Rob Buis 2021-03-15 03:23:10 PDT
(In reply to Antoine Quint from comment #10)
> Actually, looking at the code again:
> 
>     double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()),
> log(b->logicalAspectRatio()), progress);
>     dst->setAspectRatio(exp(aspectRatioDst), 1);
>     dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() :
> b->aspectRatioType());
> 
> Since in the discrete case the progress is either 0 or 1, I think the blend
> function could be just those three lines. While it's a bit extra work to
> actually blend the values when we know it'll be one or the other, it
> simplifies the logic.
> 
> That assumes that canInterpolate() will correctly identify all cases where
> the animation should be discrete, which is its contract.

The problem is that logicalAspectRatio will assert for certain aspect ratio types (auto and autoZero). I had the test fail because of that in Debug earlier.

Do you know what causes the FAILs in the last test of aspect-ratio-interpolation.html?
I am assuming some functionality is missing, since other tests in /css/css-sizing/animation seem to have similar failures. If so I wonder if we should link to the relevant bug and keep this bug open?
Comment 15 Antoine Quint 2021-03-15 03:47:52 PDT
(In reply to Rob Buis from comment #14)
> (In reply to Antoine Quint from comment #10)
> > Actually, looking at the code again:
> > 
> >     double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()),
> > log(b->logicalAspectRatio()), progress);
> >     dst->setAspectRatio(exp(aspectRatioDst), 1);
> >     dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() :
> > b->aspectRatioType());
> > 
> > Since in the discrete case the progress is either 0 or 1, I think the blend
> > function could be just those three lines. While it's a bit extra work to
> > actually blend the values when we know it'll be one or the other, it
> > simplifies the logic.
> > 
> > That assumes that canInterpolate() will correctly identify all cases where
> > the animation should be discrete, which is its contract.
> 
> The problem is that logicalAspectRatio will assert for certain aspect ratio
> types (auto and autoZero). I had the test fail because of that in Debug
> earlier.

Thanks for the explanation. That seems worthy of a comment in the commit.

> Do you know what causes the FAILs in the last test of
> aspect-ratio-interpolation.html?
> I am assuming some functionality is missing, since other tests in
> /css/css-sizing/animation seem to have similar failures. If so I wonder if
> we should link to the relevant bug and keep this bug open?

Yes, this is due to us not supporting composite operations yet, see bug 189299.
Comment 16 Antoine Quint 2021-03-15 03:50:03 PDT
Comment on attachment 423151 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1897
> +        auto* applicableStyle = progress < 0.5 ? from : to;

This can be simply "progress ? to : from" since we've already asserted that progress is 0 or 1.
Comment 17 Rob Buis 2021-03-15 04:01:34 PDT
Created attachment 423155 [details]
Patch
Comment 18 EWS 2021-03-15 06:00:10 PDT
Committed r274415: <https://commits.webkit.org/r274415>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423155 [details].
Comment 19 Rob Buis 2021-03-15 06:02:28 PDT
(In reply to Antoine Quint from comment #15)
> (In reply to Rob Buis from comment #14)
> > (In reply to Antoine Quint from comment #10)
> > > Actually, looking at the code again:
> > > 
> > >     double aspectRatioDst = WebCore::blend(log(a->logicalAspectRatio()),
> > > log(b->logicalAspectRatio()), progress);
> > >     dst->setAspectRatio(exp(aspectRatioDst), 1);
> > >     dst->setAspectRatioType(progress < 0.5 ? a->aspectRatioType() :
> > > b->aspectRatioType());
> > > 
> > > Since in the discrete case the progress is either 0 or 1, I think the blend
> > > function could be just those three lines. While it's a bit extra work to
> > > actually blend the values when we know it'll be one or the other, it
> > > simplifies the logic.
> > > 
> > > That assumes that canInterpolate() will correctly identify all cases where
> > > the animation should be discrete, which is its contract.
> > 
> > The problem is that logicalAspectRatio will assert for certain aspect ratio
> > types (auto and autoZero). I had the test fail because of that in Debug
> > earlier.
> 
> Thanks for the explanation. That seems worthy of a comment in the commit.
> 
> > Do you know what causes the FAILs in the last test of
> > aspect-ratio-interpolation.html?
> > I am assuming some functionality is missing, since other tests in
> > /css/css-sizing/animation seem to have similar failures. If so I wonder if
> > we should link to the relevant bug and keep this bug open?
> 
> Yes, this is due to us not supporting composite operations yet, see bug
> 189299.
Comment 20 Rob Buis 2021-03-15 06:04:25 PDT
(In reply to Antoine Quint from comment #15)
> > Do you know what causes the FAILs in the last test of
> > aspect-ratio-interpolation.html?
> > I am assuming some functionality is missing, since other tests in
> > /css/css-sizing/animation seem to have similar failures. If so I wonder if
> > we should link to the relevant bug and keep this bug open?
> 
> Yes, this is due to us not supporting composite operations yet, see bug
> 189299.

Thanks, I made this bug depend on 189299 and unassigned myself to get it out of my work queue for now.