Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 62820 Thumbnailer kicks in when updating the location with a fragment change
Starred by 25 users Project Member Reported by mihaip@chromium.org, Nov 11 2010 Back to list
Status: Fixed
Owner:
Email to this user bounced
Closed: Jul 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 96351
issue 120003

Blocking:
issue 121997

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
Page thumbnailing has less-than-desirable behavior:

1. Is triggered by URL fragment changes and push/replaceState (i.e. not full navigations)
2. Is not rate limited (if updating the fragment frequently, each will result in RenderView::CapturePageInfo being called)
3. Does a full page repaint, even if the browser already has the pixels for the current tab (if in the foreground)
4. Uses an expensive scaling algorithm

A testcase for this is at http://persistent.info/webkit/puzzlers/jerky-animation.html (the animation jerks when the ball crosses the middle line, since that's 500ms after the location was changed).

A common real-world scenario that maps to the testcase is to have a page with multiple sections, and to transition between sections with an animation, while also updating the location (via fragment changes or pushState) to allow sections to be bookmarked.
 
Comment 1 by jam...@chromium.org, Nov 11 2010
Comment 2 by bugdro...@gmail.com, Nov 12 2010
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=65982

------------------------------------------------------------------------
r65982 | jamesr@chromium.org | Fri Nov 12 12:36:22 PST 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/render_view.cc?r1=65982&r2=65981&pathrev=65982

Downsample thumbnail by powers of two before doing lanczos

This speeds up the resizing portion of thumbnailing from 40ms->15ms on my linux z600 at nearly fullscreen resolution.  Thumbnail resizing times on weaker hardware (like netbooks) has been observed in the 150ms+ range, so hopefully this is a proportional speedup there as well.

TEST=none
BUG= 62820 

Review URL: http://codereview.chromium.org/4846002
------------------------------------------------------------------------
Comment 3 by bugdro...@gmail.com, Nov 12 2010
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=66012

------------------------------------------------------------------------
r66012 | jamesr@chromium.org | Fri Nov 12 15:03:12 PST 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/552/src/chrome/renderer/render_view.cc?r1=66012&r2=66011&pathrev=66012

Merge 65982 - Downsample thumbnail by powers of two before doing lanczos

This speeds up the resizing portion of thumbnailing from 40ms->15ms on my linux z600 at nearly fullscreen resolution.  Thumbnail resizing times on weaker hardware (like netbooks) has been observed in the 150ms+ range, so hopefully this is a proportional speedup there as well.

TEST=none
BUG= 62820 

Review URL: http://codereview.chromium.org/4846002

TBR=jamesr@chromium.org
Review URL: http://codereview.chromium.org/4860002
------------------------------------------------------------------------
Labels: Mstone-10
Status: Assigned
Comment 5 by kerz@chromium.org, Dec 9 2010
Labels: -Mstone-10 MovedFrom-10 Mstone-11
P2 bugs with an owner that are not marked as started are being automatically moved to mstone:11.
Project Member Comment 6 by bugdroid1@chromium.org, Jan 30 2011
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=73110

------------------------------------------------------------------------
r73110 | brettw@chromium.org | Sun Jan 30 09:58:21 PST 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations_unittest.cc?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.h?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver_unittest.cc?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver.cc?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver.h?r1=73110&r2=73109&pathrev=73110
 A http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations_bench.cc?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.cc?r1=73110&r2=73109&pathrev=73110
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/skia.gyp?r1=73110&r2=73109&pathrev=73110

Integration of most changes from the GoogleTV project around the convolver/scaler.
This contains the following improvements:
- Adding a few extra convolution filters on top of the existing LANCZOS3 (used
internally in Chrome), and BOX (used in unit tests):
- LANCZOS2: a variation of LANCZOS3 except that the windowed function is
limited to the [-2:2] range.
- HAMMING1: this uses a Hamming window using the [-1:-1] range.
If we define the zoom down factor to z, and w the size of the window,
the actual cost of each filter (CPU wise) is proportional to (w * 2 * z + 1).
So, if we look at what happens when you zoom down by a factor of 4 (as often
found when creating thumbnails), the cost would be 25 for LANCZOS3,
17 for LANCZOS2, and 9 for HAMMING.
As a result, HAMMING1 can end up be roughly three times as fast as the typical
LANCZOS3.
In terms of visual quality, HAMMING1 will be obviously worse than filters that
have a larger window.
The motivation of this change is that not all processors are equally equipped,
and while LANCZOS3 does provide good quality, it will be completely inadequate
in speed on slower processors (as found on Google TV), and it would be worth
trading some visual quality for speed.
Because the definitions of what is acceptable from one platform to another will
differ, this change adds generic enums describing various trade offs between
quality and speed. And depending on the platform, these would then be mapped
to different filters. This change does not contain the other changes made to
the all the call sites to transform LANCZOS3 to the appropriate enum. Another
CL will have to be checked in for the policy definition.

