Investigate to allocate pipleline for each video

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 24 obsolete attachments)

60.56 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
10.44 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
To support video correctly, we need to choose either. This bug is for investigating [1]
-[1] Allocate pipleline for each video
-[2] Update display list for each video frame size/format change
-[3] Add ImageContainer like display list item to webrender.
Assignee: nobody → sotaro.ikeda.g
Blocks: 1341235
Depends on: 1347811
Posted patch wip patch (obsolete) — Splinter Review
Very early implementation.
Attachment #8849392 - Attachment is obsolete: true
Depends on: 1353627
Attachment #8851516 - Attachment is obsolete: true
Attachment #8854752 - Attachment is obsolete: true
Depends on: 1355317
Rebased.
Attachment #8856784 - Attachment is obsolete: true
Depends on: 1355401
rebased.
Attachment #8856877 - Attachment is obsolete: true
Depends on: 1356944
Depends on: 1356954
No longer depends on: 1356954
Depends on: 1357541
Depends on: 1358014
Rebased.
Attachment #8857290 - Attachment is obsolete: true
Depends on: 1359206
Attachment #8860361 - Attachment is obsolete: true
Blocks: 1359993
Attachment #8862143 - Attachment is obsolete: true
Attachment #8862625 - Attachment is obsolete: true
Fixed crashes. Need to reduce key generation.
Attachment #8862924 - Attachment is obsolete: true
Depends on: 1360701
Depends on: 1360717
Rebased.
Attachment #8863034 - Attachment is obsolete: true
Attachment #8868914 - Attachment is obsolete: true
Attachment #8868915 - Attachment is obsolete: true
Attachment #8869359 - Attachment is obsolete: true
Attachment #8869364 - Attachment is obsolete: true
Fixed problems when e10s was disabled.
Attachment #8869398 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccdba27d4f6d3bd0cd91003cfbad4c724a4f31e7

Some test failures were caused by async pipeline id allocation.
When pipeline id allocation was changed to sync, some problems were addressed.
Depends on: 1366915
Attachment #8869905 - Attachment is obsolete: true
Attachment #8870278 - Attachment is obsolete: true
Attachment #8871689 - Attachment is obsolete: true
Depends on: 1368483
Rebased.
Attachment #8872329 - Attachment is obsolete: true
Comment on attachment 8872509 [details] [diff] [review]
patch part 1 - Allocate pipleline for each video

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

> It is not easy to exploit based on the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

> No.

Which older supported branches are affected by this flaw?

> The bug affects since Firefox 32.

If not all supported branches, which bug introduced the flaw?

> Bug 1006957.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

> No, but it is easy to create a patch and it is not risky.

How likely is this patch to cause regressions; how much testing does it need?

> It is not likely to cause a regression. I tested locally and on tryserver, it seems enough.
Attachment #8872509 - Flags: sec-approval?
Attachment #8872509 - Flags: sec-approval?
Please ignore Comment 26.
Attachment #8872509 - Attachment description: patch - Allocate pipleline for each video → patch part 1 - Allocate pipleline for each video
Attachment #8872509 - Flags: review?(nical.bugzilla)
Attachment #8872589 - Flags: review?(nical.bugzilla)
Comment on attachment 8872509 [details] [diff] [review]
patch part 1 - Allocate pipleline for each video

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

I started reviewing this and so far so good, it's a lot to chew so it's going to take a bit more time but I am on it I promise!
Thanks!
Blocks: 1342413
Attachment #8872509 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8872589 [details] [diff] [review]
patch part 2 - Reduce DisplayList update and ImageKey generation

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

::: gfx/layers/wr/WebRenderCompositableHolder.cpp
@@ +300,5 @@
> +  aKeysToDelete.AppendElements(aHolder->mKeys);
> +  aHolder->mKeys.Clear();
> +  aHolder->mCurrentTexture = nullptr;
> +
> +  // No txture to render

nit: texture
Attachment #8872589 - Flags: review?(nical.bugzilla) → review+
Rebased.
Attachment #8872509 - Attachment is obsolete: true
Attachment #8873690 - Flags: review+
Rebased.
Attachment #8872589 - Attachment is obsolete: true
Attachment #8873693 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c57e39a677a
part 1 - Allocate pipleline for each video r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f944d9ffbe0
part 2 -Reduce DisplayList update and ImageKey generation r=nical
https://hg.mozilla.org/mozilla-central/rev/7c57e39a677a
https://hg.mozilla.org/mozilla-central/rev/5f944d9ffbe0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.