Add support for color animated properties #2149

nc4rrillo · 2017-12-01T07:10:59Z

Fixes: #2118

Hi,
This is a patch to add support for animated color properties in WebRender.

Some things to note:

I modified the animations sample to show off the color animations. I hope my change is okay :).
There is an assertion that popped up as a result of me calling gpu_cache.invalidate() in prepare_prim_for_render.

STR for the assertion is:

Open animations sample
Hit ',' or ',.' to rotate the rects until they go off screen.

Things to do before merging:

Fix assertion in gpu_cache.rs line 612
Decide on default value for color bindings
Investigate border glitch
Rebase
Measure serialization/deserialization impact
Bump WR API version

This change is

nc4rrillo · 2017-12-01T21:37:19Z

backtrace of the assertion mentioned in the PR description:

thread 'WRRenderBackend#0' panicked at 'assertion failed: `(left == right)`
  left: `Epoch(16)`,
 right: `Epoch(15)`', src/gpu_cache.rs:612:8
stack backtrace:
   0:     0x560cf93306d3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h5ffdc97e68d73ef0
                               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x560cf932cb80 - std::sys_common::backtrace::_print::h5927dee13340444c
                               at /checkout/src/libstd/sys_common/backtrace.rs:69
   2:     0x560cf93328e3 - std::panicking::default_hook::{{closure}}::h3522a43e516ec509
                               at /checkout/src/libstd/sys_common/backtrace.rs:58
                               at /checkout/src/libstd/panicking.rs:381
   3:     0x560cf9332655 - std::panicking::default_hook::hab587947abeefc97
                               at /checkout/src/libstd/panicking.rs:397
   4:     0x560cf9332d77 - std::panicking::rust_panic_with_hook::hd4c05252aed3c4fa
                               at /checkout/src/libstd/panicking.rs:577
   5:     0x560cf9332bec - std::panicking::begin_panic::h1363ae4ae483138a
                               at /checkout/src/libstd/panicking.rs:538
   6:     0x560cf9332b69 - std::panicking::begin_panic_fmt::h7a41a337a38e2164
                               at /checkout/src/libstd/panicking.rs:522
   7:     0x560cf8bc4ce4 - webrender::gpu_cache::GpuCache::get_address::h51d51a0e1440fd7a
                               at src/gpu_cache.rs:612
   8:     0x560cf8bc6388 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:395
   9:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  10:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  11:     0x560cf8bc7957 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:804
  12:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  13:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  14:     0x560cf8bc7957 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:804
  15:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  16:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  17:     0x560cf8bc7957 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:804
  18:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  19:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  20:     0x560cf8bc7957 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:804
  21:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  22:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  23:     0x560cf8bc7957 - webrender::tiling::add_to_batch::he357d026bffe0069
                               at src/tiling.rs:804
  24:     0x560cf8d1f407 - webrender::tiling::<impl webrender::prim_store::PrimitiveRun>::add_to_batch::h449f3025c24bb52e
                               at src/tiling.rs:72
  25:     0x560cf8b1b590 - webrender::tiling::<impl webrender::picture::PicturePrimitive>::add_to_batch::h7646a9dc3b5c1458
                               at src/tiling.rs:957
  26:     0x560cf8bca16b - webrender::tiling::AlphaBatcher::build::h1fc443cd6ca0ea84
                               at src/tiling.rs:1036
  27:     0x560cf8bcc2a4 - <webrender::tiling::ColorRenderTarget as webrender::tiling::RenderTarget>::build::hdf096be533dadd43
                               at src/tiling.rs:1407
  28:     0x560cf8bce31a - webrender::tiling::RenderPass::build::h17db9bc27270c8dd
                               at src/tiling.rs:1805
  29:     0x560cf8c37cba - webrender::frame_builder::FrameBuilder::build::h63c1c07d28020cc2
                               at src/frame_builder.rs:1790
  30:     0x560cf8cdd00a - webrender::frame::FrameContext::build_rendered_document::hb60abb554f1ea2a5
                               at src/frame.rs:1185
  31:     0x560cf8b10487 - webrender::render_backend::Document::render::hdd3603e1a2065eba
                               at src/render_backend.rs:117
  32:     0x560cf8b11f48 - webrender::render_backend::RenderBackend::process_document::h01d60d63fd9491f6
                               at src/render_backend.rs:419
  33:     0x560cf8b12a1d - webrender::render_backend::RenderBackend::run::h0fd1753afa3b1576
                               at src/render_backend.rs:491
  34:     0x560cf8d64fe8 - webrender::renderer::Renderer::new::{{closure}}::hc85f574d1c07b4d4
                               at src/renderer.rs:1988
  35:     0x560cf8d69184 - std::sys_common::backtrace::__rust_begin_short_backtrace::h904e1df1c5a4e253
                               at /checkout/src/libstd/sys_common/backtrace.rs:134
  36:     0x560cf8c15b0b - std::thread::Builder::spawn::{{closure}}::{{closure}}::hb24e4516680d4ee3
                               at /checkout/src/libstd/thread/mod.rs:402
  37:     0x560cf8ce0574 - <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h77ee506dc5a53b74
                               at /checkout/src/libstd/panic.rs:296
  38:     0x560cf8ce4c94 - std::panicking::try::do_call::h213fb5694738b52d
                               at /checkout/src/libstd/panicking.rs:480
  39:     0x560cf933b5be - __rust_maybe_catch_panic
                               at /checkout/src/libpanic_unwind/lib.rs:99
  40:     0x560cf8ce470c - std::panicking::try::h74dffcfbd24d1b09
                               at /checkout/src/libstd/panicking.rs:459
  41:     0x560cf8ce3d9c - std::panic::catch_unwind::hd3998450aacf89fd
                               at /checkout/src/libstd/panic.rs:361
  42:     0x560cf8c1595d - std::thread::Builder::spawn::{{closure}}::he3d1d5e244f17f2f
                               at /checkout/src/libstd/thread/mod.rs:401
  43:     0x560cf8c1cd95 - <F as alloc::boxed::FnBox<A>>::call_box::hf6386e2f3b2a3639
                               at /checkout/src/liballoc/boxed.rs:762
  44:     0x560cf9331d5b - std::sys::imp::thread::Thread::new::thread_start::h783d2fade112e868
                               at /checkout/src/liballoc/boxed.rs:772
                               at /checkout/src/libstd/sys_common/thread.rs:24
                               at /checkout/src/libstd/sys/unix/thread.rs:90
  45:     0x7fdd453b36d9 - start_thread
  46:     0x7fdd44ed6d7e - __clone
  47:                0x0 - <unknown>

