Closed Bug 1367689 Opened 7 years ago Closed 7 years ago

Make Wrench work with gecko

Categories

(Core :: Graphics: WebRender, enhancement)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jerry, Assigned: vliu)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 8 obsolete files)

0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch
9.85 KB, patch
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1366502 +++

From:
https://bugzilla.mozilla.org/show_bug.cgi?id=1366502#c2

The Wrench doesn't handle the external image well(especially for the platform specific), so I would just convert these special images to the regular rgba buffer in gecko. Then, we could use the well implemented add_image() api in Wrench to record the draw command with gecko.

What I will do here are:
1) use a preference in WebrenderImageLayer to force to put a RGBA image display item.
2) use GetAsSurface() to get the regular rgba buffer from a textureHost and use add_image() for that rgba buffer.
Whiteboard: gfx-noted
Take this bug for the following.
Assignee: nobody → vliu
Hi Jerry,

The attached patch intends to add a preference to make Wrench work with gecko. Could you please have a review to see it works as your expectation? Really thanks.
Attachment #8871628 - Flags: review?(hshih)
Hi Jerry,

The attached patch intends to add a preference to make Wrench work with gecko. Could you please have a review to see it works as your expectation? Really thanks.
Attachment #8871628 - Attachment is obsolete: true
Attachment #8871628 - Flags: review?(hshih)
Attachment #8876606 - Flags: review?(hshih)
It might be nice to make the static ENABLE_RECORDING: bool = false; an environment variable as well.
Comment on attachment 8876606 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

> It might be nice to make the static ENABLE_RECORDING: bool = false; an environment variable as well.

Vincent, could you also create an environment variable for this setting?
Attachment #8876606 - Flags: review?(hshih) → review+
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #5)

> > It might be nice to make the static ENABLE_RECORDING: bool = false; an environment variable as well.
> 
> Vincent, could you also create an environment variable for this setting?

Hi Jerry,

Could you please have review for the patch? Thanks.
Attachment #8879435 - Flags: review?(hshih)
Sorry to reattach HG format of patch.
Attachment #8879435 - Attachment is obsolete: true
Attachment #8879435 - Flags: review?(hshih)
Attachment #8879436 - Flags: review?(hshih)
Comment on attachment 8879436 [details] [diff] [review]
0001-Bug-1367689-Make-ENABLE_RECORDING-as-environment-var.patch

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

Please share the same gfxVars name for both skipping ext-image and using recording.

