1178376 - Fade in tiles that had placeholders before when we aren't using low-precision buffers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

If you don't have low-precision buffers on, the checkerboarded region is simply the background color. When the tile arrives, we immediately draw it, which can be somewhat jarring. Other browsers (maybe Chrome? IE?) seem to fade in the new tile, which seems a little nicer.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fade in tiles that were previously checkerboarded

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b420b599103

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8627276 [details] [diff] [review]
Fade in tiles that were previously checkerboarded

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

I think that we should only have a fade in transition for progressive updates. Otherwise it looks fine to me.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in tiles that were previously checkerboarded

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hmm, looks like my attachment comment got messed up (it's only in the patch?):

This doesn't do anything special for progressive updates. I am not sure of the best way to figure that out on the host side. I thought maybe the SurfaceDescriptorTiles would have a valid region for progressive updates, while non-progressive updates would not, but that doesn't seem to be the case.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8631304 [details] [diff] [review]
Optionally fade in tiles that were previously checkerboarded

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

For progressive updates, you can simply add a boolean in the ipdl message. The information is easy to get on the client-side. I don't think we can get away with fade-in transitions of non-progressive updates.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug Bug 1178376 - Put progressive paint status in tile updates

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Allow progressive painting when low-precision tiles are disabled

According to the existing comment, this will allow progressive paint
on layers that have an OMTA transform, but I don't see where it was clearing
the display port in that case anyway. Help?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in tiles that were previously checkerboarded

This now only fades tiles that are from a progressive paint, which means
that in theory at least, you should only see stuff fading in when you scroll.

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8631845 [details] [diff] [review]
Bug Bug 1178376 - Put progressive paint status in tile updates

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

::: gfx/layers/client/TiledContentClient.cpp
@@ +1550,5 @@
>    // make the progressive paint more visible to the user while scrolling.
>    // On B2G uploads are cheaper and we value coherency more, especially outside
>    // the browser, so we always use the entire user-visible area.
>    IntRect coherentUpdateRect(LayerIntRect::ToUntyped(RoundedOut(
> +#if 0 //def MOZ_WIDGET_ANDROID

Let's make this depend on the pref rather than deactivate a piece of code that is supposed to make things better in the default configuration (unless you believe that we should not do this at all, but then you'll wanyt to discuss it with kats.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 8631846 [details] [diff] [review]
Allow progressive painting when low-precision tiles are disabled

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

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ -240,5 @@
>      // failures because the harness won't wait for all the tiles to be drawn.
>      return false;
>    }
>  
> -  if (mPaintData.mCriticalDisplayPort.IsEmpty()) {

I think the implication here is that if the critical display port is empty then we have no notion of critical display port and want to paint everything. As you mention in the comment just removing this isn't right, unless we can support that this comment is no longer up to date. We shouldn't land this patch until that's the case.

There doesn't need to be something clearing the display port, just nothing setting the -critical- display port.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8631847 [details] [diff] [review]
Optionally fade in tiles that were previously checkerboarded

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +41,5 @@
> +float TileHost::FadeIn(float aOpacity, TimeStamp aNow)
> +{
> +  if (!gfxPrefs::LayersTilesFadeInEnabled()) {
> +    return aOpacity;
> +  }

Should you not check here if aNow has not been set?

@@ +43,5 @@
> +  if (!gfxPrefs::LayersTilesFadeInEnabled()) {
> +    return aOpacity;
> +  }
> +
> +  float duration = (float)gfxPrefs::LayersTilesFadeInDuration();

this should be implicit?

::: gfx/layers/composite/TiledContentHost.h
@@ +109,5 @@
>    bool IsPlaceholderTile() const { return mTextureHost == nullptr; }
>  
> +  void StartFadeIn() { mFadeStart = TimeStamp::Now(); }
> +  bool NeedFadeInComposite(TimeStamp aNow);
> +  float FadeIn(float aOpacity, TimeStamp aNow);

This could use a comment. It's not clear how FadeIn maps to a Float and in which range?

Comment 13

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

(In reply to Nicolas Silva [:nical] from comment #10)
> Comment on attachment 8631845 [details] [diff] [review]
> Bug Bug 1178376 - Put progressive paint status in tile updates
> 
> Review of attachment 8631845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +1550,5 @@
> >    // make the progressive paint more visible to the user while scrolling.
> >    // On B2G uploads are cheaper and we value coherency more, especially outside
> >    // the browser, so we always use the entire user-visible area.
> >    IntRect coherentUpdateRect(LayerIntRect::ToUntyped(RoundedOut(
> > +#if 0 //def MOZ_WIDGET_ANDROID
> 
> Let's make this depend on the pref rather than deactivate a piece of code
> that is supposed to make things better in the default configuration (unless
> you believe that we should not do this at all, but then you'll wanyt to
> discuss it with kats.

I have no objection to ripping the MOZ_WIDGET_ANDROID branch out entirely.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benoit Girard (:BenWa) from comment #12)
Thanks, fixed that stuff

Comment 15

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> Created attachment 8631846 [details] [diff] [review]
> Allow progressive painting when low-precision tiles are disabled
> 
> According to the existing comment, this will allow progressive paint
> on layers that have an OMTA transform, but I don't see where it was clearing
> the display port in that case anyway. Help?

The relevant code is at [1]. As BenWa said, the critical display is not set in the paint data if the layer has OMTA (as opposed to explicitly cleared).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp?rev=91d6e262b662&mark=167-178#172

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Allow progressive painting when low-precision tiles are disabled

Comment 17

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

Comment on attachment 8633630 [details] [diff] [review]
Allow progressive painting when low-precision tiles are disabled

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

This looks ok to me, assuming you actually want to go ahead with all this fading-in business given what Chris Lord said on IRC.

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8633630 [details] [diff] [review]
Allow progressive painting when low-precision tiles are disabled

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

Looks ok, but this could become a bit of a pain if we have a case where the compositor doesn't have the right info decide this. The critical display port was a decent indirect way to decide this, now it's even more indirect. But this is just a potential future concern so r+.

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 8633630 [details] [diff] [review]
Allow progressive painting when low-precision tiles are disabled

I clobbered :kats r+ by accident.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in tiles that were previously placeholders r=BenWa,nical,kats

Comment 21

[Nicolas Silva [:nical]]

Comment on attachment 8631845 [details] [diff] [review]
Bug Bug 1178376 - Put progressive paint status in tile updates

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

r- for the comment I had for this patch a few days ago about the ifdef 0 thingy

Comment 22

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #21)
> r- for the comment I had for this patch a few days ago about the ifdef 0
> thingy

but otherwise LGTM

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8631845 [details] [diff] [review]
Bug Bug 1178376 - Put progressive paint status in tile updates

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

Sorry I didn't see you removed the #ifef 0 in the other patch

Comment 24

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

Comment on attachment 8631845 [details] [diff] [review]
Bug Bug 1178376 - Put progressive paint status in tile updates

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

::: gfx/layers/client/TiledContentClient.h
@@ +417,5 @@
>    void PaintThebes(const nsIntRegion& aNewValidRegion,
>                     const nsIntRegion& aPaintRegion,
>                     LayerManager::DrawPaintedLayerCallback aCallback,
> +                   void* aCallbackData,
> +                   bool aIsProgressive=false);

nit: spaces around =

Comment 25

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

Comment on attachment 8633688 [details] [diff] [review]
Optionally fade in tiles that were previously placeholders r=BenWa,nical,kats

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

I'll leave this to Benwa/nical to officially r+. The removal of the ifdef is fine by me, and I have some other comments below.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +44,5 @@
> +    return aOpacity;
> +  }
> +
> +  float duration = gfxPrefs::LayersTilesFadeInDuration();
> +  float elapsed = (aNow - mFadeStart).ToMilliseconds();

If system time changes during the fade you can end up with aNow < mFadeStart. Not sure if you care about that here or in NeedFadeInComposite.

@@ +596,5 @@
>      if (tileDrawRegion.IsEmpty()) {
>        continue;
>      }
>  
> +    TimeStamp now = TimeStamp::Now();

I feel like you should probably be using the composite timestamp computed at [1] instead of pulling TimeStamp::Now. But I'm not sure.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?rev=8ee029e39ce4#1159

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nicolas Silva [:nical] from comment #23)
> Comment on attachment 8631845 [details] [diff] [review]
> Bug Bug 1178376 - Put progressive paint status in tile updates
> 
> Review of attachment 8631845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry I didn't see you removed the #ifef 0 in the other patch

Yeah, I screwed that up. I'll just remove this change from this patch and remove those lines from the second.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Put progressive paint status in tile updates

Unrot

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Allow progressive painting when low-precision tiles are disabled

Unrot

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in new progressively painted tiles

Unrot and adapt to changes Matt made in bug 1180326

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8658253 [details] [diff] [review]
Optionally fade in new progressively painted tiles

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

::: gfx/layers/composite/TextureHost.h
@@ +531,5 @@
>    FenceHandle GetAndResetAcquireFenceHandle();
>  
>    virtual void WaitAcquireFenceHandleSyncComplete() {};
>  
> +  void StartFadeIn(TimeStamp aNow) {

We could just skip the aNow parameter for both of these, and use TimeStamp::Now() in the impls since that's what the callers always pass.

Add a comment mentioning the pref that controls the duration of the fade.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +520,5 @@
>                                 aScreenRegion, aClipRect, aTransform, mFlashCounter);
> +
> +  if (opacity < aOpacity) {
> +    CompositorParent* compositor = CompositorParent::GetCompositor(mCompositor->GetCompositorID());
> +    compositor->Invalidate();

This is a bit sad, since we'll end up compositing the entire window on platforms that otherwise support partial presents.

Can we instead use Compositor::CompositeUntil when we set the fade, and have LayerTreeInvalidation detect the opacity change and invalidate just the tile in question?

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in new progressively painted tiles

This one seems to be a little more correct, as it avoids fading in the first batch of tiles.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> Comment on attachment 8658253 [details] [diff] [review]
> Optionally fade in new progressively painted tiles
> 
> Review of attachment 8658253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/TextureHost.h
> @@ +531,5 @@
> >    FenceHandle GetAndResetAcquireFenceHandle();
> >  
> >    virtual void WaitAcquireFenceHandleSyncComplete() {};
> >  
> > +  void StartFadeIn(TimeStamp aNow) {
> 
> We could just skip the aNow parameter for both of these, and use
> TimeStamp::Now() in the impls since that's what the callers always pass.

Yeah, I can do that. I thought that Compositor::GetCompositionTime() might be better, but we appear to unset that for some reason. Is there a more appropriate timestamp?

> 
> Add a comment mentioning the pref that controls the duration of the fade.

Fixed

> 
> ::: gfx/layers/composite/TiledContentHost.cpp
> @@ +520,5 @@
> >                                 aScreenRegion, aClipRect, aTransform, mFlashCounter);
> > +
> > +  if (opacity < aOpacity) {
> > +    CompositorParent* compositor = CompositorParent::GetCompositor(mCompositor->GetCompositorID());
> > +    compositor->Invalidate();
> 
> This is a bit sad, since we'll end up compositing the entire window on
> platforms that otherwise support partial presents.
> 
> Can we instead use Compositor::CompositeUntil when we set the fade, and have
> LayerTreeInvalidation detect the opacity change and invalidate just the tile
> in question?

Hmmm. I think I would need to walk the layers at the beginning of the render pass, and ask them to add invalidated regions for the tiles that are fading in. Seems hard to me, but I can look into it.

At any rate, Android is not one of the platforms that can do partial presents, AFAIK, so this wouldn't affect us there.

Comment 33

[Matt Woodrow (:mattwoodrow)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #32)

> 
> Hmmm. I think I would need to walk the layers at the beginning of the render
> pass, and ask them to add invalidated regions for the tiles that are fading
> in. Seems hard to me, but I can look into it.
> 
> At any rate, Android is not one of the platforms that can do partial
> presents, AFAIK, so this wouldn't affect us there.

Have a look at LayerTreeProperties::CloneFrom and ComputeDifferences. They do this walk already.

Comment 34

[Nicolas Silva [:nical]]

Comment on attachment 8658360 [details] [diff] [review]
Optionally fade in new progressively painted tiles

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

::: gfx/layers/composite/TextureHost.h
@@ +532,5 @@
>  
>    virtual void WaitAcquireFenceHandleSyncComplete() {};
>  
> +  void StartFadeIn(TimeStamp aNow) {
> +    mFadeStart = aNow;

I would much prefer that this stays out of the TextureHost class. Could you move this into TileHost (you can blame Matt if he made this difficult :p)?

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Optionally fade in new progressively painted tiles

This version uses CompositeUntil() and the layer tree invalidation, as Matt suggested. It also
moves the fading logic into TileHost as nical suggests and removes the TimeStamp arguments.

Matt, I wondered what you would think about merging the new RecycleTileFading() stuff with
one of the other RecycleTextureSource* calls (likely RecycleTextureSourceForTile). We'd have
to remove the optimization where we skip over some tiles, and it'll be a little less
clear what's going on, but we could save some iterations.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8658919 [details] [diff] [review]
Optionally fade in new progressively painted tiles

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

::: gfx/layers/composite/PaintedLayerComposite.cpp
@@ +181,5 @@
>  
> +const nsIntRegion
> +PaintedLayerComposite::GetInvalidRegion()
> +{
> +  nsIntRegion region = mValidRegion;

mInvalidRegion!

Comment 37

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #36)
> Comment on attachment 8658919 [details] [diff] [review]
> Optionally fade in new progressively painted tiles
> 
> Review of attachment 8658919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/PaintedLayerComposite.cpp
> @@ +181,5 @@
> >  
> > +const nsIntRegion
> > +PaintedLayerComposite::GetInvalidRegion()
> > +{
> > +  nsIntRegion region = mValidRegion;
> 
> mInvalidRegion!

Whoops! Nice catch!

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1c6511123a
https://hg.mozilla.org/integration/mozilla-inbound/rev/83295d5f54a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9394c5f63b56

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out along with everything else in the push for android reftest orange: https://treeherder.mozilla.org/logviewer.html#?job_id=13972013&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/f37d3294ec16

Comment 40

[James Willcox (:snorp) ([e-mail]) (he/him)]

These pass now, presumably because of APZ stuff, so I pushed them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=805f1533df59

Comment 41

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

Pulsebot is napping

https://hg.mozilla.org/integration/mozilla-inbound/rev/4019894b820fb7b19a37643bb99bd4eb3bdb7c84
https://hg.mozilla.org/integration/mozilla-inbound/rev/47c2fd472fcaf91fdff15b01d6bf037e8f09f49a
https://hg.mozilla.org/integration/mozilla-inbound/rev/62114ac9b274064befdbcf2d9727c133e45156db

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4019894b820f
https://hg.mozilla.org/mozilla-central/rev/47c2fd472fca
https://hg.mozilla.org/mozilla-central/rev/62114ac9b274