glennw · 2017-12-04T05:00:24Z

Wow, this is amazing work. Apologies I haven't gotten to it yet - will run through this tomorrow.

Not sure if relevant, but I think it's completely fine to do this incrementally (e.g. merge support for just animated color on rects, then follow up with other primitives etc), if that makes things easier.

bors-servo · 2017-12-04T05:42:51Z

☔ The latest upstream changes (presumably #2146) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2017-12-05T04:53:26Z

Reviewed 23 of 23 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

webrender/examples/animation.rs, line 296 at r1 (raw file):

                y += 200.;
            }

nit: extra spaces

webrender/src/frame_builder.rs, line 844 at r1 (raw file):

            &PropertyBinding::Value(value) => PropertyBinding::Value(value.premultiplied()),
            &PropertyBinding::Binding(binding) => {
                let property_binding_key : PropertyBindingKey<PremultipliedColorF> = unsafe { mem::transmute(binding.key) };

If we were storing the value outside the property, then that would probably make this a lot simpler and remove the transmute nastiness you mentioned?

webrender/src/prim_store.rs, line 1434 at r1 (raw file):

                PrimitiveKind::Rectangle => {
                    let mut rect = &mut self.cpu_rectangles[metadata.cpu_prim_index.0];

nit: extra spaces

wrench/src/rawtest.rs, line 201 at r1 (raw file):

            builder.push_clip_id(clip);
            builder.push_rect(&PrimitiveInfo::new(rect(100., 100., 100., 100.)),
                              ColorF::new(0.0, 0.0, 1.0, 1.0).into());

Perhaps we could instead have the interface be something like:

fn push_rect<P>(&mut self, color: P) where P: Into<PropertyBinding<ColorF>>

So that you can push a color directly in the common case?

wrench/src/yaml_frame_writer.rs, line 739 at r1 (raw file):

                    println!("TODO YAML YuvImage");
                }
                Border(mut item) => {

Having to use all the mutability on stored items is why I originally had the resolved value stored outside the property binding. There's pros and cons to each.

Comments from Reviewable

glennw · 2017-12-05T04:55:39Z

This generally looks good to me! My main questions are whether (a) storing the resolved value outside the binding would remove a lot of the extra mutability here and also fix the transmute in the PR, and (b) if we should use the Into trick mentioned above to simplify the case for callers that are just providing colors without a binding.

nc4rrillo · 2017-12-05T05:03:09Z

@glennw that Into trait is a good idea. I agree with moving the resolved value out of PropertyBinding. I'll make that change as well.

I'm also going to revert my changes to add this animated color support to borders and save that for a follow up.

jrmuizel · 2017-12-05T05:10:44Z

I'm a little worried about the additional serialization/deserialization and display list size cost this might add. Should we measure it?

glennw · 2017-12-05T06:02:23Z

@jrmuizel Yea, I'll do some measurements before merging anything.

nc4rrillo · 2017-12-05T07:06:21Z

I've rebased this again and implemented the changes you suggested.

The Into<PropertyBinding<ColorF>> is very good in that very little places need an API change.
AFAICT it should only be places that use GradientStops since create_gradient takes a Vec<GradientStop> directly.

I also reverted my change to PropertyBinding<T> to remove some of the mutability in my earlier changes. This resulted in me having to pass SceneProperties around to some of the FrameBuilder methods which I hope is alright.

For now only rects, box shadows, and gradients have color animation support.

Besides the assertion, another open question I have is about what good behavior for an initial value for color bindings would be. Should the client be expected to GenerateFrame with the initial value? Or should I try to do something similar to what FilterOp does with opacity?

cc: @glennw

nc4rrillo · 2017-12-05T07:35:52Z

webrender/src/prim_store.rs

+                    (None, true, true)
+                },
+                PrimitiveKind::AlignedGradient | PrimitiveKind::AngleGradient | PrimitiveKind::RadialGradient => {
+                    gpu_cache.invalidate(&metadata.gpu_location);


I don't like that I unconditionally invalidate here.

nc4rrillo · 2017-12-05T07:36:34Z

webrender/src/prim_store.rs

+                PrimitiveKind::Brush => {
+                    let brush = &mut self.cpu_brushes[metadata.cpu_prim_index.0];
+                    if let BrushKind::Solid { ref mut color, ref binding } = brush.kind {
+                        *color = scene_properties.resolve_color(&binding);


Maybe I should add an if let &PropertyBinding::Binding(_) = binding here so I'm not trying to resolve PropertyBinding::Value properties?

nc4rrillo · 2017-12-05T07:43:56Z

Review status: 0 of 14 files reviewed at latest revision, 7 unresolved discussions.

webrender/examples/animation.rs, line 296 at r1 (raw file):