Closed Bug 1178376 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 9 obsolete files)

11.10 KB, patch
snorp
: review+
Details | Diff | Splinter Review
4.44 KB, patch
snorp
: review+
Details | Diff | Splinter Review
20.71 KB, patch
nical
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
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 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.
Attachment #8627276 - Flags: feedback?(nical.bugzilla) → feedback+
Assignee: nobody → snorp
Status: NEW → ASSIGNED
Whiteboard: [gfx-noted]
Attachment #8627276 - Attachment is obsolete: true
Attachment #8631304 - Flags: review?(nical.bugzilla)
Attachment #8631304 - Flags: review?(bgirard)
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 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.
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?
Attachment #8631846 - Flags: review?(bgirard)
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.
Attachment #8631304 - Attachment is obsolete: true
Attachment #8631304 - Flags: review?(nical.bugzilla)
Attachment #8631304 - Flags: review?(bgirard)
Attachment #8631847 - Flags: review?(nical.bugzilla)
Attachment #8631847 - Flags: review?(bgirard)
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 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.
Attachment #8631846 - Flags: review?(bgirard) → review-
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?
Attachment #8631847 - Flags: review?(bgirard) → review+
(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.
(In reply to Benoit Girard (:BenWa) from comment #12)
Thanks, fixed that stuff
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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
Attachment #8631846 - Attachment is obsolete: true
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.
Attachment #8633630 - Flags: review?(bugmail.mozilla) → review+
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+.
Attachment #8633630 - Flags: review?(bugmail.mozilla)
Attachment #8633630 - Flags: review?(bgirard)
Attachment #8633630 - Flags: review+
Comment on attachment 8633630 [details] [diff] [review]
Allow progressive painting when low-precision tiles are disabled

I clobbered :kats r+ by accident.
Attachment #8633630 - Flags: review?(bugmail.mozilla) → review+
Attachment #8631847 - Attachment is obsolete: true
Attachment #8631847 - Flags: review?(nical.bugzilla)
Attachment #8633688 - Flags: review?(nical.bugzilla)
Attachment #8633688 - Flags: review?(bugmail.mozilla)
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
Attachment #8631845 - Flags: review?(nical.bugzilla) → review-
(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 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
Attachment #8631845 - Flags: review- → review+
Attachment #8633688 - Flags: review?(nical.bugzilla) → review+
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 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
Attachment #8633688 - Flags: review?(bugmail.mozilla)
(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.
Unrot and adapt to changes Matt made in bug 1180326
Attachment #8633688 - Attachment is obsolete: true
Attachment #8658253 - Flags: review?(nical.bugzilla)
Attachment #8658253 - Flags: review?(matt.woodrow)
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?
This one seems to be a little more correct, as it avoids fading in the first batch of tiles.
Attachment #8658253 - Attachment is obsolete: true
Attachment #8658253 - Flags: review?(nical.bugzilla)
Attachment #8658253 - Flags: review?(matt.woodrow)
Attachment #8658360 - Flags: review?(nical.bugzilla)
Attachment #8658360 - Flags: review?(matt.woodrow)
(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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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)?
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.
Attachment #8658360 - Attachment is obsolete: true
Attachment #8658360 - Flags: review?(nical.bugzilla)
Attachment #8658360 - Flags: review?(matt.woodrow)
Attachment #8658919 - Flags: review?(nical.bugzilla)
Attachment #8658919 - Flags: review?(matt.woodrow)
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!
Attachment #8658919 - Flags: review?(matt.woodrow) → review+
(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!
Attachment #8658919 - Flags: review?(nical.bugzilla) → review+
These pass now, presumably because of APZ stuff, so I pushed them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=805f1533df59
https://hg.mozilla.org/mozilla-central/rev/4019894b820f
https://hg.mozilla.org/mozilla-central/rev/47c2fd472fca
https://hg.mozilla.org/mozilla-central/rev/62114ac9b274
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
You need to log in before you can comment on or make changes to this bug.