1094442 - Pick tile size based on the runtime/device settings (driver paddings, screen size)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor how tile w/h is queried

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor how tile w/h is queried

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8517718 [details] [diff] [review]
Part 1: Refactor how tile w/h is queried

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

Some comments but looks fine.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ -183,5 @@
>                                         in float aTopMargin,
>                                         in float aRightMargin,
>                                         in float aBottomMargin,
> -                                       in uint32_t aAlignmentX,
> -                                       in uint32_t aAlignmentY,

UUID bump in this file

::: gfx/thebes/gfxPlatform.cpp
@@ +854,5 @@
> +int
> +gfxPlatform::GetTileWidth()
> +{
> +  MOZ_ASSERT(mTileWidth != -1);
> +  return mTileWidth;

Are you sure the CompositorParent is always created early enough to assert mTileWidth != -1 here? I don't know what the startup codepaths look like so I don't know if that's safe or not, but if you know it is then great. :)

::: layout/base/nsLayoutUtils.cpp
@@ +966,3 @@
>    aContent->SetProperty(nsGkAtoms::DisplayPortMargins,
>                          new DisplayPortMarginsPropertyData(
> +                            aMargins, alignmentX, alignmentY, aPriority),

I would even remove the fields from DisplayPortMarginsPropertyData since I don't see a need to keep them. If removing them is hard for some reason I don't see then you can keep it for now.

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Are you sure the CompositorParent is always created early enough to assert
> mTileWidth != -1 here? I don't know what the startup codepaths look like so
> I don't know if that's safe or not, but if you know it is then great. :)

I'm not sure either. That's why I have a lot of asserts to catch these cases. Hopefully testing will uncover problems.

Note that for the b2g child process cases we will need to send an IPC message to communicate the tile sizes.

> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +966,3 @@
> >    aContent->SetProperty(nsGkAtoms::DisplayPortMargins,
> >                          new DisplayPortMarginsPropertyData(
> > +                            aMargins, alignmentX, alignmentY, aPriority),
> 
> I would even remove the fields from DisplayPortMarginsPropertyData since I
> don't see a need to keep them. If removing them is hard for some reason I
> don't see then you can keep it for now.

I'll have a look.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor how tile w/h is queried r=kats

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Part 2: WIP

Using bugzilla to scp files

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Btw, one thing that concerns me is that there might be multiple CompositorParent instances created, so you want to make sure that the code to set the tile size in the constructor only gets run one on startup rather than every time. With the patches you have so far it will just always set the same value so it's not a big deal but if you change that to a heuristic you might end up with numbers that change partway through execution.

Comment 8

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

Comment on attachment 8517752 [details] [diff] [review]
Part 1: Refactor how tile w/h is queried r=kats

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

Drive by...

::: gfx/layers/ipc/CompositorParent.cpp
@@ +388,5 @@
>    }
> +
> +  gfxPlatform::GetPlatform()->SetTileSize(
> +    gfxPrefs::LayersTileWidth(),
> +    gfxPrefs::LayersTileHeight());

I'm assuming this happens "at the right time", and only once?  Even if called multiple times, the pref itself is only queried once on startup, and won't return a different value during the session, but it may be worth a comment.  Actually, may even be worth some debug check in the form of "somebody is calling getPlatform::GetTileSize without having called getPlatform::SetTileSize", which, in our case probably means an error in the code, right?

::: layout/base/nsLayoutUtils.cpp
@@ +861,2 @@
>    // And then align it to the requested alignment
> +  if (alignmentX > 0 || alignmentY > 0) {

Why not && instead of || ?  Do we really want to cover the scenario where tile width is 256 and tile height is -1 (at which point we force it to be 1 down below).

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Comment on attachment 8517752 [details] [diff] [review]
> Part 1: Refactor how tile w/h is queried r=kats
> 
> Review of attachment 8517752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by...
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +388,5 @@
> >    }
> > +
> > +  gfxPlatform::GetPlatform()->SetTileSize(
> > +    gfxPrefs::LayersTileWidth(),
> > +    gfxPrefs::LayersTileHeight());
> 
> I'm assuming this happens "at the right time", and only once?  Even if
> called multiple times, the pref itself is only queried once on startup, and
> won't return a different value during the session, but it may be worth a
> comment.

AFAIK this does happen early enough. Assertions are in place that should blow off if we mess this up however. I forget if we have more than one compositor in the case of multiple window but in any case that number should be very low. Right now I don't allow setting a different tile size so if this is called again and a new number is computed then the assertion should catch it.

> Actually, may even be worth some debug check in the form of
> "somebody is calling getPlatform::GetTileSize without having called
> getPlatform::SetTileSize", which, in our case probably means an error in the
> code, right?

I believe the current assertion will already cover that.
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +861,2 @@
> >    // And then align it to the requested alignment
> > +  if (alignmentX > 0 || alignmentY > 0) {
> 
> Why not && instead of || ?  Do we really want to cover the scenario where
> tile width is 256 and tile height is -1 (at which point we force it to be 1
> down below).

-1 is an invalid value and will be caught be an assertion in GetTileSize(). But yes this if() can be cleaned up.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: PArt 2: Adjust tile size to the stride of the gralloc buffers

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8518561 [details] [diff] [review]
PArt 2: Adjust tile size to the stride of the gralloc buffers

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

Is there a reason you're putting all this code in CompositorParent? It seems to me like this code really belongs in gfxPlatform itself. So rather than initializing mTileWidth and mTileHeight to -1 in gfxPlatform, you should just do all this computation and/or reading of the prefs in the gfxPlatform constructor and just make sure the tile size is good from the start. That will also avoid all this worry about multiple CompositorParent instances and things not being ready early enough. Even better you can put platform-specific things in the gfxAndroidPlatform subclass if you want. Thoughts?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +376,5 @@
> +  gfxPlatform::GetPlatform()->SetTileSize(w, h);
> +}
> +
> +static void
> +SetTileSize()

You seem to have two SetTileSize function implementations here. I assume one is left over accidentally?

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 8518561 [details] [diff] [review]
> PArt 2: Adjust tile size to the stride of the gralloc buffers
> 
> Review of attachment 8518561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a reason you're putting all this code in CompositorParent? It seems
> to me like this code really belongs in gfxPlatform itself. So rather than
> initializing mTileWidth and mTileHeight to -1 in gfxPlatform, you should
> just do all this computation and/or reading of the prefs in the gfxPlatform
> constructor and just make sure the tile size is good from the start. That
> will also avoid all this worry about multiple CompositorParent instances and
> things not being ready early enough. Even better you can put
> platform-specific things in the gfxAndroidPlatform subclass if you want.
> Thoughts?

The problem is that gralloc needs to be allocated in the parent process. I believe sandboxing requires this. I did move the code in gfxPlatform because I think it's better there but it's still setup from CompositorParent.

I could use gfxAndroidPlatform subclass and make this virtual but IMO it's a lot more readable to have a single function with ifdef then looking at the subclasses for all the platform to understand how the heuristics differ per platform. This is really subjective so let me know if you disagree on that.

> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +376,5 @@
> > +  gfxPlatform::GetPlatform()->SetTileSize(w, h);
> > +}
> > +
> > +static void
> > +SetTileSize()
> 
> You seem to have two SetTileSize function implementations here. I assume one
> is left over accidentally?

