Closed Bug 1335998 Opened 7 years ago Closed 7 years ago

stylo: Handle interpolation (interpolatematrix) and accumulation (accumulatematrix) of mismatched transform lists

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: boris)

References

(
URL
)

Details

Attachments

(5 files, 5 obsolete files)

Test case for mismatched transform lists
869 bytes, text/html
Details
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.
59 bytes, text/x-review-board-request
manishearth
: review+
birtles
: review+
Details
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #17202
41 bytes, text/x-github-pull-request
Details | Review 
This is the counter part of servo's transform interpolation problem that needs reference box [1].
Once servo has ComputedOperation::InterpolateMatrix type (or something), I guess we need to delegate its interpolation to servo when we build transform display list.  (Or we need to convert it to gecko's interpolatematrix.)

[1] https://github.com/servo/servo/issues/13267
Depends on: 1332657
Priority: -- → P3
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> This is the counter part of servo's transform interpolation problem that
> needs reference box [1].
> Once servo has ComputedOperation::InterpolateMatrix type (or something), I
> guess we need to delegate its interpolation to servo when we build transform
> display list.  (Or we need to convert it to gecko's interpolatematrix.)
> 
> [1] https://github.com/servo/servo/issues/13267

I think this makes sense, so let's try this way on stylo for now.
Assignee: nobody → boris.chiou
Priority: P3 → P1
Status: NEW → ASSIGNED
The interesting thing is: in order to test this, I have to disable OMTA first because compositor makes it work.
And it seems we really need to convert it into gecko interpolatematrix (in convert_transform), so we could store it in nsFrame->StyleDisplay() or nsDisplayList, as |nsCSSValueShardList| type. Therefore, we can get the result matrix in many places, e.g. for computing the bound or others.
Thank you Boris for taking care of this!
I will mark several bugs as duplicate of this.
Borrowed from Bug 1365851
Attachment #8872015 - Attachment filename: file_1335998.txt → mismatched_lists.html
Attachment #8872015 - Attachment mime type: text/plain → text/html
Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review147936
Attachment #8872306 - Flags: review+
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review147938

::: layout/style/nsStyleTransformMatrix.cpp:443
(Diff revision 1)
>                        TransformReferenceBox& aRefBox,
>                        bool* aContains3dTransform)
>  {
>    NS_PRECONDITION(aData->Count() == 4, "Invalid array!");
>  
> -  Matrix4x4 matrix1, matrix2;
> +  auto readTransform = [&](const nsCSSValue& aValue) -> Matrix4x4 {

Since this has no captures, can it just be a function? AIUI lambdas are less common in C++.

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:276
(Diff revision 1)
> +
> +        let mut item_ptr = unsafe {
> +            self.mValue.mSharedList.as_ref()
> +                .as_mut().expect("List pointer should be non-null")
> +        }.mHead;
> +        while let Some(item) = unsafe { item_ptr.as_mut() } {

generally it would be preferable to implement this as an iterator, but it seems like a one-off thing so probably not worth it

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:278
(Diff revision 1)
> +            self.mValue.mSharedList.as_ref()
> +                .as_mut().expect("List pointer should be non-null")
> +        }.mHead;
> +        while let Some(item) = unsafe { item_ptr.as_mut() } {
> +            item.mValue = values.next().expect("Values shouldn't have been exhausted");
> +            item_ptr = item.mNext;

debug assert that it isn't null

::: servo/components/style/properties/gecko.mako.rs:2285
(Diff revision 1)
> -                    pattern = "ComputedMatrix { %s }" % ", ".join(single_patterns)
> +                    pattern = "(ComputedMatrix { %s })" % ", ".join(single_patterns)
>                  else:
> -                    pattern = "ComputedMatrixWithPercents { %s }" % ", ".join(single_patterns)
> +                    pattern = "(ComputedMatrixWithPercents { %s })" % ", ".join(single_patterns)
> +            elif keyword == "interpolatematrix":
> +                fields = zip(["from_list: ref ", "to_list: ref ", "progress: "], items)
> +                pattern = " { %s }" % ", ".join([b[0] + b[1] + str(a+1) for (a,b) in enumerate(fields)])

kinda feel like directly typing out `{ from_list: ref list1, to_list: ref list2, progress: percentage3 }` would be better here. As it stands it's hard to figure out what the code does.

If not, leave a comment with the expanded form or at least part of it, like we've done in the matrix code

::: servo/components/style/properties/gecko.mako.rs:2312
(Diff revision 1)
>                  bindings::Gecko_CSSValue_GetArrayItem(gecko_value, 0),
>                  eCSSKeyword_${keyword}
>              );
>              % for index, item in enumerate(items):
> +                % if item == "list":
> +                debug_assert!(${item}${index + 1}.0.is_some());

nit: indent

::: servo/components/style/properties/longhand/box.mako.rs:1220
(Diff revision 1)
> +        impl ComputedOperation {
> +            #[inline]
> +            pub fn is_interpolate_matrix(&self) -> bool {
> +                use self::ComputedOperation::InterpolateMatrix;
> +                match self {
> +                    &InterpolateMatrix { from_list: _, to_list: _, progress: _ } => true,

`match *self { InterpolateMatrix {...} => true, }
Attachment #8872307 - Flags: review+
Comment on attachment 8872309 [details]
Bug 1335998 - Part 8: Enable table-overflowed-by-animation.html on stylo.

https://reviewboard.mozilla.org/r/143810/#review147960
Attachment #8872309 - Flags: review+
Comment on attachment 8872308 [details]
Bug 1335998 - Part 6: Add some unit tests for basic transform operations for Servo.

https://reviewboard.mozilla.org/r/143808/#review147962
Attachment #8872308 - Flags: review+
I assume this is still WIP with the actual interpolation code missing.
(In reply to Manish Goregaokar [:manishearth] from comment #17)
> I assume this is still WIP with the actual interpolation code missing.

Actually, these patches use the interpolation code on Gecko now, and I am still trying to make them use the interpolation code of two matrices on Servo. Thanks for reviewing them.
Summary: stylo: Handle servo's interpolatematrix → stylo: Handle interpolation (interpolatematrix) and accumulation (accumulatematrix) of mismatched transform lists
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review147938

> Since this has no captures, can it just be a function? AIUI lambdas are less common in C++.

Oh sorry for my indention is not clear enough. Actually there are some captures, and that's why I use lambda here.

> generally it would be preferable to implement this as an iterator, but it seems like a one-off thing so probably not worth it

Yes. I agree. actually we also iterate nsCSSValueList in gecko.mako.rs, so it might be worth to do it.

> debug assert that it isn't null

I just noticed that the mNext of the last item is null. Looks like using iterator could be better. :)

> kinda feel like directly typing out `{ from_list: ref list1, to_list: ref list2, progress: percentage3 }` would be better here. As it stands it's hard to figure out what the code does.
> 
> If not, leave a comment with the expanded form or at least part of it, like we've done in the matrix code

OK, I will just type it out. Thanks for the suggestion.
(In reply to Boris Chiou [:boris] from comment #18)
> (In reply to Manish Goregaokar [:manishearth] from comment #17)
> > I assume this is still WIP with the actual interpolation code missing.
> 
> Actually, these patches use the interpolation code on Gecko now, and I am
> still trying to make them use the interpolation code of two matrices on
> Servo. Thanks for reviewing them.

Just finish the AccumulateMatrix part. The current implementation still decompose/interpolate/recompose the matrix in Gecko (i.e. in nsStyleTransformMatrix.cpp). I will make it use Servo code for this part tomorrow. (i.e. Pass the two matrices to Servo, and decompose/interpolate/recompose them to a new interpolated matrix, and then return it back to Gecko side.)
> The current implementation still decompose/interpolate/recompose the matrix in Gecko (i.e. in nsStyleTransformMatrix.cpp).

Ah, nice. I didn't realize that we still used the gecko backend for transform animations. For OMT animations I suppose.
(In reply to Boris Chiou [:boris] from comment #21)
> Just finish the AccumulateMatrix part. The current implementation still
> decompose/interpolate/recompose the matrix in Gecko (i.e. in
> nsStyleTransformMatrix.cpp). I will make it use Servo code for this part
> tomorrow. (i.e. Pass the two matrices to Servo, and
> decompose/interpolate/recompose them to a new interpolated matrix, and then
> return it back to Gecko side.)

AccumulateMatrices normally are nested in an InterpolateMatrix, you will need to send whole set of matrices, I guess.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Boris Chiou [:boris] from comment #21)
> > Just finish the AccumulateMatrix part. The current implementation still
> > decompose/interpolate/recompose the matrix in Gecko (i.e. in
> > nsStyleTransformMatrix.cpp). I will make it use Servo code for this part
> > tomorrow. (i.e. Pass the two matrices to Servo, and
> > decompose/interpolate/recompose them to a new interpolated matrix, and then
> > return it back to Gecko side.)
> 
> AccumulateMatrices normally are nested in an InterpolateMatrix, you will
> need to send whole set of matrices, I guess.

Yes, we got something like this on Servo:
e.g.
Interpolate { from_list: AccmulateMatrix { from_list: rotate(45deg), to_list: translateX(100px), count: 2 },
              to_list: rotate(90deg),
              progress: 0.5 }

And convert it into a nsCSSValueSharedList. Gecko will read this as matrices (by applying layout info), and we use Servo code to decompose/interpolate/recompose these matrices to get the final result.
Attachment #8872309 - Attachment is obsolete: true
Attachment #8872307 - Flags: review+ → review?(manishearth)
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149560

::: layout/style/ServoBindings.h:503
(Diff revision 2)
>  void Gecko_CSSValue_SetInt(nsCSSValueBorrowedMut css_value, int32_t integer, nsCSSUnit unit);
>  void Gecko_CSSValue_SetPair(nsCSSValueBorrowedMut css_value,
>                              nsCSSValueBorrowed xvalue, nsCSSValueBorrowed yvalue);
>  void Gecko_CSSValue_SetList(nsCSSValueBorrowedMut css_value, uint32_t len);
>  void Gecko_CSSValue_SetPairList(nsCSSValueBorrowedMut css_value, uint32_t len);
> +void Gecko_CSSValue_SetSharedList(nsCSSValueBorrowedMut css_value, uint32_t len);

Change name to InitSharedList?
Attachment #8872307 - Flags: review?(manishearth) → review+
Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149562

don't completely understand Accumulate, but it seems to be ok.
Attachment #8874292 - Flags: review?(manishearth) → review+
For Servo issue, https://github.com/servo/servo/issues/13267, I think it will be easier to fix in layout/fragment.rs after introducing InterpolateMatrix. (We can send an extra PR for it.)
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149560

> Change name to InitSharedList?

OK, I will update its name. Thanks for suggestion.
Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

https://reviewboard.mozilla.org/r/145566/#review149568

2000ms is too long for crashtest.
What is the exact purpose of this test? Do we really need to wait for the end of the animation?
If we just want to check that InterpolateMatrix and AccumulateMatrix work fine on gecko side, we should wait for the animation being sent to compositor?
Attachment #8874295 - Flags: review?(hikezoe) → review+
Comment on attachment 8874293 [details]
Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

https://reviewboard.mozilla.org/r/145562/#review149564

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:233
(Diff revision 1)
>      /// This is only supported on the main thread.
> -    pub fn set_list<I>(&mut self, mut values: I) where I: ExactSizeIterator<Item=nsCSSValue> {
> +    pub fn set_list<I>(&mut self, values: I) where I: ExactSizeIterator<Item=nsCSSValue> {
>          debug_assert!(values.len() > 0, "Empty list is not supported");
>          unsafe { bindings::Gecko_CSSValue_SetList(self, values.len() as u32); }
>          debug_assert_eq!(self.mUnit, nsCSSUnit::eCSSUnit_List);
> -        let mut item_ptr = &mut unsafe {
> +        let list = &mut unsafe {

I would use an explicit type annotation here to be sure
Comment on attachment 8874293 [details]
Bug 1335998 - Part 3: Use IntoIterator for nsCSSValueList.

https://reviewboard.mozilla.org/r/145562/#review149572
Attachment #8874293 - Flags: review?(manishearth) → review+
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2050
(Diff revision 1)
>  
> +#[cfg(feature = "gecko")]
> +impl From<Matrix4x4> for ComputedMatrix {
> +    fn from(matrix: Matrix4x4) -> ComputedMatrix {
> +        // Matrix4x4 is [u32; 16usize]. However, it should be f32 on Gecko, so we
> +        // have to transmute it first.

Wait, why is this?

I'm not very comfortable with this transmute -- in Gecko are we storing the bit representations of floats in u32 fields? That doesn't seem right.
Attachment #8874294 - Flags: review?(manishearth)
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> Wait, why is this?
> 
> I'm not very comfortable with this transmute -- in Gecko are we storing the bit representations of floats in u32 fields? That doesn't seem right.

Yes. it's weird. In structs_debug.rs, we set ```pub type Matrix4x4 = [u32; 16usize];```. However, in Gecko, we use ```gfx::Float``` (i.e. float) [1] in Matrix4x4. Do you have any idea to let bindgen generate f32 for it?

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko/generated/structs_debug.rs#2214
[2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Matrix.h#483
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> Yes. it's weird. In structs_debug.rs, we set ```pub type Matrix4x4 = [u32; 16usize];```. However, in Gecko, we use ```gfx::Float``` (i.e. float) [1] in Matrix4x4. Do you have any idea to let bindgen generate f32 for it?
> 
> [1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko/generated/structs_debug.rs#2214
> [2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Matrix.h#483

OK. I will use a workaround. Just pass Matrix4x4::components[16], which is a native array type. This way may avoid this problem.
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149574

> OK. I will use a workaround. Just pass Matrix4x4::components[16], which is a native array type. This way may avoid this problem.

Ohh, the root cause is, gfx::Float is defined as ```u32``` in structs_debug.rs. I think this is a bug...
Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149570

::: commit-message-45411:6
(Diff revision 2)
> +Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.
> +
> +We use this arm to store the interpolated result of two mismatched
> +transform lists, and we resolve it until we know the reference box size
> +(on Gecko side). The conversion from ComputedOperation::InterpolateMatrix
> +to eCSSKeyword_interpolatematrix will be implemented in this patch series.

(Nit: later in this patch series)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1635
(Diff revision 2)
> +            TransformOperation::InterpolateMatrix { .. } => {
> +                // We delay the matrix interpolation after getting frame info, so use an identity
> +                // matrix.
> +                let identity = ComputedMatrix::identity();
> +                result.push(TransformOperation::Matrix(identity));
> +            }

I don't really understand this comment, or maybe it's the code I don't follow.

This is in build_identity_transform_list. We use that when we try to do add_weighted on two transform lists and one of them is None. This relates to my comment a little later, but it's not clear why identity is the right value here.

Do we ever expect to pass the result of this to add_weighted? If so, then I guess we need to modify can_interpolate_list. If not, then do we need to put anything in the list at all?

I'm not saying it's wrong, it's just not clear why this is the correct thing to do -- perhaps simply because we haven't documented how interpolatematrix is expected to be used.

::: servo/components/style/properties/longhand/box.mako.rs:1184
(Diff revision 2)
>                        computed::LengthOrPercentage,
>                        computed::Length),
>              Scale(CSSFloat, CSSFloat, CSSFloat),
>              Rotate(CSSFloat, CSSFloat, CSSFloat, computed::Angle),
>              Perspective(computed::Length),
> +            // For interpolated mismatched transform lists.

Nit: For mismatched transform lists.

::: servo/components/style/properties/longhand/box.mako.rs:1267
(Diff revision 2)
>          /// Part of CSS Transform Module Level 2 and defined at
>          /// [§ 13.1. 3D Transform Function](https://drafts.csswg.org/css-transforms-2/#funcdef-perspective).
>          ///
>          /// The value must be greater than or equal to zero.
>          Perspective(specified::Length),
> +        /// A intermediate type for interpolation on mismatched transform lists.

Nit: interpolation of

::: servo/components/style/properties/longhand/box.mako.rs:1878
(Diff revision 2)
> +                        debug_assert!(result.is_empty(),
> +                                      "transform list can only contain one InterpolateMatrix");

Why is that?

It's not really clear to me what the invariants are around using interpolatematrix. They probably need to be documented somewhere in this patch.

e.g.

* If we use interpolatematrix it should be the only function in the list
* If we have a transform list with interpolatematrix in it, we should never call add_weighted on it (or pass it to add_weighted)
* etc.

I'm not sure if any of those are true, they're just examples of the sorts of invariants we need to document if they exist.
Hi, Manish, Emilio,

I got a bug in bindgen. When I add "mozilla::gfx::Float" into ServoBindings.toml, its definition is |u32| [1]. However, in gecko |mozilla::gfx::Float| is defined as |float| [2] in gfx/2d/Types.h

[1] in structs_debug.rs (my local build)
    pub mod gfx {
        #[allow(unused_imports)]
        use self::super::super::super::root;
        pub type IntRegion = [u64; 3usize];
        pub type Float = u32;
        ...
    }

[2] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Types.h#18

Do you know what is the potential problem in rust-bindgen to generate this type? This result seems incorrect.
Flags: needinfo?(manishearth)
Flags: needinfo?(emilio+bugs)
Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149588

::: servo/components/style/properties/helpers/animated_properties.mako.rs:654
(Diff revision 1)
> +                        }
> +                    % endif
> +                % endif
> +            % endfor
> +            _ => {
> +                panic!("Expected weighted addition of computed values of the same \

This comment should be "Expected accumulation of ..."

(I notice the comment in the previous definition of add() is also wrong. That's probably my fault.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2638
(Diff revision 1)
> +                let to_list = build_identity_transform_list(from_list);
> +                Ok(add_weighted_transform_lists(from_list, &to_list, count as f64, 1.0))

Do we need to build an identity transform list here?

The corresponding code on the Gecko side is just:

  resultList = AddTransformLists(0.0, listA, aCount, listA,
                                 eCSSKeyword_accumulatematrix);

Could we do the same here:

  Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))

Does that work?

(And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)

::: servo/components/style/properties/longhand/box.mako.rs:1188
(Diff revision 1)
>              Perspective(computed::Length),
>              // For interpolated mismatched transform lists.
>              InterpolateMatrix { from_list: T,
>                                  to_list: T,
>                                  progress: Percentage },
> +            // For accumulate operation on mismatched transform lists.

Nit: of mismatched transform lists.

::: servo/components/style/properties/longhand/box.mako.rs:2000
(Diff revision 1)
> +                            debug_assert!(result.is_empty(),
> +                                          "transform list can only contain one AccumulateMatrix");

(As with the previous patch, somewhere we need to explain why this assertion makes sense.)
Attachment #8874292 - Flags: review?(bbirtles) → review+
Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149570

> I don't really understand this comment, or maybe it's the code I don't follow.
> 
> This is in build_identity_transform_list. We use that when we try to do add_weighted on two transform lists and one of them is None. This relates to my comment a little later, but it's not clear why identity is the right value here.
> 
> Do we ever expect to pass the result of this to add_weighted? If so, then I guess we need to modify can_interpolate_list. If not, then do we need to put anything in the list at all?
> 
> I'm not saying it's wrong, it's just not clear why this is the correct thing to do -- perhaps simply because we haven't documented how interpolatematrix is expected to be used.

I think it is possible to pass an InterpolateMatirx into add_weighted(). gfx/tests/crashtests/815489.html has this situation:
e.g.
```
[InterpolateMatrix { from_list: T(Some([Rotate(1, 0, 0, Degree(67166.57))])),
                     to_list: T(Some([InterpolateMatrix { from_list: T(Some([Rotate(1, 0, 0, Degree(67166.57))])),
                                                          to_list: T(Some([Skew(Radian(0), Degree(86))])),
                                                          progress: Percentage(0.0145912245) }])),
                     progress: Percentage(0.018843267) }]
```
For this case, can_interpolate_list() returns false, so we may have a nested InterpolateMatrix.

However, if one of the transform list is none, it is impossible to create an InterpolateMatrix (and pass it into build_identity_transform_list() later). Hence, we can just assert this case. (i.e. build_identity_transform_list() should assert if we pass InterpolateMatrix in.)

> Why is that?
> 
> It's not really clear to me what the invariants are around using interpolatematrix. They probably need to be documented somewhere in this patch.
> 
> e.g.
> 
> * If we use interpolatematrix it should be the only function in the list
> * If we have a transform list with interpolatematrix in it, we should never call add_weighted on it (or pass it to add_weighted)
> * etc.
> 
> I'm not sure if any of those are true, they're just examples of the sorts of invariants we need to document if they exist.

> * If we use interpolatematrix it should be the only function in the list

Yes, that is what I want to assert. TransfromList can only contain one InterpolateMatrix:
e.g.
```
[ InterpolateMatrix { from_list: ..., to_list: InterpolateMatrix {...} } ]
[ InterpolateMatrix { from_list: InterpolateMatrix {...}, to_list: ... } ]
```
This is valid, because this transform list has only one InterpolateMatrix. (Therefore, we may pass an InterpolateMatirx into add_weighed()). A test case has this situation: http://searchfox.org/mozilla-central/source/gfx/tests/crashtests/815489.html. And this may happen if we trigger a transition from a middle point of the interpolation to its original value. (i.e. mouse hover)

e.g.
```
[ InterpolateMatrix { from_list: ..., to_list: ... },
  Interpolatematrix { from_list: ..., to_list: ... } ]
```
This is not correct. We only creat one InterpolateMatirx for a list.

I will document this into box.mako.rs, before the arm of ```InterpolateMatrix```.
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149596

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:273
(Diff revision 2)
> +        for (item, new_value) in list.unwrap().into_iter().zip(values) {
> +            *item = new_value;
> +        }

Does this work? |list|, at this point, points to mHead which is a nsCSSValueList* pointer which we should navigate through by looking up its mNext members. Something like [1]

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko_bindings/sugar/ns_css_value.rs#236

Unless IntoIterator is defined for this somewhere else?

::: servo/components/style/properties/gecko.mako.rs:2393
(Diff revision 2)
> +                "list" : "TransformList(Some(convert_shared_list_to_operations(%s)))",
> +            }
> +            pre_symbols = "("
> +            post_symbols = ")"
> +            if keyword == "interpolatematrix" or keyword == "accumulatematrix":
> +                pre_symbols = " {"

Is the space before the { here intentional?
Attachment #8872307 - Flags: review?(bbirtles)
Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review149602

Clearing review on this for now since it sounds like there are a few tweaks to be made.
Attachment #8872306 - Flags: review?(bbirtles)
(In reply to Boris Chiou [:boris] from comment #47)
> e.g.
> ```
> [ InterpolateMatrix { from_list: ..., to_list: ... },
>   Interpolatematrix { from_list: ..., to_list: ... } ]
> ```
> This is not correct. We only creat one InterpolateMatirx for a list.

When an underlying value is InterpolateMatrix and a subsequent transform is additive, what happens? Also more additive or accumulative transform on the underlying InterpolateMatrix?
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149596

> Does this work? |list|, at this point, points to mHead which is a nsCSSValueList* pointer which we should navigate through by looking up its mNext members. Something like [1]
> 
> [1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/servo/components/style/gecko_bindings/sugar/ns_css_value.rs#236
> 
> Unless IntoIterator is defined for this somewhere else?

Yes. In Part 3:
```
/// Mutable Iterator of nsCSSValueList.
#[allow(non_camel_case_types)]
pub struct nsCSSValueListMutIterator<'a> {
    current: *mut nsCSSValueList,
    phantom: PhantomData<&'a mut nsCSSValue>,
}

impl<'a> Iterator for nsCSSValueListMutIterator<'a> {
    type Item = &'a mut nsCSSValue;
    fn next(&mut self) -> Option<Self::Item> {
        match unsafe { self.current.as_mut() } {
            Some(item) => {
                self.current = item.mNext;
                Some(&mut item.mValue)
            },
            None => None
        }
    }
}

impl<'a> IntoIterator for &'a mut nsCSSValueList {
    type Item = &'a mut nsCSSValue;
    type IntoIter = nsCSSValueListMutIterator<'a>;

    fn into_iter(self) -> Self::IntoIter {
        nsCSSValueListMutIterator { current: self as *mut nsCSSValueList,
                                    phantom: PhantomData }
    }

```
When calling next() at the first time, we check the first nsCSSValueList and assign the mNext to current. This should be the same as that in [1].

> Is the space before the { here intentional?

Yes.
Comment on attachment 8874295 [details]
Bug 1335998 - Part 3: Add a crashtest for mismatched transform lists.

https://reviewboard.mozilla.org/r/145566/#review149576

::: commit-message-e9aaf:1
(Diff revision 1)
> +Bug 1335998 - Part 7: Add a crashtest for mismatched transfrom lists.

Nit: transform lists
(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> (In reply to Boris Chiou [:boris] from comment #47)
> > e.g.
> > ```
> > [ InterpolateMatrix { from_list: ..., to_list: ... },
> >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > ```
> > This is not correct. We only creat one InterpolateMatirx for a list.
> 
> When an underlying value is InterpolateMatrix and a subsequent transform is
> additive, what happens? Also more additive or accumulative transform on the
> underlying InterpolateMatrix?

I cannot run wpt now, and just pull new code to test it (i.e. accumulation-per-property.html/addition-per-property.html) again to make sure the case you mentioned. Thanks for the reminder.
(In reply to Boris Chiou [:boris] from comment #45)
> Hi, Manish, Emilio,
> 
> I got a bug in bindgen. When I add "mozilla::gfx::Float" into
> ServoBindings.toml, its definition is |u32| [1]. However, in gecko
> |mozilla::gfx::Float| is defined as |float| [2] in gfx/2d/Types.h
> 
> [1] in structs_debug.rs (my local build)
>     pub mod gfx {
>         #[allow(unused_imports)]
>         use self::super::super::super::root;
>         pub type IntRegion = [u64; 3usize];
>         pub type Float = u32;
>         ...
>     }
> 
> [2]
> http://searchfox.org/mozilla-central/rev/
> 507743376d1ba753d14ab6b9305b7c6358570be8/gfx/2d/Types.h#18
> 
> Do you know what is the potential problem in rust-bindgen to generate this
> type? This result seems incorrect.

So this is because we mark as opaque all types in mozilla::gfx::*:

http://searchfox.org/mozilla-central/source/layout/style/ServoBindings.toml#264

So bindgen only tries to make it a type with the appropriate layout.

If we need more types from mozilla::gfx we need to make that condition more precise.
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #54)
> So this is because we mark as opaque all types in mozilla::gfx::*:
> 
> http://searchfox.org/mozilla-central/source/layout/style/ServoBindings.
> toml#264
> 
> So bindgen only tries to make it a type with the appropriate layout.
> 
> If we need more types from mozilla::gfx we need to make that condition more
> precise.

OK. I see. Thanks. So I have to let gfx::Float non-opaque at least.
Flags: needinfo?(manishearth)
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

Still need some changes for RawGeckoGfxMatrix4x4, so clear r?.
Attachment #8874294 - Flags: review?(bbirtles)
Comment on attachment 8872307 [details]
Bug 1335998 - Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list.

https://reviewboard.mozilla.org/r/143806/#review149938

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:273
(Diff revision 2)
> +        for (item, new_value) in list.unwrap().into_iter().zip(values) {
> +            *item = new_value;
> +        }

Ok thanks. Sorry, I didn't see part 3.

::: servo/components/style/properties/gecko.mako.rs:2393
(Diff revision 2)
> +                "list" : "TransformList(Some(convert_shared_list_to_operations(%s)))",
> +            }
> +            pre_symbols = "("
> +            post_symbols = ")"
> +            if keyword == "interpolatematrix" or keyword == "accumulatematrix":
> +                pre_symbols = " {"

Ok, it feels like there should be some other way of factoring this that would avoid this oddity, but for now a short comment describing the expected output might make this code less confusing for the next person who looks at it.
Attachment #8872307 - Flags: review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> (In reply to Boris Chiou [:boris] from comment #47)
> > e.g.
> > ```
> > [ InterpolateMatrix { from_list: ..., to_list: ... },
> >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > ```
> > This is not correct. We only creat one InterpolateMatirx for a list.
> 
> When an underlying value is InterpolateMatrix and a subsequent transform is
> additive, what happens? Also more additive or accumulative transform on the
> underlying InterpolateMatrix?

I haven't seen a case which concatenates two or more InterpolateMatrices until now, actually. In other words, I have no idea to make a underlying value be InterpolateMatrix by contaminating the iterationComposite: 'accumulate' and composite: 'add'. Besides, all transform related tests in {additive, accumulate}-per-property.html are passed. If you find one, please let me know to debug it. I believe the current implementation could support it, and only some assertions to make sure this case doesn't happen. Therefore, if it is possible, we can just remove the assertions. Thanks for the reminder.
(In reply to Boris Chiou [:boris] from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> > (In reply to Boris Chiou [:boris] from comment #47)
> > > e.g.
> > > ```
> > > [ InterpolateMatrix { from_list: ..., to_list: ... },
> > >   Interpolatematrix { from_list: ..., to_list: ... } ]
> > > ```
> > > This is not correct. We only creat one InterpolateMatirx for a list.
> > 
> > When an underlying value is InterpolateMatrix and a subsequent transform is
> > additive, what happens? Also more additive or accumulative transform on the
> > underlying InterpolateMatrix?
> 
> I haven't seen a case which concatenates two or more InterpolateMatrices
> until now, actually. In other words, I have no idea to make a underlying
> value be InterpolateMatrix by contaminating the iterationComposite:
> 'accumulate' and composite: 'add'. Besides, all transform related tests in
> {additive, accumulate}-per-property.html are passed. If you find one, please
> let me know to debug it. I believe the current implementation could support
> it, and only some assertions to make sure this case doesn't happen.
> Therefore, if it is possible, we can just remove the assertions. Thanks for
> the reminder.

I am sorry for the confusion. I meant we have multiple *nested* InterpolateMatrix into a list.
say;
　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, { duration: 1000 });
  div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, { duration: 10, composite: 'add', iterationComposite: 'accumulate' });
(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> I am sorry for the confusion. I meant we have multiple *nested*
> InterpolateMatrix into a list.
> say;
> 　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, {
> duration: 1000 });
>   div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, {
> duration: 10, composite: 'add', iterationComposite: 'accumulate' });

Thanks for this case! Let me double-check it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> I am sorry for the confusion. I meant we have multiple *nested*
> InterpolateMatrix into a list.
> say;
> 　div.animate({ transform: [ 'rotate(100deg)', 'translateX(100px)' ] }, {
> duration: 1000 });
>   div.animate({ transform: [ 'scale(2)', 'translateY(200px)' ] }, {
> duration: 10, composite: 'add', iterationComposite: 'accumulate' });

I see. So we may have a nested InterpolateMatrix like this (so complicated):

[
  InterpolateMatrix { from_list: [ AccumulateMatrix { from_list: ...,
                                                      to_list: [ InterpolateMatrix { from_list: ...,
                                                                                     to_list: ...,
                                                                                     progress: ... },
                                                                 Scale(2, 2, 1) ],
                                                      count: 2 } ])),
                      to_list: [ AccumulateMatrix { from_list: ...,
                                                    to_list: [ InterpolateMatrix { from_list: ...,
                                                                                   to_list: ...,
                                                                                   progress: ... },
                                                               Translate(0px, 20px, 0px) ],
                                                    count: 2 } ],
                      progress: Percentage(0.7808252) }
]

In other words, an InterpolateMatrix could be concatenated with other ComputedOperations. I will remove that assertion. Thanks for this example.
Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review149588

> Do we need to build an identity transform list here?
> 
> The corresponding code on the Gecko side is just:
> 
>   resultList = AddTransformLists(0.0, listA, aCount, listA,
>                                  eCSSKeyword_accumulatematrix);
> 
> Could we do the same here:
> 
>   Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))
> 
> Does that work?
> 
> (And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)

This simplication works. Thanks!

And this is not the only place we need build_identity_transform_list for accumulatematrix:
e.g.
```
div {
  transfrom: rotate(45deg)
}

div.animate( [ { transform: 'translateX(100px)', composite: 'accumulate' },
               { transform: 'none' } ],
             1000);
```

We create an AccumualteMatrix { from_list: Rotate(45deg), to_list: translateX(100px), count: ... }, and do the interpolation of this AccumulateMatrix and none, which calls add_weighted(). So we may build an identity transform from add_weighted(). Therefore, I think we still need to handle AccumulateMatrix. (i.e. return an identity matrix for it)
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review150258
Attachment #8874294 - Flags: review?(manishearth) → review+
(In reply to Boris Chiou [:boris] (away 6/9~6/12) from comment #62)
> Comment on attachment 8874292 [details]
> Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.
> 
> https://reviewboard.mozilla.org/r/145560/#review149588
> 
> > Do we need to build an identity transform list here?
> > 
> > The corresponding code on the Gecko side is just:
> > 
> >   resultList = AddTransformLists(0.0, listA, aCount, listA,
> >                                  eCSSKeyword_accumulatematrix);
> > 
> > Could we do the same here:
> > 
> >   Ok(add_weighted_transform_lists(from_list, from_list, count as f64, 0.0))
> > 
> > Does that work?
> > 
> > (And is this the only place we need build_identity_transform_list to work with interpolatematrix/accumulatematrix?)
> 
> This simplication works. Thanks!
> 
> And this is not the only place we need build_identity_transform_list for
> accumulatematrix:
> e.g.
> ```
> div {
>   transfrom: rotate(45deg)
> }
> 
> div.animate( [ { transform: 'translateX(100px)', composite: 'accumulate' },
>                { transform: 'none' } ],
>              1000);
> ```
> 
> We create an AccumualteMatrix { from_list: Rotate(45deg), to_list:
> translateX(100px), count: ... }, and do the interpolation of this
> AccumulateMatrix and none, which calls add_weighted(). So we may build an
> identity transform from add_weighted(). Therefore, I think we still need to
> handle AccumulateMatrix. (i.e. return an identity matrix for it)

Ok, so as I understand it, in this case we will do:

  (&Some(ref from_list), &None) => {
      // http://dev.w3.org/csswg/css-transforms/#none-transform-animation
      let to_list = build_identity_transform_list(from_list);
      add_weighted_transform_lists(from_list, &to_list, self_portion, other_portion)
  }

build_identity_transform_list will do:

  TransformOperation::AccumulateMatrix { .. } => {
      // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
      let identity = ComputedMatrix::identity();
      result.push(TransformOperation::Matrix(identity));
  }

then we will pass that to add_weighted_transform_lists which does:

  if can_interpolate_list(from_list, to_list) {

But can_interpolate_list returns false so we just do:

  } else {
      use values::specified::Percentage;
      let from_transform_list = TransformList(Some(from_list.to_vec()));
      let to_transform_list = TransformList(Some(to_list.to_vec()));
      result.push(
          TransformOperation::InterpolateMatrix { from_list: from_transform_list,
                                                  to_list: to_transform_list,
                                                  progress: Percentage(other_portion as f32) });
  }

Strictly speaking we don't need to generate an identity matrix in this case. We could just pass in the from_transform twice. That is, after all, what we do in Gecko:

  if (HasAccumulateMatrix(list1)) {
    result = AddDifferentTransformLists(0, list1,
                                        aCoeff1, list1,
                                        eCSSKeyword_interpolatematrix);
  } else {
    result = AddTransformLists(0, list1, aCoeff1, list1);
  }

But this seems reasonable too.

I still think we should explain why identity is a suitable value to return in this case.
Comment on attachment 8872306 [details]
Bug 1335998 - Part 1: Define ComputedOperation::InterpolateMatrix.

https://reviewboard.mozilla.org/r/143804/#review150418

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1642
(Diff revision 3)
>              TransformOperation::Perspective(..) => {
>                  // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
>                  let identity = ComputedMatrix::identity();
>                  result.push(TransformOperation::Matrix(identity));
>              }
> +            TransformOperation::InterpolateMatrix { .. } => panic!("Not supported"),

s/Not supported/Interpolation of InterpolateMatrix is not supported/ ?

::: servo/components/style/properties/longhand/box.mako.rs:1185
(Diff revision 3)
>                        computed::Length),
>              Scale(CSSFloat, CSSFloat, CSSFloat),
>              Rotate(CSSFloat, CSSFloat, CSSFloat, computed::Angle),
>              Perspective(computed::Length),
> +            // For mismatched transform lists.
> +            // A vector of |ComptuedOperation| could contain an |InterpolateMatrix| and other

ComputedOperation
Attachment #8872306 - Flags: review?(bbirtles) → review+
Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review150434

::: servo/components/style/properties/helpers/animated_properties.mako.rs:662
(Diff revision 2)
> +                        }
> +                    % endif
> +                % endif
> +            % endfor
> +            _ => {
> +                panic!("Expected Expected accumulation of computed values of the same \

Expected

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1663
(Diff revision 2)
>                  // http://dev.w3.org/csswg/css-transforms/#identity-transform-function
>                  let identity = ComputedMatrix::identity();
>                  result.push(TransformOperation::Matrix(identity));

This comment doesn't make sense. We should explain why identity is a suitable value to use here.
Comment on attachment 8874294 [details]
Bug 1335998 - Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo.

https://reviewboard.mozilla.org/r/145564/#review149604

::: commit-message-1a600:6
(Diff revision 1)
> +Note: There are some tiny differences between interpolation results of 2d/3d
> +      matrices on Gecko and Servo, especially if there is skew() or any 3d
> +      transform function.

What are the differences / why?

(We don't necessarily need a full explanation, just something that makes this less mysterious. e.g. "due differences in precision used to represent the components" etc.)
Attachment #8874294 - Flags: review?(bbirtles) → review+
Comment on attachment 8874292 [details]
Bug 1335998 - Part 2: Define ComputedOperation::AccmulateMatrix.

https://reviewboard.mozilla.org/r/145560/#review150434

> This comment doesn't make sense. We should explain why identity is a suitable value to use here.

Understood. I will update this.
Attachment #8872306 - Attachment is obsolete: true
Attachment #8874292 - Attachment is obsolete: true
Attachment #8874293 - Attachment is obsolete: true
Attachment #8872308 - Attachment is obsolete: true
Attached file Servo PR, #17202 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86c0fdac3bc2
Part 1: Implement Gecko_CSSValue_InitSharedList and read the transform from the shared list. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/ab49fc64a4bf
Part 2: Delegate matrix decomposition/interpolation/recomposition to Servo. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/1df01865d100
Part 3: Add a crashtest for mismatched transform lists. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/86c0fdac3bc2
https://hg.mozilla.org/mozilla-central/rev/ab49fc64a4bf
https://hg.mozilla.org/mozilla-central/rev/1df01865d100
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
  
Blocks: 1395881

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: