Closed Bug 836430 Opened 7 years ago Closed 6 years ago

[HiDPI] Store swipe animation snapshots in HiDPI on HiDPI hardware

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 - affected
firefox28 --- verified

People

(Reporter: spohl, Assigned: spohl)

References

(Depends on 2 open bugs, Blocks 1 open bug, )

Details

Attachments

(2 files, 8 obsolete files)

The swipe animation snapshots for bug 678392 should be stored in HiDPI when the user is on HiDPI hardware. The following tutorial explains the approach:
http://www.html5rocks.com/en/tutorials/canvas/hidpi/

The bug to implement an equivalent to webkitBackingStorePixelRatio on the canvas context (mentioned in tutorial) is bug 780362. Depending on the fix for bug 780362, we may get HiDPI snapshots for free or we will have to opt into it. We may also choose to implement a workaround (by increasing the canvas size to 200% and applying a CSS scale transform of 50%) while bug 780362 is not fixed.
Attached patch patch (obsolete) — Splinter Review
Here's a patch that works, but it isn't very elegant.
I'm not sure how much cleaner we can get it without built-in backing-store-scaling support in the canvas API or majorly changing the DOM structure of the snapshot containers.

Unfortunately, `background-size: contain` and its other magical values don't have the desired behavior here, until we're willing to throw away all the snapshots whenever the content area is resized.

I chose to have the code edit the stylesheet directly, because the affected elements do not yet exist when their sizes are declared.
Attachment #715696 - Flags: feedback?(spohl.mozilla.bugs)
Comment on attachment 715696 [details] [diff] [review]
patch

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

This patch covers exactly what I had in mind as a workaround, i.e. without backing-store-scaling support (bug 780362).

::: browser/base/content/browser-gestureSupport.js
@@ +993,5 @@
>      img.src = url;
>      return img;
>    },
>  
> +  _scale: window.matchMedia("(resolution: 2dppx)").matches ? 2 : 1,

We may want to make sure that this always returns true for computer systems with multiple monitors if one of them is HiDPI, no matter which monitor the browser is on (since the user may move tabs/windows between monitors).
Attachment #715696 - Flags: feedback?(spohl.mozilla.bugs) → feedback+
Depends on: history-swipe
Blocks: 673875
Duplicate of this bug: 873916
I think this is critical to fix now that bug 860493 has landed. The low-res screenshots seem to be used for both forwards/back swipes as well as overscroll. The latter, in particular, is a big problem on Retina. It's easy to trigger unintentionally, and causes a rather disorienting flicker to blurryness. A number of things on the page shift around by a few pixels too.

I'm not normally sensitive to motion-sickness from animations, but the blurring effect this causes has started to make me physically queasy from just a few minutes of playing with it.
Probably the main reason why this hasn't been fixed yet is due to concerns about memory consumption. We've already had to reduce the number of stored snapshots from 20 down to only 5 due to MaxHeap regressions [1]. By taking this patch, we'd likely be quadrupling the memory consumption on retina systems, which brings us right back to the equivalent of taking 20 snapshots at LoDPI...

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/NOCX_tDfFCA
Though the memory considerations only really apply for those snapshots we store permanently for the previous/next pages, not for the snapshot of the current page, right? We could keep the snapshot of the current page as a high resolution canvas during the animation, and then compress it to an image (and, if we must, downscale it to non-HiDPI) when the animation has finished. Or do we already do this in some way?

Also, not really related to HiDPI, but have you tried to store those snapshots that are not immediately needed, i.e. are not the direct previous or next page of the current tab, on disk? Though reloading them from disk on each tab switch might be expensive, too...
Great feedback. Thanks, Markus! My thoughts below:

(In reply to Markus Stange [:mstange] from comment #6)
> Though the memory considerations only really apply for those snapshots we
> store permanently for the previous/next pages, not for the snapshot of the
> current page, right? We could keep the snapshot of the current page as a
> high resolution canvas during the animation, and then compress it to an
> image (and, if we must, downscale it to non-HiDPI) when the animation has
> finished. Or do we already do this in some way?

We do use the canvas for the current page during the animation and the (async) image encoding is kicked off simultaneously. What we don't have is a 200% canvas, scaled down by 50% during animation. This may improve things a bit because the current page would be HiDPI, but since we'd most likely store a downscaled snapshot, we'd still be left with LoDPI for any previous or next pages. I'm also concerned that working with a 200% canvas, downscaling etc. would be costly in terms of responsiveness. The performance regressions we're already facing without a 200% canvas are referenced in bug 860493 comment 9.

> Also, not really related to HiDPI, but have you tried to store those
> snapshots that are not immediately needed, i.e. are not the direct previous
> or next page of the current tab, on disk? Though reloading them from disk on
> each tab switch might be expensive, too...

I've thought about this, but ended up not investigating it further when we decided to reduce the number of snapshots from 20 to 5. There is also the problem of fast-swiping. Just swiping twice in one direction quickly would bring us into a scenario where we would either have to display a blank page to be responsive, or wait for the snapshot to be loaded from disk. Without fast-swiping, when swiping back for example, there is still the question of when to load the 'new' previous snapshot from disk into memory for it to be ready for the next swipe.
spohl : given this https://bugzilla.mozilla.org/show_bug.cgi?id=860493#c23, (history swipe animation disabled by default) any reason to track this ? My initial intention of tracking this was that would be enabled by default and we may see this issue in the wild but looks like that's not the case here and this may be necessary to be tracked in that case.
Right, I don't think this needs to track for a particular release right now. Even though history swipe animations have now been turned on in nightly (and aurora once we merge), they will remain turned off in release until we're ready for them. For this particular bug here, although desirable from a UX perspective, we're not even 100% sure we're going to fix it due to the hit on memory consumption.
Blocks: 935852
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Stephen Pohl [:spohl] from comment #9)
> Right, I don't think this needs to track for a particular release right now.
> Even though history swipe animations have now been turned on in nightly (and
> aurora once we merge), they will remain turned off in release until we're
> ready for them. For this particular bug here, although desirable from a UX
> perspective, we're not even 100% sure we're going to fix it due to the hit
> on memory consumption.

Untracking for now in that case, please feel feel to uplift if the patch seems to fix the issue and is low risk .
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch Part1: Patch (obsolete) — Splinter Review
Updated patch for current trunk.

A few things that I'd like to add (in separate patches) before we land this:
1) Store snapshots in both hi-res and lo-res, when necessary. This means we'll have to store scale information along with the actual encoded snapshot in the snapshot array. This addresses multi-monitor systems.
2) Add an event listener to detect when the scale changes (rather than checking continuously or not at all like this first patch does).
Assignee: nobody → spohl.mozilla.bugs
Attachment #715696 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #828806 - Flags: feedback+
Duplicate of this bug: 935852
Store scale info along with the actual image data in the snapshot objects. Also added method descriptions to the methods added in the first patch and a few other minor style changes.
Attachment #829324 - Flags: review?(felipc)
Attachment #828806 - Attachment description: Patch → Part1: Patch
Attachment #828806 - Flags: review?(felipc)
This checks for the current resolution via window.devicePixelRatio. This allows us to take snapshots at any given resolution. If the browser moves from an external monitor to a Retina screen, previous snapshots will appear in the correct size, although blurry. I think this is an acceptable compromise and doesn't force us to store HiDPI snapshots when they aren't required.
Attachment #829328 - Flags: review?(felipc)
Duplicate of this bug: 936497
Depends on: 933389
Comment on attachment 828806 [details] [diff] [review]
Part1: Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +1114,5 @@
> +    }
> +  },
> +
> +  _rules: {},
> +  _getRuleBySelector: function HSA__getRuleBySelector(selector) {

So writing the rules in the stylesheet is unecessarily complicated and it might incur performance penalties, Frank said.
I think we can just set the rules in the style attribute of the element before adding it to the DOM.

Frank said that it was originally done this way because the elements were removed/re-added to the DOM, losing the attribute that was set. But if that is still how it works, we can just make sure to re-add the style attribute too before re-adding the element.  And I think this will also simplify this code a lot
Attachment #828806 - Flags: review?(felipc)
Comment on attachment 829324 [details] [diff] [review]
Part 2: Store image data and scale info in snapshot objects

Clearing this review because I think the changes to part 1 may cause changes here.
Attachment #829324 - Flags: review?(felipc)
Attachment #829328 - Flags: review?(felipc) → review+
Thanks, Felipe! Frank, would you like me to try and address Felipe's feedback, or do you have time to update your patch?
Flags: needinfo?(fryn)
Duplicate of this bug: 937337
Stephen, thanks for picking this up. I'm no longer contributing code to the Firefox project. Go for it!
Flags: needinfo?(fryn)
Duplicate of this bug: 936978
Attached patch Part 1 & 2 (obsolete) — Splinter Review
Tried to address feedback. Combined parts 1 and 2 for easier review.
Attachment #828806 - Attachment is obsolete: true
Attachment #829324 - Attachment is obsolete: true
Attachment #831031 - Flags: review?(felipc)
Updated for new patch (Part 1 & 2). Carrying over r+.
Attachment #829328 - Attachment is obsolete: true
Attachment #831033 - Flags: review+
Attached patch Part 1 & 2 (obsolete) — Splinter Review
Forgot to remove some commented-out code.
Attachment #831031 - Attachment is obsolete: true
Attachment #831031 - Flags: review?(felipc)
Attachment #831037 - Flags: review?(felipc)
Comment on attachment 831037 [details] [diff] [review]
Part 1 & 2

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

::: browser/base/content/browser-gestureSupport.js
@@ +1109,5 @@
> +   * @param aId
> +   *        The ID of a
> +   * @return Returns the CSS rule for the given selector, or null if none
> +   *         could be found.
> +   */

nit: need to update this comment block
Attachment #831037 - Flags: review?(felipc) → review+
Attached patch Part 1 & 2 (obsolete) — Splinter Review
Thank you, Felipe, for catching that! Updated comment block. Carrying over r+.
Attachment #831037 - Attachment is obsolete: true
Attachment #831062 - Flags: review+
Kicked off a try build before landing:
https://tbpl.mozilla.org/?tree=Try&rev=c3448db92409
Comment on attachment 831062 [details] [diff] [review]
Part 1 & 2

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

::: browser/base/content/browser-gestureSupport.js
@@ +1013,4 @@
>  
>      // Kick off snapshot compression.
>      aCanvas.toBlob(function(aBlob) {
> +        snapshots[currIndex].image = aBlob;

I may need some help on the JS side of things here. Browser-chrome tests are failing [1], claiming that snapshots[currIndex] is undefined here. Aren't we initializing this a few lines above? Shouldn't this be defined then? Or is the problem that we're doing this in a callback?

[1] https://tbpl.mozilla.org/php/getParsedLog.php?id=30513780&tree=Try&full=1
Attachment #831062 - Flags: feedback?(felipc)
Comment on attachment 831062 [details] [diff] [review]
Part 1 & 2

From the try log, I think what is undefined is not the value of currIndex, but "snapshots[currIndex]". So I imagine something running async is removing a snapshot in-between and changing the index values on the array. (probably through a call to _addSnapshotRefToArray I imagine).

Does that make sense?
Attachment #831062 - Flags: feedback?(felipc)
Attached patch Part 1 & 2Splinter Review
> From the try log, I think what is undefined is not the value of currIndex,
> but "snapshots[currIndex]".

I agree. It seems perfectly possible that we are removing the snapshot at this particular index before the async toBlob call completes. If it was deleted, there is no sense in trying to store the encoded image anymore, so a null check here should do the trick.

Only other two changes were to null check aBox in HSA__scaleSnapshot and to explicitly pass null to HSA__scaleSnapshot in HSA__installCurrentPageSnapshot when this._curBox is undefined. This can happen when no animation is running, which happens anytime the user navigates without swiping. This used to cause JS errors, visible in the error console.

Sending for another quick review and will kick off a try build shortly.
Attachment #831062 - Attachment is obsolete: true
Attachment #831977 - Flags: review?(felipc)
Updated for new patch (part 1 & 2). Carrying over r+.
Attachment #831033 - Attachment is obsolete: true
Attachment #831978 - Flags: review+
Sent to try, along with the patches in bug 938189:
https://tbpl.mozilla.org/?tree=Try&rev=006bb18afe21
Could the value of currIndex be changing in between the call and the callback?
Comment on attachment 831977 [details] [diff] [review]
Part 1 & 2

Still failing because |snapshots| is undefined now. Dropping r? while I'm looking into it.
Attachment #831977 - Flags: review?(felipc)
I suspect that the new failures are due to the patches in bug 938189. Resubmitted the patches here in isolation:
https://tbpl.mozilla.org/?tree=Try&rev=6cb53249e210
Comment on attachment 831977 [details] [diff] [review]
Part 1 & 2

Try looks green for the patches here, setting back to r?.
Attachment #831977 - Flags: review?(felipc)
Attachment #831977 - Flags: review?(felipc) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/efdc5d237e3f
https://hg.mozilla.org/mozilla-central/rev/7c3719fec07c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 940090
No longer depends on: history-swipe
Stephen, is there anything QA needs to verify here? If not please add the [qa-] whiteboard tag. If so please add the verifyme keyword with an explanation of the testing needed. Thanks.
Flags: needinfo?(spohl.mozilla.bugs)
(Note that this can only be tested on a Retina screen.)

The following STR should be good enough to verify the fix:
1. In about:config, set browser.snapshots.limit to a value greater than 0, such as 5.
2. Restart the browser.
3. Once restarted, swipe to the right or left. Any website, even about:start is okay to test with.

Expected Result:
The swipe animation should be using a snapshot of the current website that is crisp, rather than blurry. Please compare with a build prior to this patch to see what 'blurry' means.
Flags: needinfo?(spohl.mozilla.bugs)
Keywords: verifyme
I've verified this on a MBP 13" with Retina display using the steps in comment #40. The swipe animation uses crisp snapshots.

Notes:
- The animation itself is a little janky.
- If I don't let a page I am about visit completely and swipe back and then forth I can end up with an endless cycle of blank pages. This was easier to reproduce in some sites than others, but it's not reliable.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 1382560
No longer blocks: 1382560
You need to log in before you can comment on or make changes to this bug.