Reconsider viewport update policy when pinch zooming

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: snorp, Assigned: rbarker)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Right now when you pinch zoom out, you often see a broken-looking page because we don't have tiles for the newly exposed area. Only when you release the pinch do we update the viewport to include this area. I think we should consider other times to do the update. One strategy would be to use a timer, and update the viewport rect when there isn't any (significant) pinching activity after, say, 250ms. That way we don't introduce compositor jank for quick pinchs, but slower ones where you are more likely to notice weird stuff will get updated. Since this is mostly a problem when we zoom out, we could also restrict this behavior to that case.
Blocks: 776030
No longer blocks: 1206874
(Assignee)

Comment 1

2 years ago
Created attachment 8757027 [details] [diff] [review]
0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-16052614-30ddb0a.patch
(Assignee)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 2

2 years ago
Created attachment 8757036 [details] [diff] [review]
0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-r-16052614-dd6eaf3.patch
Attachment #8757027 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8757036 - Flags: review?(botond)
Comment on attachment 8757036 [details] [diff] [review]
0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-r-16052614-dd6eaf3.patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3874,5 @@
> +void AsyncPanZoomController::DoDelayedRequestContentRepaint()
> +{
> +  if (!IsDestroyed() && mPaintTimerSet) {
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +    ScheduleComposite();

There shouldn't be a need to schedule another composite here - we schedule one on each scale event already, and when the repaint comes back that will schedule its own.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +1196,5 @@
>    // GetSnapPointForDestination). It should generally be determined by the
>    // type of event that's triggering the scroll.
>    Maybe<CSSPoint> FindSnapPointNear(const CSSPoint& aDestination,
>                                      nsIScrollableFrame::ScrollUnit aUnit);
> +  void DoDelayedRequestContentRepaint();

We've started organizing the members of AsyncPanZoomController by functionality. Notice the comments that say "The functions and members in this section are used to ...". These sections are towards the end of the class declaration, and then there's still a bunch of uncategorized stuff earlier on.

In keeping with this, please do one of the following:

  1) Find a relevant section, and add these members there.
     (I had a quick look and didn't see an obvious fit.)

  2) Add a new section for these members.

  3) Put these members in the uncategorized section.

@@ +1197,5 @@
>    // type of event that's triggering the scroll.
>    Maybe<CSSPoint> FindSnapPointNear(const CSSPoint& aDestination,
>                                      nsIScrollableFrame::ScrollUnit aUnit);
> +  void DoDelayedRequestContentRepaint();
> +  bool mPaintTimerSet;

The name of this variable should include the word "scale" or "pinch", to make it clear it's specific to that, as opposed to being a more general paint timer.

::: gfx/thebes/gfxPrefs.h
@@ +186,5 @@
>    DECL_GFX_PREF(Live, "apz.y_skate_highmem_adjust",            APZYSkateHighMemAdjust, float, 0.0f);
>    DECL_GFX_PREF(Live, "apz.y_skate_size_multiplier",           APZYSkateSizeMultiplier, float, 2.5f);
>    DECL_GFX_PREF(Live, "apz.y_stationary_size_multiplier",      APZYStationarySizeMultiplier, float, 3.5f);
>    DECL_GFX_PREF(Live, "apz.zoom_animation_duration_ms",        APZZoomAnimationDuration, int32_t, 250);
> +  DECL_GFX_PREF(Live, "apz.scale_repaint_delay_ms",            APZScaleRepaintDelay, int32_t, 500);

Please add the pref to modules/libpref/init/all.js as well.
Attachment #8757036 - Flags: review?(botond) → review+

Comment 4

2 years ago
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be44f7cfc72
Throttle repaints when pinch zooming r=botond

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9be44f7cfc72
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.