840967 - [layers-refactoring] read the pref for layers backend only at startup

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

With the new architecture, the possibility of having several layer backends coexisting in the compositor thread/process makes the implementation of async textures (async-video) rather tricky.

The easiest way to get around this is to make it so that if the layers acceleration pref is changed, it takes effect next time gecko starts (instead of next time a widget is created like today).
It is not a problem to have non-omtc basic layers and some GL on the compositor thread, the issue is specific to having several backends on the compositor thread.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Nicolas, is this something you'd do as a part of async video, or can it be given to somebody else?

Comment 2

[Nicolas Silva [:nical]]

It can be done by somebody else. To get async video to work completely I'll need it done, so if nobody has time for this I'll do it. It's a rather small change, so it's not a big deal if I end up doing it, but I filed it so people can take it if they have nothing to do).

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Which preference is this?

Comment 4

[Nicolas Silva [:nical]]

It's the combination of layers.acceleration.disabled and layers.acceleration.force-enabled that lets us decide what layers backend to use.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Get the layers acceleration prefs only once

Nical, is this what you had in mind?  Is there a better place for the methods or a different implementation you though we would do?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Get the layers acceleration prefs only once

Nicolas, is this what you had in mind?

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 716191 [details] [diff] [review]
Get the layers acceleration prefs only once

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

LGTM

