1410334 - WARN:webrender::scene: Property binding PropertyBindingKey { id: PropertyBindingId { namespace: 2989, uid: 1234 }, _phantom: PhantomData } has an invalid value.

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Peter Chang[:pchang]]

+++ This bug was initially created as a clone of Bug #1377894 +++

Seen this getting spammed in a bunch of intermittent failure logs:

WARN:webrender::scene: Property binding PropertyBindingKey { id: PropertyBindingId { namespace: 2989, uid: 1234 }, _phantom: PhantomData } has an invalid value.

https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=110316321&lineNumber=39747
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=110740104&lineNumber=40599

Comment 1

[Peter Chang[:pchang]]

Create this follow-up bug to fix the warning log with non-zero id.

Comment 2

[Peter Chang[:pchang]]

Attachment: Bug 1410334 - Add new WrAnimationProperty,

Before we don't pass the transform/opacity value for OMTA when calling PushStackingContext because
the animation value will be calculated after animation sampling. In binding.rs, we need to know
the animation type to create the matched PropertyBindingKey. But the animation id is not enough
to know the matched animation type. Therefore, passing the transform/opacity with animation id
can help to create the matched PropertyBindingKey.

When calling PushStackingContext, we need to know whick kind of animation running and create
the matched PropertyBindingKey. Before we only pass the Therefore, passing the transform/opacity with the valid
animation id

Review commit: https://reviewboard.mozilla.org/r/191496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191496/

Comment 3

[Peter Chang[:pchang]]

Before we don't pass the transform/opacity value for OMTA when calling PushStackingContext[1] because the animation value will be calculated after animation sampling. In binding.rs[2], we need to know the animation type to create the matched PropertyBindingKey. But the only information we have is 'animation id' and it is not enough to know the matched animation type. Therefore, passing the transform/opacity with animation id can help to create the matched PropertyBindingKey.

[1]http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/gfx/webrender_bindings/WebRenderAPI.cpp#655
[2]http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/gfx/webrender_bindings/src/bindings.rs#1151

Comment 4

[Peter Chang[:pchang]]

I will ask for review after try is passed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=811153da9e7611fa33a0326da94e14179ab87342

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Ah that makes sense. However I think your patch might have the opposite problem in some scenarios - if a stacking context has e.g. a fixed transform and an animated opacity, your patch will now create property bindings for both opacity and transform, but in the compositor we will only provide a binding value for the opacity.

I think it would be better to indicate more explicitly which property is animated, by changing the parameter list of PushStackingContext. We can add an enum or bitflags or something that can indicate which properties are animated and which are not.

Comment 6

[Peter Chang[:pchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Ah that makes sense. However I think your patch might have the opposite
> problem in some scenarios - if a stacking context has e.g. a fixed transform
> and an animated opacity, your patch will now create property bindings for
> both opacity and transform, but in the compositor we will only provide a
> binding value for the opacity.
> 
> I think it would be better to indicate more explicitly which property is
> animated, by changing the parameter list of PushStackingContext. We can add
> an enum or bitflags or something that can indicate which properties are
> animated and which are not.
Yes, I got some try failrues about my patch. It looks like we need to add something to indicate the animation effect. Probably we can store the animated type inside animation id which is a unique uint64 number.

Comment 7

[Intermittent Failures Robot]

2 failures in 849 pushes (0.002 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 1
* autoland: 1

Platform breakdown:
* linux64-qr: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1410334&startday=2017-11-06&endday=2017-11-12&tree=all

Comment 8

[Peter Chang[:pchang]]

I add another new WrAnimationProperty to store the id and effect type.

I will send out the review after the following try is green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6da9c0e8f644ea2f2e74b85669e770a869e6f7

Comment 9

[Peter Chang[:pchang]]

With my patch, it creates the correct animation property inside WebRender. But I still see the warning logs because we don't have the inital value before animation sampling is done. But at least we can reduce to create wrong animation properties from [1].

[1]https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/gfx/webrender_bindings/src/bindings.rs#1254
[2]https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/gfx/webrender_bindings/src/bindings.rs#1264

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Yeah, getting this landed soon would be great. I'll need it to properly handle the upcoming API change at https://github.com/servo/webrender/pull/2043#issuecomment-344709584

Comment 11

[Peter Chang[:pchang]]

Attachment: Bug 1410334 - Create correct animation properties based on WrAnimationProperty info,

Review commit: https://reviewboard.mozilla.org/r/200068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200068/

Comment 12

[Peter Chang[:pchang]]

Comment on attachment 8920496 [details]
Bug 1410334 - Add new WrAnimationProperty,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191496/diff/1-2/

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8928796 [details]
Bug 1410334 - Create correct animation properties based on WrAnimationProperty info,

https://reviewboard.mozilla.org/r/200068/#review205218

Overall I think this would probably have been cleaner if we added a NoAnimation type to the enum of animation types, and passed a const wr::WrAnimationProperty& instead of a pointer to a wr::WrAnimationProperty. But this is ok too.

::: layout/painting/nsDisplayList.cpp:6830
(Diff revision 1)
>    animationInfo.EnsureAnimationsId();
>    mWrAnimationId = animationInfo.GetCompositorAnimationsId();
>  
> +
> +  wr::WrAnimationProperty prop;
> +  if (mWrAnimationId) {

mWrAnimationId will always be nonzero here, so you can remove the if condition and just initialize it unconditionally

::: layout/painting/nsDisplayList.cpp:6836
(Diff revision 1)
> +    prop.id = mWrAnimationId;
> +    prop.effect_type = wr::WrAnimationType::Transform;
> +  }
> +
>    StackingContextHelper sc(aSc, aBuilder, nsTArray<wr::WrFilterOp>(), nullptr,
> -                           mWrAnimationId);
> +                           mWrAnimationId ? &prop : nullptr);

Ditto here, always pass &prop

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8920496 [details]
Bug 1410334 - Add new WrAnimationProperty,

https://reviewboard.mozilla.org/r/191496/#review205220

Comment 15

[Peter Chang[:pchang]]

Comment on attachment 8928796 [details]
Bug 1410334 - Create correct animation properties based on WrAnimationProperty info,

https://reviewboard.mozilla.org/r/200068/#review205326

::: layout/painting/nsDisplayList.cpp:6830
(Diff revision 1)
>    animationInfo.EnsureAnimationsId();
>    mWrAnimationId = animationInfo.GetCompositorAnimationsId();
>  
> +
> +  wr::WrAnimationProperty prop;
> +  if (mWrAnimationId) {

Yes, that's right. Removed.

::: layout/painting/nsDisplayList.cpp:6836
(Diff revision 1)
> +    prop.id = mWrAnimationId;
> +    prop.effect_type = wr::WrAnimationType::Transform;
> +  }
> +
>    StackingContextHelper sc(aSc, aBuilder, nsTArray<wr::WrFilterOp>(), nullptr,
> -                           mWrAnimationId);
> +                           mWrAnimationId ? &prop : nullptr);

Done

Comment 16

[Peter Chang[:pchang]]

Comment on attachment 8928796 [details]
Bug 1410334 - Create correct animation properties based on WrAnimationProperty info,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200068/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d62f3eba985
Add new WrAnimationProperty, r=kats
https://hg.mozilla.org/integration/autoland/rev/24b517065b54
Create correct animation properties based on WrAnimationProperty info, r=kats

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8d62f3eba985
https://hg.mozilla.org/mozilla-central/rev/24b517065b54