Closed Bug 1670842 Opened 4 years ago Closed 4 years ago

Consider making picture caching required

Categories

(Core :: Graphics: WebRender, task, P3)

task

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: gw, Assigned: gw)

Details

Attachments

(1 file)

Should picture caching become required?

There are only two scenarios in which picture caching is disabled:

  1. During pinch-zoom on platforms without native OS compositor active.
  2. As a debug setting.

Picture caching is required even during pinch-zoom on platforms using an OS compositor (DC, CA, Wayland) since we must draw into OS surfaces. It is a performance win to disable picture caching during pinch-zoom on platforms with no OS compositor (e.g. android), however I'm not sure it's worth the extra complexity for that single use case (recent android versions do have a platform compositor API that we can take advantage of in the future, which will further reduce places the non-picture-caching path is active).

If we do require picture caching to be active in all cases, we get:

  • Remove an extra code path that requires maintenance and testing.
  • Dirty rects and partial present are always available, even during pinch-zoom.
  • Simplifies much of the CPU code that deals with 'compositor' clips (a performance win + code simplification)
  • Simplifies how we calculate clipped regions of child surfaces (performance win + code simplification)
  • Simplifies parts of the partial present handling renderer code paths.
  • Simplifies some upcoming optimization work to retain more state (e.g. tile assignments) between scenes / frames.

One scenario where it was previously beneficial to disable picture caching was for full-screen video / canvas. However, now that we promote these to compositor surfaces, there's no penalty to having picture caching enabled in these cases.

Flags: needinfo?(mstange.moz)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmathies)
Flags: needinfo?(jnicol)

I think I'm fine with this from an android perspective. Zooming on some devices seems to be either
a) fine, even with picture caching enabled, or
b) poor, even with picture caching disabled.

So I don't think it makes a huge difference. Fewer code paths is a good thing, and I suspect it would fix bug 1668804

Flags: needinfo?(jnicol)
Severity: -- → S3
Priority: -- → P3

Sounds good to me. I think regressing Android zooming perf would be the only concern, so if that's not an issue (according to comment 1), great!

Flags: needinfo?(mstange.moz)
Assignee: nobody → gwatson

This patch removes the public API and high level logic for
disabling picture caching for debugging and pinch-zoom in
some cases.

Follow up patches will remove and simplify the internal parts
of WR that remain to handle the disabled picture caching
code path.

I turn off picture caching to get consistent gpu times when looking at slow sites. Glenn suggested a better alternative is to have debug option to consider every tile invalid every frame and I agree that would be better.

Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmathies)
Pushed by gwatson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a99d2faf30de Pt 1 - Remove option to disable picture caching. r=jnicol
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: