shahul.vm Reporter
	Description • 8 years ago

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)

	Loic
	Comment 1 • 8 years ago

Could you attach the SVG testcase, please.

	shahul.vm Reporter
	Comment 2 • 8 years ago

please find my attached sample

	Alice0775 White
	Comment 3 • 8 years ago

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

Regressed by : Bug 1097464

	Thinker Li [:sinker]
	Comment 4 • 8 years ago

Roger! I would look into it later.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 5 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 6 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 7 • 8 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 8 • 8 years ago

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".

	Thinker Li [:sinker]
	Comment 9 • 8 years ago

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.

	Thinker Li [:sinker]
	Comment 10 • 8 years ago

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.

	Thinker Li [:sinker]
	Comment 11 • 8 years ago

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.

	Andrew Overholt [:overholt]
	Comment 12 • 8 years ago

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 ...

	Thinker Li [:sinker]
	Comment 13 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 14 • 8 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 8 years ago

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.

	Andrew Overholt [:overholt]
	Comment 16 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 17 • 8 years ago

(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 ...

	Thinker Li [:sinker]
	Comment 18 • 8 years ago

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

	Andrew Overholt [:overholt]
	Comment 19 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 20 • 8 years ago

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

Please also check comment 15.

	Matt Woodrow (:mattwoodrow)
	Comment 21 • 8 years ago

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.

	Astley Chen (inactive)
	Comment 22 • 8 years ago

Tool late and risky for 51.

Thinker, are you still working on this bug?

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 8 years ago

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

	Astley Chen (inactive)
	Comment 24 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 25 • 8 years ago

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.

	Thinker Li [:sinker]
	Comment 26 • 8 years ago

rebase

	Thinker Li [:sinker]
	Comment 27 • 8 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 28 • 8 years ago

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

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

Looks good!

	Thinker Li [:sinker]
	Comment 29 • 8 years ago

Hi Matt, had you read comment 27?

	Matt Woodrow (:mattwoodrow)
	Comment 30 • 8 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 8 years ago

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

	Thinker Li [:sinker]
	Comment 32 • 7 years ago

(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.

	Julien Cristau [:jcristau]
	Comment 33 • 7 years ago

Mass wontfix for bugs affecting firefox 52.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 34 • 7 years ago

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.

	Astley Chen (inactive)
	Comment 35 • 7 years ago

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

	Astley Chen (inactive)
	Comment 36 • 7 years ago

Based on comment 35.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 37 • 7 years ago

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

	u459114
	Comment 38 • 7 years ago

A simpler version. Remove animation and other unrelative stuffs

	u459114
	Comment 39 • 7 years ago

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

	u459114
	Comment 40 • 7 years ago

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.