Last Comment Bug 660565 - HTML5 HW accelerated video is slow
: HTML5 HW accelerated video is slow
Status: RESOLVED FIXED
: mobile, perf, regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Maemo
: -- normal (vote)
: mozilla6
Assigned To: Oleg Romashin (:romaxa)
:
:
Mentors:
Depends on:
Blocks: opengl-mobile 654950
  Show dependency treegraph
 
Reported: 2011-05-29 18:57 PDT by Oleg Romashin (:romaxa)
Modified: 2013-12-27 14:33 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
6+


Attachments
Cache temportary surface (7.67 KB, patch)
2011-05-29 21:44 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Cache offscreen FBO, for OGL container, (7.70 KB, patch)
2011-05-30 11:57 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Backout patch (14.31 KB, patch)
2011-06-15 11:34 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Backout 654950, removed missing test (14.28 KB, patch)
2011-06-15 15:05 PDT, Oleg Romashin (:romaxa)
roc: review-
Details | Diff | Splinter Review
use matr.transform + round in case of non-integer translation (2.13 KB, patch)
2011-06-15 18:39 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Round NonIntegerTranslation on mobile (2.64 KB, patch)
2011-06-15 22:12 PDT, Oleg Romashin (:romaxa)
roc: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-05-29 18:57:20 PDT
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) 2011-05-29 20:41:10 PDT
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) 2011-05-29 21:44:53 PDT
Created attachment 536003 [details] [diff] [review]
Cache temportary surface

We can cache temporary container surface
Comment 3 Oleg Romashin (:romaxa) 2011-05-30 11:57:25 PDT
Created attachment 536138 [details] [diff] [review]
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) 2011-06-13 22:56:44 PDT
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) 2011-06-14 01:01:54 PDT
Initial regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94042ef440fa&tochange=15bfb9729ff3
Comment 6 Oleg Romashin (:romaxa) 2011-06-14 19:37:25 PDT
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) 2011-06-14 19:59:41 PDT
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) 2011-06-14 21:14:11 PDT
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) 2011-06-14 21:15:08 PDT
Oh, this?

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

OK.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-14 21:16:04 PDT
Why does the child have a cliprect?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-14 21:18:39 PDT
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) 2011-06-14 22:39:11 PDT
> -        !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) 2011-06-14 23:47:04 PDT
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) 2011-06-15 03:03:11 PDT
Right. See comment #11.
Comment 15 Oleg Romashin (:romaxa) 2011-06-15 09:29:47 PDT
Is it possible to make some test with certain layout which is checking this intermediate surface issue
Comment 16 Oleg Romashin (:romaxa) 2011-06-15 11:34:05 PDT
Created attachment 539600 [details] [diff] [review]
Backout patch
Comment 17 Oleg Romashin (:romaxa) 2011-06-15 15:05:16 PDT
Created attachment 539668 [details] [diff] [review]
Backout 654950, removed missing test
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 16:17:31 PDT
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) 2011-06-15 16:21:11 PDT
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) 2011-06-15 18:39:44 PDT
Created attachment 539702 [details] [diff] [review]
use matr.transform + round in case of non-integer translation
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 19:55:15 PDT
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) 2011-06-15 22:12:19 PDT
Created attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 22:27:15 PDT
Comment on attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile

Review of attachment 539719 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 24 Oleg Romashin (:romaxa) 2011-06-16 02:17:19 PDT
Try server looks good, please push it
http://tbpl.mozilla.org/?tree=Try&rev=85da463bad9c
Comment 25 Dão Gottwald [:dao] 2011-06-16 03:56:26 PDT
http://hg.mozilla.org/mozilla-central/rev/5a46d17c72ca
Comment 26 Matt Brubeck (:mbrubeck) 2011-06-16 09:20:19 PDT
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) 2011-06-16 09:54:43 PDT
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) 2011-06-16 09:55:37 PDT
If it affects Android, it could explain bug 661606.
Comment 29 Oleg Romashin (:romaxa) 2011-06-16 10:14:10 PDT
Ah, ok I see, probably make sense and this could be a reason
Comment 30 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-16 14:53:43 PDT
Let's give this sime time to bake on trunk before moving to aurora. Leaving nom'ed for the next triage.
Comment 31 [:Cww] 2011-06-22 12:02:14 PDT
May be causing bug 659119?
Comment 32 Asa Dotzler [:asa] 2011-06-23 14:56:02 PDT
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) 2011-06-23 15:04:26 PDT
Pretty much no risk on desktop, more on mobile.
Comment 34 Asa Dotzler [:asa] 2011-06-23 15:29:01 PDT
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) 2011-06-24 03:14:49 PDT
(In reply to comment #31)
> May be causing bug 659119?

This isn't in FF5 or FF6, so no.
Comment 36 [:Cww] 2011-06-24 10:24:45 PDT
(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) 2011-06-24 10:26:54 PDT
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) 2011-06-27 16:19:36 PDT
Pushed to Aurora for Firefox 6:
http://hg.mozilla.org/releases/mozilla-aurora/rev/4f94c4333638

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