Closed Bug 1410334 Opened 3 years ago Closed 2 years ago

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

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(2 files)

+++ 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
Create this follow-up bug to fix the warning log with non-zero id.
Assignee: nobody → howareyou322
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
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.
Status: NEW → ASSIGNED
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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
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
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 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
Attachment #8928796 - Flags: review?(bugmail) → review+
Comment on attachment 8920496 [details]
Bug 1410334 - Add new WrAnimationProperty,

https://reviewboard.mozilla.org/r/191496/#review205220
Attachment #8920496 - Flags: review?(bugmail) → review+
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
Pushed by pchang@mozilla.com:
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8d62f3eba985
https://hg.mozilla.org/mozilla-central/rev/24b517065b54
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.