Reuse same ExternalImageId for different video frames if possible

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

31.62 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
Assignee

Description

10 months ago
Created by Bug 1474583 Comment 35.

Current gecko reuse different ExternalImageId for each video frame. It trigger doc.render() for each webrender frame generation. If we reuse same ExternalImageId for different video frames. We could reduce doc.render() calls.
Assignee

Updated

10 months ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Updated

10 months ago
Blocks: 1474583
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 7

10 months ago
Comment on attachment 8994491 [details] [diff] [review]
patch - Reuse same ExternalImageId for different video frames if possible

:jrmuizel, can you review the patch? Since :nical is in PTOs.
Attachment #8994491 - Flags: review?(jmuizelaar)
Assignee

Comment 8

10 months ago
Attachment #8994491 - Attachment is obsolete: true
Attachment #8994491 - Flags: review?(jmuizelaar)
Assignee

Updated

10 months ago
Attachment #8994677 - Flags: review?(jmuizelaar)
Assignee

Updated

10 months ago
Attachment #8994677 - Flags: review?(jmuizelaar)
Assignee

Comment 10

10 months ago
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ae561888a1e3ef5d4f527fec8f74fc62118703f4

It seems that we need to keep WebRenderTextureHostWrapper alive until related wr::Epoch is completed.
Comment on attachment 8994677 [details] [diff] [review]
patch - Reuse same ExternalImageId for different video frames if possible

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

This seems ok to me but I didn't look that closely. Nical should probably still look at it after it lands.
Attachment #8994677 - Flags: review+
Assignee

Comment 13

10 months ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Comment on attachment 8994677 [details] [diff] [review]
> patch - Reuse same ExternalImageId for different video frames if possible
> 
> Review of attachment 8994677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems ok to me but I didn't look that closely. Nical should probably
> still look at it after it lands.

Thanks for the review! I will ask :nical for post review.
Comment hidden (obsolete)
Assignee

Comment 15

10 months ago
Fixed HoldExternalImage() calls.
Attachment #8994729 - Attachment is obsolete: true
Attachment #8994744 - Flags: review+
Assignee

Comment 17

10 months ago
Addressed debug build problems.
Attachment #8994744 - Attachment is obsolete: true
Assignee

Updated

10 months ago
Attachment #8994804 - Flags: review+

Comment 19

10 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb705198ab3
Reuse same ExternalImageId for different video frames if possible r=jrmuizel

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bb705198ab3
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee

Comment 21

10 months ago
Comment on attachment 8994804 [details] [diff] [review]
patch - Reuse same ExternalImageId for different video frames if possible

:nical, can you do post-review the patch?
Attachment #8994804 - Flags: review?(nical.bugzilla)
Depends on: 1478570
Comment on attachment 8994804 [details] [diff] [review]
patch - Reuse same ExternalImageId for different video frames if possible

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

::: gfx/webrender_bindings/RenderTextureHostWrapper.h
@@ +12,5 @@
> +namespace mozilla {
> +
> +namespace wr {
> +
> +class RenderTextureHostWrapper final : public RenderTextureHost

It'd be nice to have a comment here explaining what the wrapper is for.
Attachment #8994804 - Flags: review?(nical.bugzilla) → review+
Assignee

Comment 23

10 months ago
Thanks. I am going to add a comment.
(In reply to Pulsebot from comment #19)
> Pushed by sikeda@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb705198ab3
> Reuse same ExternalImageId for different video frames if possible r=jrmuizel

Could this patch have caused bug 1479175? All crashes contain WR+.
Assignee

Updated

8 months ago
Blocks: 1467768
You need to log in before you can comment on or make changes to this bug.