- Improvements in speed by around 10% (the actual speed up depends on the
parameters of the scale (scale ratios, sizes of images), as well as the actual
processor on which this is run on. The 10% was measured on scale down of
1920x1080 images to 1920/4x1080/4 using the LANCZOS3 filter on a 32bit Atom
based using the image_operations_bench. Actual numbers for a 64bit processor
are discussed below.
This optimization attempts to basically eliminate all zeroes on each side of
the filter_size, since it is very likely that the calculated window will go one
fraction of a pixel outside of the window where the function is actuall not
zero. In many cases, this means it gets rid the convolution by one point. So,
using the math above, (w * 2 * z + 1) will have 1 subtracted. The code though
is generic and will get rid of more points if possible.

- To measure speed, a small utility image_operations_bench was added. Its
purpose is to simply measure speed of the actual speed of the convolution
without any regards to the actual data. Run with --help for a list of options.
The actual measured number is in MB/s (source MB + dest MB / time).
The following numbers were found on a 64 bit Release build on a z600:
| zero optimization |
Filter | no | yes |
Hamming1 | 459 | 495 |
Lanczos2 | 276 | 294 |
Lanczos3 | 202 | 207 |
The command line was:
for i in HAMMING1 LANCZOS2 LANCZOS3 ; do echo $i; out/Release/image_operations_bench -source 1920x1080 -destination 480x270 -m $i -iter 50 ; done 
The actual improvements for the zero optimization mentioned above are much
more prevalent on a 32bit Atom.

- Commented that there is half-pixel error inside the code in image_operations.
Because this would effectively changes the results of many scales that are
used in win_layout tests, this would effectively break them. As a result, the
change here only adds comments about what needs to be changed, but does not
fix the issue itself. A subsequent change will remove the comments and enable
the fix, and also adds the corrected reference images used for the test.
See bug 69999: http://code.google.com/p/chromium/issues/detail?id=69999
- Enhanced the convolver to support arbitrary strides, instead of the hard
coded 4 * width. This value is correct on most platforms, but is not on
GoogleTV since buffers allocated need to be 32 pixel multiples to exploit HW
capabilities.

- Added numerous unit tests to cover the new filters as well as adding other
ones that are more rigourous than the existing ones. Such a test is the reason,
we have found the half pixel error mentioned above.

TEST=This was tested against the existing unit tests, and the added unit tests on
a 64 bit Linux platform. The tests were then ran under valgrind to check for
possible memory leaks/ and errors. The tests do come out clean (except the
preexisting file descriptor 'leaks' coming from other tests that are linked
with test_shell_tests

Actual credit to most of the actual changes go to various contributors of the
Google TV team.

Note that there are two types of optimizations that are possible beyond these
changes that are not done here:
1/ Use the fact that the filter coefficients will be periodic to reduce the cost
of calculating the coefficients (though typically in the noise), but rather when
the convolution is done to decrease cache misses on the coefficients.
Experiments showed that on an Atom, this can yield 5 % improvement.
2/ This code is the prime target for the use of SIMD instructions.

BUG= 47447 ,  62820 , 69999
Patch by evannier@google.com
Original review http://codereview.chromium.org/5575010/
------------------------------------------------------------------------
Project Member Comment 7 by bugdroid1@chromium.org, Mar 9 2011
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=77527

------------------------------------------------------------------------
r77527 | jiesun@chromium.org | Wed Mar 09 13:55:38 PST 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver_unittest.cc?r1=77527&r2=77526&pathrev=77527
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver.cc?r1=77527&r2=77526&pathrev=77527
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/convolver.h?r1=77527&r2=77526&pathrev=77527
 M http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.cc?r1=77527&r2=77526&pathrev=77527

SIMD implementation of Convolver for Lanczos filter etc.

replace current convolver function (horizontal/vertical) with SSE2 intrinsic version. Performance is not tuned to the optimal carefully in this patch. but it still should beat C version easily.

BUG= 62820 
TEST=unittest. and image_operation_bench.

Review URL: http://codereview.chromium.org/6334070
------------------------------------------------------------------------
Comment 8 by kareng@google.com, Mar 9 2011
Labels: -Mstone-11 Mstone-12 MovedFrom-11
rolling over all nonblockers from m11 to m12
Labels: -Jank bulkmove TaskForce-Jank
Page thumbnailing has less-than-desirable behavior:

1. Is triggered by URL fragment changes and push/replaceState (i.e. not full navigations)
2. Is not rate limited (if updating the fragment frequently, each will result in RenderView::CapturePageInfo being called)
3. Does a full page repaint, even if the browser already has the pixels for the current tab (if in the foreground)
4. Uses an expensive scaling algorithm

A testcase for this is at http://persistent.info/webkit/puzzlers/jerky-animation.html (the animation jerks when the ball crosses the middle line, since that's 500ms after the location was changed).

A common real-world scenario that maps to the testcase is to have a page with multiple sections, and to transition between sections with an animation, while also updating the location (via fragment changes or pushState) to allow sections to be bookmarked.
Owner: ----
Status: Available
I'm not working on this now and do not anticipate having time to work on it in the near future.  The correct fix to this is to do the thumbnailing on the browser side, which was a project that Brett was interested in at some point (but I dunno if anyone is actually working on it).
Comment 11 by k...@google.com, Apr 25 2011
Labels: -Mstone-12 Mstone-13 MovedFrom12
Moving out of M12.
Labels: -Mstone-13 Mstone-14 MovedFrom-13
Moving !type=meta|regression and !releaseblocker to next mstone
Labels: -MovedFrom12 MovedFrom-12
Comment 14 by k...@google.com, Jul 28 2011
Labels: -Mstone-14 Mstone-15 MovedFrom-14
Punting out non-critical bugs.  Please move back to 14 if you believe this was done in error.
Comment 15 by kareng@google.com, Sep 8 2011
Labels: MovedFrom15 builkmove
moving all non-essential bugs out of m15. please feel free to move back if it's a blocker.
Comment 16 by kareng@google.com, Sep 8 2011
Labels: Mstone-16
Comment 17 by lilli@google.com, Oct 17 2011
Labels: importantForGames
Comment 18 by lilli@google.com, Oct 17 2011
Labels: -importantForGames Hotlist-ImportantForGames
Comment 19 by laforge@google.com, Oct 24 2011
Labels: -Mstone-16 MovedFrom-16 Mstone-17
Comment 20 by lilli@google.com, Nov 4 2011
Owner: groby@chromium.org
Comment 21 by groby@chromium.org, Nov 14 2011
As for #1, fragment navigation currently _is_ treated as full navigation. (page_id on render view changes for fragment navigation)

This kind of disagrees with the comment in ChromeRenderView::CapturePageInfo (http://google.com/codesearch#OAMlx_jo-ck/src/chrome/renderer/chrome_render_view_observer.cc&type=cs&l=774) that suggests we want to ignore fragments for navigation purposes.

So, there's a disconnect in what should be considered proper policy.


Comment 22 by groby@chromium.org, Nov 14 2011
Cc: groby@chromium.org
Owner: satorux@chromium.org
Also, the proper fix (according to comment 10) is being worked on by satorux - transferring ownership.
Comment 23 Deleted
Comment 24 by groby@chromium.org, Nov 16 2011
in-browser thumbnailing is worked on in  crbug.com/65936 

Not sure if that's a dupe, since it doesn't address issues 1 & 2
Hey Satoru -- can you comment on whether you're working on doing thumbnailing in the browser side (and if that will fix this issue)? Thanks!
Blockedon: 96351
Owner: mazda@chromium.org
Browser side thumbnailing is now assigned to mazda@
Comment 27 by k...@google.com, Dec 19 2011
Labels: -Mstone-17 MovedFrom-17
Removing milestone on all bugs marked available and targeted at m17.  Please re-triage to a new milestone if desired.
Comment 28 by nduca@google.com, Jan 25 2012
Do we have a design doc for this? I'd like to understand our overall plan.
Comment 29 by mazda@chromium.org, Jan 25 2012
Not yet.
I'm currently focusing on in-browser thumbnailing ( http://crbug.com/96351 );
Comment 30 by jamesr@google.com, Jan 25 2012
It's the exact same problem.
Comment 31 by mazda@chromium.org, Jan 25 2012
Cc: satorux@chromium.org
Ah, yes. In-browser thumbnailing solves this issue and the remaining issue is that in-browser thumbnailing does not work on GPU composited pages.
I'm thinking of writing a design doc for the way to make in-browser thumbnailing work on GPU composited pages, but it's not done, yet.

Satoru-san,
Have you written any design doc on in-browser thumbnailing?
mazda@, unfortunately, no. Please write a doc, once you get something to work locally . :)
Cc: paulir...@chromium.org
Comment 34 by noel@chromium.org, Mar 1 2012
Cc: anan...@chromium.org lafo...@chromium.org hbridge@google.com
 Issue 71019  has been merged into this issue.
Status: Assigned
Available + Owner == Default to Assigned
Comment 36 by mazda@chromium.org, Mar 25 2012
Blockedon: 120003
Comment 37 by mazda@chromium.org, Mar 25 2012
Blockedon: -120003
Comment 38 by mazda@chromium.org, Mar 25 2012
Blockedon: 120003
Blocking: 121997
Comment 40 by mazda@chromium.org, Apr 11 2012
Labels: OS-All Mstone-20
This problem is heavily impacting the new animations in the web store.

- We change our URL as we navigate around (categories, detail page, etc...)
- We are building pretty heavy animations (zoom in detail dialog, a rich cross fade between categories)
- There are noticeable pauses during these animations (similars to one in test case supplied) that make animations look pretty bad (basically, prevent us from shipping the feature).

Is there any workaround that enables us to turn off thumbnailing on a per site basis. If not, could we exclude the Chrome Web Store from thumbnailing as a workaround to unblock us?

Dave: as a a workaround, can you delay the updating of the URL to until after the animation finishes?
Comment 43 by lingda@google.com, Apr 23 2012
If we can just disable thumbnailing for the Chrome Web Store, that would work too for the moment. We can use a known static image instead of taking a new thumbnail each time.
Comment 44 by k...@google.com, Apr 27 2012
Labels: -Mstone-20 MovedFrom-20
These bugs have hit their move limit.  Please re-target as appropriate.
Labels: -MovedFrom-10 -MovedFrom-11 -bulkmove -MovedFrom-13 -MovedFrom-12 -MovedFrom-14 -MovedFrom15 -builkmove -MovedFrom-16 -MovedFrom-17 -MovedFrom-20 Mstone-20
Special-casing something like this for the web store feels a little weird; it seems like this should just further motivate us to really fix the problem. mazda@ is working on improving thumbnailing overall and has a bunch of changes going into M20; perhaps he can comment on whether this will be addressed or not.
Comment 46 by lingda@google.com, Apr 30 2012
Yes, talked to mazda@ about this and we will be waiting for the change to go into M20 instead of doing any special casing for the web store.
Labels: -Mstone-20 bulkmove MovedFrom-20 Mstone-21
If this is still need to be part of M20, punt back.
Comment 48 by mazda@chromium.org, Jun 21 2012
Blockedon: -96351 -120003 chromium:96351 chromium:120003
Blocking: -121997 chromium:121997
Labels: -Mstone-21 Mstone-22
Comment 49 by mazda@chromium.org, Jul 19 2012
Blockedon: -chromium:96351 -chromium:120003 chromium:96351 chromium:120003
Status: Fixed
Now In-browser thumbnailing is enabled on all platforms ( issue 120003 ) and this issue is also fixed.
Project Member Comment 50 by bugdroid1@chromium.org, Oct 13 2012
Blocking: -chromium:121997 chromium:121997
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member Comment 51 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-Internals -Mstone-22 Cr-Internals M-22
Project Member Comment 52 by bugdroid1@chromium.org, Mar 14 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Sign in to add a comment