660565 - HTML5 HW accelerated video is slow

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Even with GPU YUV decoding video still very slow, because we rendering video layer using intermediate surface in ContainerLayerOGL.
Flash video works fine, but not HTML5 video. with HTML5 video we have non integer translation and non-empty clipRect.... don't know what causing that, possibly some video controls, or something else.

With disabled intermediate surface (hackish) youtube HTML5 video works fast

Comment 1

[Bas Schouten (:bas.schouten)]

We should consider retaining intermediate surfaces. That might very well help a lot. The one downside is depending on how deep the tree is the amount of intermediate surfaces retained could grow large. We could retain a limited amount though, depending on certain heuristics.

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Cache temportary surface

We can cache temporary container surface

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Cache offscreen FBO, for OGL container,

This will help us get rid of expensive create/destroy offscreen surface which is very expensive in SGX drivers

Comment 4

[Oleg Romashin (:romaxa)]

Hmm, I've checked latest trunk with flash plugin rendering and it also always rendering with Intermediate surface.... but it did not happen before... seems it some regression.

Comment 5

[Oleg Romashin (:romaxa)]

Initial regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94042ef440fa&tochange=15bfb9729ff3

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 536138 [details] [diff] [review]
Cache offscreen FBO, for OGL container,

Latest regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6a76f02f19e1&tochange=f7fc3beccae7

Not sure  but it might be caused by bug 654950

Comment 7

[Oleg Romashin (:romaxa)]

Ok, after reversion of http://hg.mozilla.org/mozilla-central/rev/ed0850a1bbb7 video start working fast without intermediate surface

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How does that patch affect whether an intermediate surface is used? I don't see how it can.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, this?

-        !contTransform.PreservesAxisAlignedRectangles()) {
+        contTransform.HasNonIntegerTranslation()) {

OK.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why does the child have a cliprect?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could revert just that change, but someone would have to also fix the scissor-rect calculation code, which was completely wrong before.

Comment 12

[Oleg Romashin (:romaxa)]

> -        !contTransform.PreservesAxisAlignedRectangles()) {
> +        contTransform.HasNonIntegerTranslation()) {
> 
> OK.

yeah, this one... is it ok to revert just this change or is it super important for bug 654950 ?

Comment 13

[Oleg Romashin (:romaxa)]

reverting only that one change breaking layer rendering... I see only bottom part of image layer rendered on youtube.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. See comment #11.

Comment 15

[Oleg Romashin (:romaxa)]

Is it possible to make some test with certain layout which is checking this intermediate surface issue

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Backout patch

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Backout 654950, removed missing test

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539668 [details] [diff] [review]
Backout 654950, removed missing test

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

Why are you backing it out? Isn't this just going to reopen bug 654950?

We should be changing just the line in comment #9 and then fixing CalculateScissorRect to handle non-translation transforms.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539668 [details] [diff] [review]
Backout 654950, removed missing test

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

Why are you backing it out? Isn't this just going to reopen bug 654950?

We should be changing just the line in comment #9 and then fixing CalculateScissorRect to handle non-translation transforms.

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: use matr.transform + round in case of non-integer translation

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539702 [details] [diff] [review]
use matr.transform + round in case of non-integer translation

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

::: gfx/layers/Layers.cpp
@@ +339,5 @@
> +      gfxRect trScissor = matrix.TransformBounds(r);
> +      trScissor.Round();
> +      if (!gfxUtils::GfxRectToIntRect(trScissor, &scissor)) {
> +        return nsIntRect(currentClip.TopLeft(), nsIntSize(0, 0));
> +      }

This is the right idea.

Let's just take this code path always. No need to check HasNonIntegerTranslation, this code path works fine for the integer translation case.

But add an assertion that at least the transform has PreservesAxisAlignedRectangles.

@@ +413,5 @@
>    } else {
>      useIntermediateSurface = PR_FALSE;
>      gfxMatrix contTransform;
>      if (!mEffectiveTransform.Is2D(&contTransform) ||
> +        !contTransform.PreservesAxisAlignedRectangles()) {

I think we should probably use OPTIMIZE_MOBILE here; when the GPU is sufficiently powerful that the intermediate surface is not a problem, we should get slightly better results by not rounding the clip.

Comment 22

[Oleg Romashin (:romaxa)]

Attachment: Round NonIntegerTranslation on mobile

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile

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

Comment 24

[Oleg Romashin (:romaxa)]

Try server looks good, please push it
http://tbpl.mozilla.org/?tree=Try&rev=85da463bad9c

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5a46d17c72ca

Comment 26

[Matt Brubeck (:mbrubeck)]

This regression is present in Firefox 6.  Is this fix suitable for landing on the Aurora channel?  If so, please request mozilla-approval-aurora.

Comment 27

[Oleg Romashin (:romaxa)]

Comment on attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile

I think this is relevant for desktop versions (which are using OGL backend).
Not sure if it is relevant for mobile like android because it in software mode... but let's see

Comment 28

[Matt Brubeck (:mbrubeck)]

If it affects Android, it could explain bug 661606.

Comment 29

[Oleg Romashin (:romaxa)]

Ah, ok I see, probably make sense and this could be a reason

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's give this sime time to bake on trunk before moving to aurora. Leaving nom'ed for the next triage.

Comment 31

[[:Cww]]

May be causing bug 659119?

Comment 32

[Asa Dotzler [:asa]]

Can we get a risk analysis here? It's been baking for a week now.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pretty much no risk on desktop, more on mobile.

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile

Mark says they would like this for mobile and given that the risk is low to desktop and the Mobile folks are OK with their risk, I'm approving.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> May be causing bug 659119?

This isn't in FF5 or FF6, so no.

Comment 36

[[:Cww]]

(In reply to comment #35)
Sorry, I meant the bug, not the patch.  As in the patch might also address bug 659119?

Comment 37

[Matt Brubeck (:mbrubeck)]

The patch fixes a regression that first appeared in Firefox 6, so it's probably not a big help for bug 659119.

Comment 38

[Matt Brubeck (:mbrubeck)]

Pushed to Aurora for Firefox 6:
http://hg.mozilla.org/releases/mozilla-aurora/rev/4f94c4333638