Opps, bad patch management.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Part 2: Adjust tile size to the stride of the gralloc buffers

Comment 14

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=b73097e0012b

Comment 15

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor how tile w/h is queried r=kats

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8519097 [details] [diff] [review]
Part 2: Adjust tile size to the stride of the gralloc buffers

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

Ok, given the updated part 1 I think this should be ok. Various comments below though, please address them before landing.

::: gfx/layers/ipc/CompositorParent.h
@@ +153,5 @@
>  
> +  virtual bool RecvGetTileSize(int32_t* aWidth, int32_t* aHeight) MOZ_OVERRIDE
> +  {
> +    *aWidth = gfxPlatform::GetPlatform()->GetTileWidth();
> +    *aHeight = gfxPlatform::GetPlatform()->GetTileHeight();

I would prefer if this implementation were moved into the .cpp for consistency (and less touching of .h files in the future if this code changes).

::: gfx/layers/ipc/PCompositor.ipdl
@@ +87,5 @@
>    // Make sure any pending composites are started immediately and
>    // block until they are completed.
>    sync FlushRendering();
>  
> +  // Get the size of the tiles. This number can not change at runtime.

s/can not/should not/

::: gfx/thebes/gfxPlatform.cpp
@@ +884,5 @@
> +void
> +gfxPlatform::ComputeTileSize()
> +{
> +  int32_t w = gfxPrefs::LayersTileWidth();
> +  int32_t h = gfxPrefs::LayersTileHeight();

I would also like a NS_RUNTIMEABORT if XRE_GetProcessType() != GeckoProcessType_Default to guard against this accidentally getting run in the child process. According to https://wiki.mozilla.org/Nested_Content_Processes#Process_Creation this check should be safe even when we have nested content processes.

@@ +899,5 @@
> +                                 android::GraphicBuffer::USAGE_HW_TEXTURE);
> +
> +    if (alloc.get()) {
> +      w = alloc->getStride(); // We want the tiles to be gralloc stride aligned.
> +      h = gfxPrefs::LayersTileHeight();

No need to assign h since it got assigned this value up above already. You can replace this with a comment saying that we're leaving the height as the pref value intentionally.

@@ +904,5 @@
> +    }
> +#endif
> +  }
> +
> +  gfxPlatform::GetPlatform()->SetTileSize(w, h);

You can just call SetTileSize directly since this method is already non-static.

::: gfx/thebes/gfxPlatform.h
@@ +301,5 @@
>      int GetTileHeight();
>      void SetTileSize(int aWidth, int aHeight);
> +    /**
> +     * The compositor should calculate the tile size. That calculated size should
> +     * be set in the child processes to match the compositor tile size.

This comment feels inappropriate. I would prefer something like:

"Calling this function will compute and set the ideal tile size for the platform. This should only be called in the parent process; child processes should be updated via SetTileSize to match the value computed in the parent."

::: modules/libpref/init/all.js
@@ +3884,5 @@
>  pref("layers.low-precision-buffer", false);
>  pref("layers.tile-width", 256);
>  pref("layers.tile-height", 256);
> +// If this is set the tile size will only be treated as a suggestion.
> +// On B2G we will round this to the stride of the unlying allocation.

s/unlying/underlying/

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Part 2: Adjust tile size to the stride of the gralloc buffers

Comment 18

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=3d7feed629f8

Comment 19

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=948a7c56ff53

Comment 20

[Benoit Girard (:BenWa)]

Attachment: Part 1: Refactor how tile w/h is queried r=kats

rebased

Comment 21

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/767ad1362a2d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6cf65da621

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/767ad1362a2d
https://hg.mozilla.org/mozilla-central/rev/4a6cf65da621