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
Assignee

Description

2 years ago
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

Updated

2 years ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Updated

2 years ago
Blocks: 1341235
Assignee

Updated

2 years ago
Depends on: 1347811
Assignee

Comment 1

2 years ago
Posted patch wip patch (obsolete) — Splinter Review
Very early implementation.
Assignee

Comment 2

2 years ago
Attachment #8849392 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1353627
Assignee

Comment 3

2 years ago
Attachment #8851516 - Attachment is obsolete: true
Assignee

Comment 4

2 years ago
Attachment #8854752 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1355317
Assignee

Comment 5

2 years ago
Rebased.
Attachment #8856784 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1355401
Assignee

Comment 6

2 years ago
rebased.
Attachment #8856877 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1356944
Assignee

Updated

2 years ago
Depends on: 1356954
Assignee

Updated

2 years ago
No longer depends on: 1356954
Assignee

Updated

2 years ago
Depends on: 1357541
Assignee

Updated

2 years ago
Depends on: 1358014
Assignee

Comment 7

2 years ago
Rebased.
Attachment #8857290 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1359206
Assignee

Comment 8

2 years ago
Attachment #8860361 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Blocks: 1359993
Assignee

Comment 9

2 years ago
Attachment #8862143 - Attachment is obsolete: true
Assignee

Comment 10

2 years ago
Attachment #8862625 - Attachment is obsolete: true
Assignee

Comment 11

2 years ago
Fixed crashes. Need to reduce key generation.
Attachment #8862924 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1360701
Assignee

Updated

2 years ago
Depends on: 1360717
Assignee

Comment 12

2 years ago
Rebased.
Attachment #8863034 - Attachment is obsolete: true
Assignee

Comment 13

2 years ago
Attachment #8868914 - Attachment is obsolete: true
Assignee

Comment 14

2 years ago
Attachment #8868915 - Attachment is obsolete: true
Assignee

Comment 15

2 years ago
Attachment #8869359 - Attachment is obsolete: true
Assignee

Comment 16

2 years ago
Attachment #8869364 - Attachment is obsolete: true
Assignee

Comment 18

2 years ago
Fixed problems when e10s was disabled.
Attachment #8869398 - Attachment is obsolete: true
Assignee

Comment 19

2 years ago
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.
Assignee

Updated

2 years ago
Depends on: 1366915
Assignee

Comment 20

2 years ago
Attachment #8869905 - Attachment is obsolete: true
Assignee

Comment 22

2 years ago
Attachment #8870278 - Attachment is obsolete: true
Assignee

Comment 23

2 years ago
Attachment #8871689 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1368483
Assignee

Comment 25

2 years ago
Rebased.
Attachment #8872329 - Attachment is obsolete: true
Assignee

Comment 26

2 years ago
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?
Assignee

Updated

2 years ago
Attachment #8872509 - Flags: sec-approval?
Assignee

Comment 27

2 years ago
Please ignore Comment 26.
Assignee

Updated

2 years ago
Attachment #8872509 - Attachment description: patch - Allocate pipleline for each video → patch part 1 - Allocate pipleline for each video
Assignee

Updated

2 years ago
Attachment #8872509 - Flags: review?(nical.bugzilla)
Assignee

Updated

2 years ago
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!
Assignee

Comment 33

2 years ago
Thanks!
Assignee

Updated

2 years ago
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+
Assignee

Comment 35

2 years ago
Rebased.
Attachment #8872509 - Attachment is obsolete: true
Attachment #8873690 - Flags: review+
Assignee

Comment 36

2 years ago
Rebased.
Attachment #8872589 - Attachment is obsolete: true
Attachment #8873693 - Flags: review+

Comment 38

2 years ago
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

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c57e39a677a
https://hg.mozilla.org/mozilla-central/rev/5f944d9ffbe0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.