::: gfx/layers/d3d9/LayerManagerD3D9.cpp
@@ +42,5 @@
>  {
>    ScopedGfxFeatureReporter reporter("D3D9 Layers", force);
>  
>    /* XXX: this preference and blacklist code should move out of the layer manager */
> +  bool forceAccelerate = gfxPlatform::LayersAccelerationForceEnabled();

Please remove the XXX comment now that you have fixed it.

::: gfx/thebes/gfxPlatform.cpp
@@ +1732,5 @@
> +   * If LAYERS_ACCELRATION_PREFS_ON_RESTART_ONLY is not defined, layers acceleration
> +   * prefs will only be re-evaluated every time we call LayersAcceleration* methods below.
> +   * We do want to initialize both at the same time, to guarantee the values do not change.
> +   */
> +#define LAYERS_ACCELERATION_PREFS_ON_RESTART_ONLY 1

I don't think we need this, we should only support changing layers acceleration on restart (that is the current situation)

::: widget/windows/nsWindow.cpp
@@ +3218,5 @@
> +  aManagerPrefs->mDisableAcceleration = gfxPlatform::LayersAccelerationDisabled();
> +  aManagerPrefs->mForceAcceleration = gfxPlatform::LayersAccelerationForceEnabled();
> +
> +  // This is a bit dangerous; if these prefs do not exist, the variables will stay
> +  // unmodified.

Why is it dangerous? Should we supply defaults to GetBool?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Nick Cameron [:nrc] from comment #7)
>>    /* XXX: this preference and blacklist code should move out of the layer manager */
> +  bool forceAccelerate = gfxPlatform::LayersAccelerationForceEnabled();
> Please remove the XXX comment now that you have fixed it.

Good point.  I wasn't sure I was in fact fixing the whole thing - wasn't sure if it was just talking about the Preferences::GetBool call or the rest of the code that uses that value.  I'll check with Jeff M., he added that comment.


>> +#define LAYERS_ACCELERATION_PREFS_ON_RESTART_ONLY 1

>I don't think we need this, we should only support changing layers acceleration on >restart (that is the current situation)

We do it all the time now, or just on restart?  I thought Nicolas added the bug because we were doing it all the time?  I added this mostly because of thinking we could land this change to central, with it undefined, then have a small merge with it defined, then remove the define.  But that may not be worth the effort.  What do you think?

> Why is it dangerous? Should we supply defaults to GetBool?

Chances are we won't run into a problem, but a number of assumptions need to hold in order for things to work well.  For example, if LayerManagerPrefs passed to GetLayerManagerPref() is being reused, and the value went from being set to not being there at all, the method will return with it still being set.  We also assume that the LayerManagerPrefs constructor sets the default values correctly, so that the first time we call GetLayerManagerPref(), we get the right result in case the prefs are missing.

So, likely just a nit, so I thought I'd ask in a review if it makes sense to change it or just let it be.  Thoughts?

Jeff, the question for you about the XXX - should it be removed now?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> (In reply to Nick Cameron [:nrc] from comment #7)
> >>    /* XXX: this preference and blacklist code should move out of the layer manager */
> > +  bool forceAccelerate = gfxPlatform::LayersAccelerationForceEnabled();
> > Please remove the XXX comment now that you have fixed it.
> 
> Good point.  I wasn't sure I was in fact fixing the whole thing - wasn't
> sure if it was just talking about the Preferences::GetBool call or the rest
> of the code that uses that value.  I'll check with Jeff M., he added that
> comment.
> 
> 
> >> +#define LAYERS_ACCELERATION_PREFS_ON_RESTART_ONLY 1
> 
> >I don't think we need this, we should only support changing layers acceleration on >restart (that is the current situation)
> 
> We do it all the time now, or just on restart?  I thought Nicolas added the
> bug because we were doing it all the time?  I added this mostly because of
> thinking we could land this change to central, with it undefined, then have
> a small merge with it defined, then remove the define.  But that may not be
> worth the effort.  What do you think?
> 
> > Why is it dangerous? Should we supply defaults to GetBool?
> 
> Chances are we won't run into a problem, but a number of assumptions need to
> hold in order for things to work well.  For example, if LayerManagerPrefs
> passed to GetLayerManagerPref() is being reused, and the value went from
> being set to not being there at all, the method will return with it still
> being set.  We also assume that the LayerManagerPrefs constructor sets the
> default values correctly, so that the first time we call
> GetLayerManagerPref(), we get the right result in case the prefs are missing.
> 
> So, likely just a nit, so I thought I'd ask in a review if it makes sense to
> change it or just let it be.  Thoughts?
> 
> Jeff, the question for you about the XXX - should it be removed now?

The XXX should stay. From what I remember, it means that the layer manager should not be responsible for getting the information about what it should do itself. Instead the caller should be taking care of that.

Comment 10

[Nick Cameron [:nrc]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> >> +#define LAYERS_ACCELERATION_PREFS_ON_RESTART_ONLY 1
> 
> >I don't think we need this, we should only support changing layers acceleration on >restart (that is the current situation)
> 
> We do it all the time now, or just on restart?  I thought Nicolas added the
> bug because we were doing it all the time?  I added this mostly because of
> thinking we could land this change to central, with it undefined, then have
> a small merge with it defined, then remove the define.  But that may not be
> worth the effort.  What do you think?
>

We currently need a restart and I think we will always need a restart. So I would land without the define.
 
> > Why is it dangerous? Should we supply defaults to GetBool?
> 
> Chances are we won't run into a problem, but a number of assumptions need to
> hold in order for things to work well.  For example, if LayerManagerPrefs
> passed to GetLayerManagerPref() is being reused, and the value went from
> being set to not being there at all, the method will return with it still
> being set.  We also assume that the LayerManagerPrefs constructor sets the
> default values correctly, so that the first time we call
> GetLayerManagerPref(), we get the right result in case the prefs are missing.
> 
> So, likely just a nit, so I thought I'd ask in a review if it makes sense to
> change it or just let it be.  Thoughts?
> 

Sounds like it is worth addressing. I think those GetBool methods can take a default argument, so we should provide one to make sure the one in LayerManagerPrefs gets overwritten. Or maybe there is an alternative to GetBool that always writes something out. (I think they are one and the same).

Comment 11

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #10)
> (In reply to Milan Sreckovic [:milan] from comment #8)
> > >> +#define LAYERS_ACCELERATION_PREFS_ON_RESTART_ONLY 1
> > 
> > >I don't think we need this, we should only support changing layers acceleration on >restart (that is the current situation)
> > 
> > We do it all the time now, or just on restart?  I thought Nicolas added the
> > bug because we were doing it all the time?  I added this mostly because of
> > thinking we could land this change to central, with it undefined, then have
> > a small merge with it defined, then remove the define.  But that may not be
> > worth the effort.  What do you think?
> >
> 
> We currently need a restart and I think we will always need a restart. So I
> would land without the define.

We check the pref each time we create a window, which is in most case when we start, So if you open multiple windows it is not only on restart.

Let's just do without the defines, there isn't much value in supporting reading the pref each time we create a window. We can safely land a patch like this on central without playing around with defines.

>  
> > > Why is it dangerous? Should we supply defaults to GetBool?
> > 
> > Chances are we won't run into a problem, but a number of assumptions need to
> > hold in order for things to work well.  For example, if LayerManagerPrefs
> > passed to GetLayerManagerPref() is being reused, and the value went from
> > being set to not being there at all, the method will return with it still
> > being set.  We also assume that the LayerManagerPrefs constructor sets the
> > default values correctly, so that the first time we call
> > GetLayerManagerPref(), we get the right result in case the prefs are missing.
> > 
> > So, likely just a nit, so I thought I'd ask in a review if it makes sense to
> > change it or just let it be.  Thoughts?
> > 
> 
> Sounds like it is worth addressing. I think those GetBool methods can take a
> default argument, so we should provide one to make sure the one in
> LayerManagerPrefs gets overwritten. Or maybe there is an alternative to
> GetBool that always writes something out. (I think they are one and the
> same).

We should use this one:

static bool GetBool(const char* aPref, bool aDefault = false)

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Get the layers acceleration prefs only once per app lifetime, not once per window creation

Addressed the review comments.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 716623 [details] [diff] [review]
Get the layers acceleration prefs only once per app lifetime, not once per window creation

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

Sorry, there are a few things I had not though about this morning:

::: gfx/thebes/gfxPlatform.cpp
@@ +1724,5 @@
>  }
> +
> +static bool sLayersAccelerationForceEnabled = false;
> +static bool sLayersAccelerationDisabled = false;
> +void LayersAccelerationInitialize()

InitLayersAccelerationPref would be clearer (this name may be understood as in "initialize the accelerated backend") or LayersAccelerationPrefInitialize, or FetchLayersAccelerationPref. Well, you get the idea.

@@ +1754,5 @@
> + * We may need to only check this value once, and not change it, as
> + * layers refactoring and async video (see 840967) gets complicated
> + * if we allow different values during the lifetime of the app.
> + */
> +bool gfxPlatform::LayersAccelerationForceEnabled()

I am uncomfortable with the name here as well, because I read this as "Force the Acceleration" because of the verb. Maybe something like GetForceLayersAcceleration or IsLayerAccelerationForced?

::: widget/windows/nsWindow.cpp
@@ +3221,5 @@
>                         &aManagerPrefs->mPreferOpenGL);
>    Preferences::GetBool("layers.prefer-d3d9",
>                         &aManagerPrefs->mPreferD3D9);
>    Preferences::GetBool("layers.offmainthreadcomposition.enabled",
>                         &aManagerPrefs->mOffMainThreadCompositing);

Actually, I'd like these 3 prefs to go into gfxPlatform as well
I think it is a bug that "layers.offmainthreadcomposition.enabled" is checked when widgets are created because at startup we initialize OMTC depending on the pref, so it could cause problems to start Firefox without OMTC, then change the pref, and then create a new window without restarting.
I didn't know about the preferD3D9/OpenGL prefs (they don't show up in about config on Linux), but that's info we also want to not change before restart so it should be read at startup.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for the review. Right now, they just match the preference: layers.acceleration.force-enabled => LayersAccelerationForceEnabled(); I think it's useful to have the method "match" the preference name, and it's probably too late to rename the preference.  However, I'll come up with something that is less confusing, but still retains this kind of a mapping (e.g., add Get or Is or something like that).  I'll add the other ones to the "startup" values.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Are the prefs somehow tagged as "need to restart for this to take effect" so that the users know it when they change it?  If so, where?

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Get the layers acceleration prefs only once per app lifetime, not once per window creation

Evaluate the other three preferences once per lifetime, change the method names to better note that this is getting the preference.

Comment 17

[Nick Cameron [:nrc]]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Are the prefs somehow tagged as "need to restart for this to take effect" so
> that the users know it when they change it?  If so, where?

afaik, no not in anyway. And anyone who is fiddling with about:config should have that kind of knowledge in advance, I think. about:config is deliberately user-unfriendly.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't have the permission to land, could somebody do it?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to inbound. Please rebase.

Comment 20

[Nicolas Silva [:nical]]

Indeed, someone did part of what you did at the same time (fetch layers.offmainthreadcomposition.enabled only in gfxPlatform)

You can just rebase and not care about this one pref anymore.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - I just meant to land this on the graphics branch, but I don't have the permission to do that.  Let's start with that, and then I'll put together a patch for central/inbound.  Nical, can you land it on the graphics branch?

Comment 22

[Nicolas Silva [:nical]]

This patch is safe to land on inbound/central and we merge central to gfx rather often, so I think it's simpler if we just land on inbound/central and wait for the merge to get it into graphics (waiting a few more days is okay and this way we avoid the merge conflict of applying on both branches and then merging).
I still have a few items on my TODO before I can use what you added in this patch, so if we haven't been able to merge by the time I get them done (because of, say, streaming WebGL buffers) I'll land on gfx.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

OK, but the additional preference is in the graphics branch alone, Joe added it a month ago.  Are you saying that this additional preference should not exist in central or graphics branch, and the next merge from central is going to remove it from the graphics branch?

Comment 24

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #23)
> OK, but the additional preference is in the graphics branch alone, Joe added
> it a month ago.  Are you saying that this additional preference should not
> exist in central or graphics branch, and the next merge from central is
> going to remove it from the graphics branch?

Oh ok, then the merge will complain anyway, i'll just land it in the gfx branch and we'll fix the merge when the time comes.

So this patch should not land to mozilla-central. It does not fix a bug (as in "bad program behvor"), so it is okay to wait for it to land along with the refactoring in mozilla-central.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Right.  What you said :-)

Comment 26

[Nicolas Silva [:nical]]

landed in gfx branch