HTML5 HW accelerated video is slow

RESOLVED FIXED in Firefox 6

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

({mobile, perf, regression})

Trunk
mozilla6
ARM
Maemo
mobile, perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6 fixed, fennec6+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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
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.
(Assignee)

Comment 2

6 years ago
Created attachment 536003 [details] [diff] [review]
Cache temportary surface

We can cache temporary container surface
(Assignee)

Comment 3

6 years ago
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
Attachment #536003 - Attachment is obsolete: true
Attachment #536138 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Initial regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94042ef440fa&tochange=15bfb9729ff3
(Assignee)

Comment 6

6 years ago
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
Attachment #536138 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

6 years ago
Ok, after reversion of http://hg.mozilla.org/mozilla-central/rev/ed0850a1bbb7 video start working fast without intermediate surface
Blocks: 654950
How does that patch affect whether an intermediate surface is used? I don't see how it can.
Oh, this?

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

OK.
Why does the child have a cliprect?
We could revert just that change, but someone would have to also fix the scissor-rect calculation code, which was completely wrong before.
(Assignee)

Comment 12

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

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

Comment 13

6 years ago
reverting only that one change breaking layer rendering... I see only bottom part of image layer rendered on youtube.
Right. See comment #11.
(Assignee)

Comment 15

6 years ago
Is it possible to make some test with certain layout which is checking this intermediate surface issue
(Assignee)

Comment 16

6 years ago
Created attachment 539600 [details] [diff] [review]
Backout patch
Assignee: nobody → romaxa
Attachment #536138 - Attachment is obsolete: true
Attachment #539600 - Flags: review?(roc)
(Assignee)

Comment 17

6 years ago
Created attachment 539668 [details] [diff] [review]
Backout 654950, removed missing test
Attachment #539600 - Attachment is obsolete: true
Attachment #539600 - Flags: review?(roc)
Attachment #539668 - Flags: review?(roc)
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.
Attachment #539668 - Flags: review?(roc) → review-
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.
(Assignee)

Comment 20

6 years ago
Created attachment 539702 [details] [diff] [review]
use matr.transform + round in case of non-integer translation
Attachment #539668 - Attachment is obsolete: true
Attachment #539702 - Flags: review?(roc)
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.
(Assignee)

Comment 22

6 years ago
Created attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile
Attachment #539702 - Attachment is obsolete: true
Attachment #539702 - Flags: review?(roc)
Attachment #539719 - Flags: review?(roc)
Comment on attachment 539719 [details] [diff] [review]
Round NonIntegerTranslation on mobile

Review of attachment 539719 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #539719 - Flags: review?(roc) → review+
(Assignee)

Comment 24

6 years ago
Try server looks good, please push it
http://tbpl.mozilla.org/?tree=Try&rev=85da463bad9c
Keywords: checkin-needed, perf
http://hg.mozilla.org/mozilla-central/rev/5a46d17c72ca
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
This regression is present in Firefox 6.  Is this fix suitable for landing on the Aurora channel?  If so, please request mozilla-approval-aurora.
tracking-fennec: --- → ?
status-firefox6: --- → affected
Keywords: mobile
(Assignee)

Comment 27

6 years ago
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
Attachment #539719 - Flags: approval-mozilla-aurora?
If it affects Android, it could explain bug 661606.
(Assignee)

Comment 29

6 years ago
Ah, ok I see, probably make sense and this could be a reason
Let's give this sime time to bake on trunk before moving to aurora. Leaving nom'ed for the next triage.

Comment 31

6 years ago
May be causing bug 659119?

Comment 32

6 years ago
Can we get a risk analysis here? It's been baking for a week now.
Pretty much no risk on desktop, more on mobile.

Comment 34

6 years ago
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.
Attachment #539719 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to comment #31)
> May be causing bug 659119?

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

Comment 36

6 years ago
(In reply to comment #35)
Sorry, I meant the bug, not the patch.  As in the patch might also address bug 659119?
The patch fixes a regression that first appeared in Firefox 6, so it's probably not a big help for bug 659119.
Pushed to Aurora for Firefox 6:
http://hg.mozilla.org/releases/mozilla-aurora/rev/4f94c4333638
status-firefox6: affected → fixed
Target Milestone: mozilla7 → mozilla6
Keywords: regression
tracking-fennec: ? → 6+
You need to log in before you can comment on or make changes to this bug.