::: gfx/webrender_bindings/src/bindings.rs
@@ +875,5 @@
>                                  out_renderer: &mut *mut WrRenderer)
>                                  -> bool {
>      assert!(unsafe { is_in_render_thread() });
>  
> +    let recorder: Option<Box<ApiRecordingReceiver>> = if unsafe { enable_recording() }{

enable_recording() }{

Put a space between the last parentheses.
Attachment #8879436 - Flags: review?(hshih)
Hi Jerry,

Thanks for the suggestion. Could you please review the patch?
Attachment #8879436 - Attachment is obsolete: true
Attachment #8880754 - Flags: review?(hshih)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #8)
> Comment on attachment 8879436 [details] [diff] [review]
> 0001-Bug-1367689-Make-ENABLE_RECORDING-as-environment-var.patch
> 
> Review of attachment 8879436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please share the same gfxVars name for both skipping ext-image and using
> recording.
> 

The attached patch also merge the previous r+ed patch into one. Could you please have a review? Thanks
Attachment #8876606 - Attachment is obsolete: true
Attachment #8880754 - Attachment is obsolete: true
Attachment #8880754 - Flags: review?(hshih)
Attachment #8880814 - Flags: review?(hshih)
Comment on attachment 8880814 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

The patch comment is not clear.

Please say something like:

Title: Add a preference to turn on wrench in gecko.
Comment: The Wrench doesn't handle the ext-image well. This patch also add a preference to skip the usage of ext-image and use the implemented add_image() for replacement.
Attachment #8880814 - Flags: review?(hshih) → review+
Comment on attachment 8880814 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +733,5 @@
> +    // Outputs a wr-record-*.bin file for each window that is shown
> +    // Note: wrench will panic if external images are used, they can
> +    // be disabled in WebRenderBridgeParent::ProcessWebRenderCommands
> +    // by commenting out the path that adds an external image ID
> +    fn gfx_use_Wrench() -> bool;

Can we keep 'Wrench' lowercase?
Comment on attachment 8880814 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

::: gfx/config/gfxVars.h
@@ +35,5 @@
>    _(PDMWMFDisableD3D9Dlls,      nsCString,        nsCString())          \
>    _(DXInterop2Blocked,          bool,             false)                \
>    _(UseWebRender,               bool,             false)                \
>    _(UseWebRenderANGLE,          bool,             false)                \
> +  _(UseWrench,                  bool,             false)                \

Where does this get set to true?
Comment on attachment 8880814 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

::: gfx/config/gfxVars.h
@@ +35,5 @@
>    _(PDMWMFDisableD3D9Dlls,      nsCString,        nsCString())          \
>    _(DXInterop2Blocked,          bool,             false)                \
>    _(UseWebRender,               bool,             false)                \
>    _(UseWebRenderANGLE,          bool,             false)                \
> +  _(UseWrench,                  bool,             false)                \

EnableWebRenderRecording is also probably a better name.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> Comment on attachment 8880814 [details] [diff] [review]
> 0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch
> 
> Review of attachment 8880814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/config/gfxVars.h
> @@ +35,5 @@
> >    _(PDMWMFDisableD3D9Dlls,      nsCString,        nsCString())          \
> >    _(DXInterop2Blocked,          bool,             false)                \
> >    _(UseWebRender,               bool,             false)                \
> >    _(UseWebRenderANGLE,          bool,             false)                \
> > +  _(UseWrench,                  bool,             false)                \
> 
> Where does this get set to true?

Carried patch in the attached file r+ed from Jerry to wait for review.

Actually I didn't add this part to set. Based on this, I added this part in gfxPlatform::InitWebRenderConfig(). The basic idea is using a user preference named gfx.webrender.recording.enabled to set enable/disable. Can you please have a review? Thanks.
Attachment #8880814 - Attachment is obsolete: true
Attachment #8882354 - Flags: review?(jmuizelaar)
I think an environment variable makes more sense for this kind of thing. It's sort of a hassle to change the pref, restart and then change the pref back.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> I think an environment variable makes more sense for this kind of thing.
> It's sort of a hassle to change the pref, restart and then change the pref
> back.

Thanks for the good suggestion to use environment variable. Could you please have a review?
Attachment #8882354 - Attachment is obsolete: true
Attachment #8882354 - Flags: review?(jmuizelaar)
Attachment #8882657 - Flags: review?(jmuizelaar)
Attachment #8882657 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8882657 [details] [diff] [review]
0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch

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

::: gfx/thebes/gfxEnv.h
@@ +107,3 @@
>    // WARNING:
>    // Please make sure that you've added your new envvar to the list above in
>    // alphabetical order. Please do not just append it to the end of the list.

See this comment ^
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8882657 [details] [diff] [review]
> 0001-Bug-1367689-Add-a-preference-to-make-Wrench-work-wit.patch
> 
> Review of attachment 8882657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxEnv.h
> @@ +107,3 @@
> >    // WARNING:
> >    // Please make sure that you've added your new envvar to the list above in
> >    // alphabetical order. Please do not just append it to the end of the list.
> 
> See this comment ^

Thanks for your remind. I will move to correct position.
Reattached r+ed patch, plus modifying the patch based on the above suggestion.
Attachment #8882657 - Attachment is obsolete: true
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee42d106b3
Add a preference to make Wrench work with Gecko. r=jrmuize, jerry
https://hg.mozilla.org/mozilla-central/rev/2fee42d106b3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Is ENABLE_WR_RENDERING a typo? I think ENABLE_WR_RECORDING makes more sense to me.
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #24)
> Is ENABLE_WR_RENDERING a typo? I think ENABLE_WR_RECORDING makes more sense
> to me.

Thanks for remind of me for the typo. Bug 1390401 was filed to fix it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: