1271983 - Animated SVG image is blurred in FF 46

Status: NEW

(Core :: SVG, defect, P3)

Description

[shahul.vm]

Attachment: thf-issue-ff.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

I created a banner animation with HTML5, SVG & JS.
Please find my attachment.

SVG arrow is moving from right to left and scaling is also applied to it.


Actual results:

SVG arrow is blurred. In ff46(in all other versions it is working fine)

Comment 1

[Loic]

Could you attach the SVG testcase, please.

Comment 2

[shahul.vm]

Attachment: ff-bug-eg.zip

please find my attached sample

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54

Regressed by : Bug 1097464

Comment 4

[Thinker Li [:sinker]]

Roger! I would look into it later.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Thinker, do you think you'll get to this soon?

Comment 6

[Thinker Li [:sinker]]

I would go back to this bug in next few days.

Comment 7

[Thinker Li [:sinker]]

Attachment: index-html.diff

This patch would fix the problem of this bug.

Now, Gecko would reuse the layer and content for this case.  That means Gecko would not repaint the surfaces of the layers.  So, you would get blur for scaling-up the image.  So, it would fix the problem by using a bigger image (change height of the image for SVG), and scale it to the proper size.

This would improve the FPS of animations, but with the side-effect like this.  I am not sure if we should fix it.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks Thinker - just so that I understand.  What was done in bug 1097464 has sped up the SVG example attached, but results in the blurring reported.  Changing the Gecko code so that the blurring is gone, would mean slowing things down - is that even an option we have?

As an alternative, your attachment changes the example so that the author can choose between the "fast and blurry" and "slow and sharp".

Comment 9

[Thinker Li [:sinker]]

For now, if the size of an image is too small, even a SVG, it would not draw a bigger picture.  So, the user would see a blurred image after scaling up.  And, Gecko don't use post-scale for painted layers now.  So, I think we could apply a post scale to painted layers and draw bigger surfaces, the we could get a clear SVG image even being scaled-up.  Although, it don't fix the problem of scaling-up during an animation running at the compositor.

Comment 10

[Thinker Li [:sinker]]

Attachment: WIP.diff

This patch apply a pre-scale on the container layer to make the painted layer where the image is painted on being rendered in bigger size.

For the case here, the image is scaled up to a big size at beginning.  So, once it is rendered in the surface of the painted layer, it is big enough to being used during animation.

Comment 11

[Thinker Li [:sinker]]

Attachment: WIP-v1.diff

This patch keep the pre-scale of a layer to it's biggest value ever.
For preserve3d layers, their pre-scale would consider the scaling applied by the accumulated (effective) transform.  This make sure the painted layer would be rendered in the size big enough.

And, I have found not only SVG image would be blurred, but also other HTML content.  So, even you place the image with a div and text inside it, the content of the div is also blurred.

Comment 12

[Andrew Overholt [:overholt]]

Thinker, have you had any recent progress here (I know you're on PTO until next week)?

It seems we may not want to back out the regressing patch here ...

Comment 13

[Thinker Li [:sinker]]

I will update the patch today, and start the review.

Comment 14

[Thinker Li [:sinker]]

Attachment: Use pre-scale/post-scale for extend-3d layers

This patch would change pre-scale of container layer, and set a post-scale on child layers.  |PaintedLayerData::Accumulate()| would consider the scales of |ContainerState| to scale bounds of painted layers.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8797075 [details] [diff] [review]
Use pre-scale/post-scale for extend-3d layers

Review of attachment 8797075 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +5170,4 @@
>      scale = gfxSize(1.0, 1.0);
>    }
>  
> +  if (aContainerFrame->Combines3DTransformWithAncestors()) {

Shouldn't we deal with async animations here (given that we're going to allow them now).

This code seems like it'll pick a scale based on the current transform, so could change depending on where we are in the animation. If we're running the animation async, but doing ocassional paints for other reasons, then the rendered content will change resolution randomly.

@@ +5190,5 @@
> +      nsDisplayTransform::INCLUDE_PRESERVE3D_ANCESTORS |
> +      nsDisplayTransform::INCLUDE_PERSPECTIVE |
> +      nsDisplayTransform::OFFSET_BY_ORIGIN;
> +    Matrix4x4 p3dTransform =
> +      nsDisplayTransform::GetResultingTransformMatrix(aContainerFrame,

Can't we just use the layer transform and combine it with the parent layer transforms rather than computing this again from scratch?

@@ +5196,5 @@
> +                                                      appunit2pixel,
> +                                                      flags);
> +
> +    // Compute the sizes of sides after the transformation.
> +    Point origin = p3dTransform.TransformPoint(Point3D(0, 0, 0)).As2DPoint();

You should use aContainerFrame->GetRect().x/y here and below, I don't think moving the rect to 0 gives you the same results.

Comment 16

[Andrew Overholt [:overholt]]

Any chance of getting this fixed and in a shape we'd consider uplifting to 50?

Comment 17

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Comment on attachment 8797075 [details] [diff] [review]
> Use pre-scale/post-scale for extend-3d layers
> 
> Review of attachment 8797075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +5170,4 @@
> >      scale = gfxSize(1.0, 1.0);
> >    }
> >  
> > +  if (aContainerFrame->Combines3DTransformWithAncestors()) {
> 
> Shouldn't we deal with async animations here (given that we're going to
> allow them now).
> 
> This code seems like it'll pick a scale based on the current transform, so
> could change depending on where we are in the animation. If we're running
> the animation async, but doing ocassional paints for other reasons, then the
> rendered content will change resolution randomly.

It is probable having multiple animated layers.  The result is complicated, for example, multiple rotations on different container layers.  It appears complicated to find a scale.

The idea here is to make the image clear at least for the case it has been scaled up at beginning.
And, the image would eventually be clear after the stop of animations since a repaint would be triggered.  The down side is as what you mentioned.

> 
> @@ +5190,5 @@
> > +      nsDisplayTransform::INCLUDE_PRESERVE3D_ANCESTORS |
> > +      nsDisplayTransform::INCLUDE_PERSPECTIVE |
> > +      nsDisplayTransform::OFFSET_BY_ORIGIN;
> > +    Matrix4x4 p3dTransform =
> > +      nsDisplayTransform::GetResultingTransformMatrix(aContainerFrame,
> 
> Can't we just use the layer transform and combine it with the parent layer
> transforms rather than computing this again from scratch?

Yes, I think we can pass this information along with ContainerLayerParameters.

... skip ...

Comment 18

[Thinker Li [:sinker]]

Andrew, I will have a long vocation one week later.  I am not sure I can land it before that.

Comment 19

[Andrew Overholt [:overholt]]

OK, sounds like a fix-optional for 50. Thanks.

Comment 20

[Thinker Li [:sinker]]

Attachment: Use pre-scale/post-scale for extend-3d layers, v2

Hi Matt,
This patch pass parent's effective matrix to children for container layers with extend3d.

Please also check comment 15.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8801628 [details] [diff] [review]
Use pre-scale/post-scale for extend-3d layers, v2

Ok, this is still really complex and it suffers from the same problem as earlier (if we have an async animation, then the chosen scale factors will be effectively random, determined by when we happen to schedule main thread paints).

I had a look at the test case, and it doesn't actually do anything with 3d transforms or perspective, it just uses preserve-3d (which doesn't change anything).

How about we just allow the existing code to run if preserve-3d is set? It's not obvious why we can't pass scale factors down through 2d preserve-3d transforms.

That will fix this testcase without us having to worry about how to deal with scale factors for true perspective/3d-transformed content.

Comment 22

[Astley Chen (inactive)]

Tool late and risky for 51.

Thinker, are you still working on this bug?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Astley, is there anything you can do to help get this bug moving again?

Comment 24

[Astley Chen (inactive)]

Let me check with Thinker in person and update here. Leave ni?

Comment 25

[Thinker Li [:sinker]]

Attachment: Scale preserve-3d container layers

I would like to provide a complete fixing.  However this bug is opened for a long while, it is more reasonable to provide a quick and acceptable fix.

This patch just let pre-scale does it's job whenever there is no effective perspective transform.  That means no any ancestor in the same 3D rendering context having perspective property.

Comment 26

[Thinker Li [:sinker]]

Attachment: Scale preserve-3d container layers, v2

rebase

Comment 27

[Thinker Li [:sinker]]

Comment on attachment 8823546 [details] [diff] [review]
Scale preserve-3d container layers, v2

I have found a problem from reftest if we use scale factors for layers without regarding preserve-3d.  Some painted layers would be scaled down at a container layer, but be scaled up at parent or one of ancestors.  It makes result output blurred too.

see https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/TX94WPbiQomb8CttZFxx7w/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8823546 [details] [diff] [review]
Scale preserve-3d container layers, v2

Review of attachment 8823546 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 29

[Thinker Li [:sinker]]

Hi Matt, had you read comment 27?

Comment 30

[Matt Woodrow (:mattwoodrow)]

Can you explain why that happens?

Shouldn't we be combining the scale factors as we move down through the 4 transforms and end up with 1.0 to put on the PaintedLayer?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Thinker, looks like this bug has been stuck for the last month or so? Any chance we can get it moving along again?

Comment 32

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> Can you explain why that happens?
> 
> Shouldn't we be combining the scale factors as we move down through the 4
> transforms and end up with 1.0 to put on the PaintedLayer?

Apparently, painted layers are not sharing the same behavior.
I would check if it is possible to apply a pre-scaling on painted layer.

Comment 33

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Seems so close to fixing.... But not close enough for 53. Maybe for 54/55!  
Is this a widespread problem? Do we know? I don't see duplicate bugs reported, so maybe it doesn't need to be a priority.

Comment 35

[Astley Chen (inactive)]

As Thinker is not active on this bug now, I'll leave it unassigned and have someone to follow up.

Comment 36

[Astley Chen (inactive)]

Based on comment 35.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Not a recent regression, seems like an edge case, so I'll leave this to the SVG backlog.

Comment 38

[u459114]

Attachment: ff-bug-eg-2.tar.gz

A simpler version. Remove animation and other unrelative stuffs

Comment 39

[u459114]

keep ni, since I am still figuring out what happened here

Comment 40

[u459114]

ff-bug-eg-2.tar.gz, I also add an inline SVG element in 3D context. Only SVG-as-image becomes blur, the rendering result of inline SVG is super ok.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:jwatt, since the bug has high priority, could you have a look please?

For more information, please visit auto_nag documentation.