Closed Bug 1180326 Opened 4 years ago Closed 4 years ago

Create a new ContentClient that supports async transaction without minimum size

Categories

(Core :: Graphics: Layers, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

(Whiteboard: [backout-asap])

Attachments

(6 files, 10 obsolete files)

47.61 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.63 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
10.89 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
15.69 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.69 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.36 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Most of our current ContentClients require sync transactions, except for tiling which has a fixed minimum size (that we want to increase).

We want a new simple ContentClient implementation that supports async transactions for small layers that we can use for non-scrollable layers.
Attached patch Part 4: Add new content client (obsolete) — Splinter Review
Blocks: 1126950
Attachment #8629520 - Attachment is obsolete: true
Attachment #8629521 - Attachment is obsolete: true
Attachment #8629522 - Attachment is obsolete: true
Attached patch Part 4: Add new content client (obsolete) — Splinter Review
Attachment #8629523 - Attachment is obsolete: true
Attachment #8630212 - Flags: review?(jmuizelaar)
Attachment #8630213 - Flags: review?(jmuizelaar)
Attachment #8630212 - Attachment is obsolete: true
Attachment #8630212 - Flags: review?(jmuizelaar)
Attachment #8631313 - Flags: review?(jmuizelaar)
Attachment #8630213 - Attachment is obsolete: true
Attachment #8630213 - Flags: review?(jmuizelaar)
Attachment #8631315 - Flags: review?(jmuizelaar)
Attachment #8630214 - Attachment is obsolete: true
Attachment #8631317 - Flags: review?(jmuizelaar)
Attachment #8630215 - Attachment is obsolete: true
Attachment #8631318 - Flags: review?(jmuizelaar)
Attachment #8630216 - Attachment is obsolete: true
Attachment #8631319 - Flags: review?(jmuizelaar)
Comment on attachment 8631315 [details] [diff] [review]
Part 2: Add support for variable tile sizes

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

::: gfx/layers/TiledLayerBuffer.h
@@ +193,5 @@
>    nsTArray<Tile>  mRetainedTiles;
>    TilesPlacement  mTiles;
>    float           mResolution;
>    gfx::IntSize    mTileSize;
> +  gfx::IntPoint   mTileOrigin;

Include a coordinate space in the type here.
Attachment #8631319 - Flags: review?(jmuizelaar) → review+
Attachment #8631313 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8631318 [details] [diff] [review]
Part 4: Add new content client

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

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +233,5 @@
>    if (!gfxPlatform::GetPlatform()->UseProgressivePaint()) {
>      // pref is disabled, so never do progressive
>      return false;
>    }
> +      

Whitespace

::: gfx/layers/client/SingleTiledContentClient.cpp
@@ +143,5 @@
> +
> +  extraPainted.MoveBy(mTilingOrigin);
> +  extraPainted.And(extraPainted, aNewValidRegion);
> +  mPaintedRegion.OrWith(paintRegion);
> +  mPaintedRegion.Or(mPaintedRegion, extraPainted);

mPaintedRegion.OrWith(extraPainted);

@@ +184,5 @@
> +  }
> +
> +  // Mark the area we just drew into the back buffer as invalid in the front buffer as they're
> +  // now out of sync.
> +  mTile.mInvalidFront.Or(mTile.mInvalidFront, paintRegion);

OrWith

::: gfx/layers/client/SingleTiledContentClient.h
@@ +126,5 @@
> +  virtual ClientTiledLayerBuffer* GetTiledBuffer() override { return mTiledBuffer; }
> +  virtual ClientTiledLayerBuffer* GetLowPrecisionTiledBuffer() override { return nullptr; }
> +
> +private:
> +  nsRefPtr<ClientSingleTiledLayerBuffer> mTiledBuffer;

Can't this just be:
ClientSingleTiledLayerBuffer mTiledBuffer;

It seems like the relationship is always 1:1

::: gfx/layers/client/TiledContentClient.h
@@ +411,5 @@
>                     const nsIntRegion& aPaintRegion,
>                     LayerManager::DrawPaintedLayerCallback aCallback,
>                     void* aCallbackData) = 0;
>  
> +  virtual bool SupportsProgressiveUpdate() { return true; }

It probably makes sense for this to be pure virtual like ProgressiveUpdate.
Attachment #8631318 - Flags: review?(jmuizelaar) → review+
Attachment #8631320 - Flags: review?(jmuizelaar) → review+
Attachment #8631315 - Flags: review?(jmuizelaar) → review+
Attachment #8631317 - Flags: review?(jmuizelaar) → review+
Blocks: 1188995
We will need this backed out asap. This is causing a smoketest blocker. Please see bug 1189261.
Flags: needinfo?(nhirata.bugzilla)
Whiteboard: [backout-asap]
Depends on: 1189399
This also break hwc composition on flame-kk. Local backout fixed the problem.
The fix for this bug is causing many identified issues on both Flame and Aries.

- Status bar can disappear or flicker in many apps (dialer, messages, music). bug 1189261
- Keyboard is flickering, flashes black: bug 1189399
- FTU Cellular Data screen shows black bars and cuts off part of the toggle switch. (Flame only) bug 1189417
- Card view will shift around and tile when swiping cards. (Flame only)
- Video player UI will flicker heavily.
- SIM selection in dialer when using multiple SIMs flickers heavily (Flame only)
- Camera video recording UI will flicker (possibly Flame only)
- Full screen dialogs 'jump' up when appearing on screen, specifically seen when changing screen time-out in Display settings.
- Email Setup for a new account, Display notifications for new messages toggle has shaded UI overlapping
Flags: needinfo?(matt.woodrow)
Patch to disable these patches are in bug bug 1189399.  I asked Sotaro to review that patch.
Depends on: 1189710
Flags: needinfo?(nhirata.bugzilla)
Depends on: 1192918
Flags: needinfo?(matt.woodrow)
Depends on: 1219230
Duplicate of this bug: 1112193
Depends on: 1223003
You need to log in before you can comment on or make changes to this bug.