Closed Bug 1477608 Opened 2 years ago Closed 2 years ago

Reuse same ExternalImageId for different video frames if possible

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

31.62 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
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: nobody → sotaro.ikeda.g
Blocks: 1474583
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)
Attachment #8994491 - Attachment is obsolete: true
Attachment #8994491 - Flags: review?(jmuizelaar)
Attachment #8994677 - Flags: review?(jmuizelaar)
Attachment #8994677 - Flags: review?(jmuizelaar)
(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+
(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.
Fixed HoldExternalImage() calls.
Attachment #8994729 - Attachment is obsolete: true
Attachment #8994744 - Flags: review+
Addressed debug build problems.
Attachment #8994744 - Attachment is obsolete: true
Attachment #8994804 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/7bb705198ab3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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+
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+.
Blocks: 1467768
You need to log in before you can comment on or make changes to this bug.