1197361 - New tab page images are larger than the need to be

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

We currently have some performance problems handling large images. The new tab pages hits these really badly and it doesn't seem to benefit very much from using such large images.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Set newtab scaled size to CSS size

Each newtab cell is 290x210 CSS pixels and the snapshotted tiles are 290x180px CSS pixels I think. Ugly WIP to test scaling and caching the correct sizes. Refreshing a newtab on this quad core Windows 7 w/ an Intel HD 4600 went from 52 ms with 9 tiles to 0.25 ms, a 213X improvement. Will flesh this out next week as we also have to correct a bit for high dpi devices.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Just a reminder for me later, newtab.css new tiles are set here:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.css#132

Comment 3

[Mason Chang [Inactive] [:mchang]]

You can try this for new tab, might not be as effective on hi-dpi yet.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Ugly WIP

This creates a new window with the nsIWindowWatcher to load the newTab.xhtml page. Then it can query the site cell's height/width and cache that value. However, this doesn't always work. Sometimes the nsIWindowWatcher fails to create the page for some reason.

Also, parsing newtab.css to fetch the width/height from source seems like a bad idea.

Jimm: Do you have a good route to get the newtab.css width/height at [1] during page snapshotting at [2]? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.css#133
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbUtils.jsm#57

Comment 5

[Vladan Djeric (:vladan)]

Tim: are thumbnails needed for anything other than about:newtab?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Yeah, Tab Groups and Ctrl+Tab use them as well.

Comment 7

[Jim Mathies [:jimm]]

PageThumbs is a generic, public api so changes oriented around controlling the size of the requested images should be built with that in mind. You can expand existing apis with optional params or add additional apis that specify new control settings that the front end then uses. Changes should: 1) not break existing consumers (front-end and addons), and 2) improve the control developers have over the thumbnails PageThumbs provides.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Mason Chang [:mchang] from comment #4)
> Jimm: Do you have a good route to get the newtab.css width/height at [1]
> during page snapshotting at [2]? Thanks!
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> newTab.css#133
> [2]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/
> PageThumbUtils.jsm#57

That might be a good question for someone like Dao. The only way I know of is to use an api like getComputedStyle of the rendered layout. There might be other tricks you can use.

Comment 9

[Vladan Djeric (:vladan)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Yeah, Tab Groups and Ctrl+Tab use them as well.

I think the benefit of using smaller thumbnails for better newtab performance greatly outweighs the inconvenience of using smaller thumbnails in Tab Groups.

How does Ctrl+Tab use thumbnails? Is this a Mac feature?

Comment 10

[Vladan Djeric (:vladan)]

(In reply to Jim Mathies [:jimm] from comment #7)
> PageThumbs is a generic, public api so changes oriented around controlling
> the size of the requested images should be built with that in mind. You can
> expand existing apis with optional params or add additional apis that
> specify new control settings that the front end then uses. Changes should:
> 1) not break existing consumers (front-end and addons), and 2) improve the
> control developers have over the thumbnails PageThumbs provides.

Mason is proposing a change that would greatly benefit newtab performance during a common operation for almost all users. There should be a discussion about the tradeoffs, not affecting existing consumers shouldn't be the only consideration IMHO

Comment 11

[Mason Chang [Inactive] [:mchang]]

Hi Dao,

Could you please answer questions in comment 4? Or if you have other proposals to allow Firefox's newtab to use the properly scaled image, I'd greatly appreciate it. Thanks!

Comment 12

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Mason Chang [:mchang] from comment #4)
> > Jimm: Do you have a good route to get the newtab.css width/height at [1]
> > during page snapshotting at [2]? Thanks!
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> > newTab.css#133
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/
> > PageThumbUtils.jsm#57
> 
> That might be a good question for someone like Dao. The only way I know of
> is to use an api like getComputedStyle of the rendered layout. There might
> be other tricks you can use.

Seems brittle since the window could be unmaximized and then postage-stamp sized thumbnails would carry over to maximized windows. It would probably be saner (and easier) to take a look at how the about:newtab layout is implemented and use that to generously approximate the thumbnail size based on the screen size.

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > (In reply to Mason Chang [:mchang] from comment #4)
> > > Jimm: Do you have a good route to get the newtab.css width/height at [1]
> > > during page snapshotting at [2]? Thanks!
> > > 
> > > [1]
> > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> > > newTab.css#133
> > > [2]
> > > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/
> > > PageThumbUtils.jsm#57
> > 
> > That might be a good question for someone like Dao. The only way I know of
> > is to use an api like getComputedStyle of the rendered layout. There might
> > be other tricks you can use.
> 
> Seems brittle since the window could be unmaximized and then postage-stamp
> sized thumbnails would carry over to maximized windows. It would probably be
> saner (and easier) to take a look at how the about:newtab layout is
> implemented and use that to generously approximate the thumbnail size based
> on the screen size.

Is there a way we cannot approximate the thhumbnail size based on the screen size? We're already guessing and getting bad results. As long as we can get the specified CSS Size, we will have the exact size and it will perform better. Thanks!

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #13)
> Is there a way we cannot approximate the thhumbnail size based on the screen
> size? We're already guessing and getting bad results.

What we're doing right now isn't at all an approximation of the about:newtab thumbnail size if I remember correctly. Or if it is, it's a very bad approximation and we can definitely do better.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > (In reply to Mason Chang [:mchang] from comment #4)
> > > Jimm: Do you have a good route to get the newtab.css width/height at [1]
> > > during page snapshotting at [2]? Thanks!
> > > 
> > > [1]
> > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> > > newTab.css#133
> > > [2]
> > > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/
> > > PageThumbUtils.jsm#57
> > 
> > That might be a good question for someone like Dao. The only way I know of
> > is to use an api like getComputedStyle of the rendered layout. There might
> > be other tricks you can use.
> 
> Seems brittle since the window could be unmaximized and then postage-stamp
> sized thumbnails would carry over to maximized windows.

So it looks like about:newtab doesn't change the thumbnail size based on the window size nor the screen size. newTab.css just hardcodes the size in CSS pixels regardless of any outside factors. This is a little bit surprising to me and it means that if we restrict thumbnails to that size, they will definitely be too small for other API consumers :(

Comment 16

[Jim Mathies [:jimm]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #9)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > Yeah, Tab Groups and Ctrl+Tab use them as well.
> 
> I think the benefit of using smaller thumbnails for better newtab
> performance greatly outweighs the inconvenience of using smaller thumbnails
> in Tab Groups.
> 
> How does Ctrl+Tab use thumbnails? Is this a Mac feature?

I don't see any problem with the use of smaller thumbnail sizes by default, we just need to be careful about not breaking existing consumers.

Looking at addon usage, those that care about dims use the getThumbnailSize helper for dims, which is great to see. Others ignore dims completely and just ask for a capture and resulting canvas, while others load the local thumbnail file directly via FileUtils apis and PageThumbsStorage.getFilePathForURL(url).

Updating getThumbnailSize seems safe. When I revamped some of these apis last year I was curious why we chose 1/3 the dims of the desktop myself as it seemed a bit excessive. My guess was that it was due to ctrl-tab, which likes really big thumbnails for its ui.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #16)
> Updating getThumbnailSize seems safe. When I revamped some of these apis
> last year I was curious why we chose 1/3 the dims of the desktop myself as
> it seemed a bit excessive. My guess was that it was due to ctrl-tab, which
> likes really big thumbnails for its ui.

Nope. Ctrl+Tab displays six thumbnails in a row plus some margin inside or outside the panel, so for that something like 1/7 of the screen size would have been sufficient.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [:dao] from comment #17)
> (In reply to Jim Mathies [:jimm] from comment #16)
> > Updating getThumbnailSize seems safe. When I revamped some of these apis
> > last year I was curious why we chose 1/3 the dims of the desktop myself as
> > it seemed a bit excessive. My guess was that it was due to ctrl-tab, which
> > likes really big thumbnails for its ui.
> 
> Nope. Ctrl+Tab displays six thumbnails in a row plus some margin inside or
> outside the panel, so for that something like 1/7 of the screen size would
> have been sufficient.

I never understood why it was 1/3, that was the way the code was. As for ctrl-tab, I currently get a ratio of about 1/5 with 3 tabs displayed (from measuring a screenshot in photoshop).

Comment 19

[Dão Gottwald [:dao]]

(In reply to Jim Mathies [:jimm] from comment #18)
> As for
> ctrl-tab, I currently get a ratio of about 1/5 with 3 tabs displayed (from
> measuring a screenshot in photoshop).

That doesn't seem to make sense... The thumbnail size is determined depending on the max. number of thumbnails (which is six):
http://hg.mozilla.org/mozilla-central/annotate/fb720c90eb49/browser/base/content/browser-ctrlTab.js#l162

Comment 20

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Mason Chang [:mchang] from comment #13)
> > Is there a way we cannot approximate the thhumbnail size based on the screen
> > size? We're already guessing and getting bad results.
> 
> What we're doing right now isn't at all an approximation of the about:newtab
> thumbnail size if I remember correctly. Or if it is, it's a very bad
> approximation and we can definitely do better.

The problem is that we cache approximate page thumb nails based on screen size / 3, then explicitly scale them down to a specific size in CSS via newtab.css. This is very very slow. From comment 1, we can get a 213x performance rendering improvement by just caching the page thumb nail sizes to the same size as explicitly set in newtab.css. Hence, I wanted to fetch the explicitly set size in newtab.css and create thumbnails to that size. Sorry for the poor explanation before. 

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Jim Mathies [:jimm] from comment #18)
> > As for
> > ctrl-tab, I currently get a ratio of about 1/5 with 3 tabs displayed (from
> > measuring a screenshot in photoshop).
> 
> That doesn't seem to make sense... The thumbnail size is determined
> depending on the max. number of thumbnails (which is six):
> http://hg.mozilla.org/mozilla-central/annotate/fb720c90eb49/browser/base/
> content/browser-ctrlTab.js#l162

This also doesn't seem to be the case anymore. On my Thinkpad, I can get 8 tiles. On my Macbook Pro, I get 12 tiles. On my Desktop with an external monitor, I get 15 tiles.

What can we do so that we cache and store the thumbnail sizes to the explicitly set CSS size in newtab.css? Thanks!

Comment 21

[Jim Mathies [:jimm]]

(In reply to Mason Chang [:mchang] from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > (In reply to Mason Chang [:mchang] from comment #13)
> > > Is there a way we cannot approximate the thhumbnail size based on the screen
> > > size? We're already guessing and getting bad results.
> > 
> > What we're doing right now isn't at all an approximation of the about:newtab
> > thumbnail size if I remember correctly. Or if it is, it's a very bad
> > approximation and we can definitely do better.
> 
> The problem is that we cache approximate page thumb nails based on screen
> size / 3, then explicitly scale them down to a specific size in CSS via
> newtab.css. This is very very slow. From comment 1, we can get a 213x
> performance rendering improvement by just caching the page thumb nail sizes
> to the same size as explicitly set in newtab.css. Hence, I wanted to fetch
> the explicitly set size in newtab.css and create thumbnails to that size.
> Sorry for the poor explanation before. 

Sounds like page thumbs caching needs to cache more than one image. It could start with the size we use now, or slightly reduced, then add to the cache two or three levels below if and when smaller thumbnails are requested. It would only do the conversion to a smaller size once.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > (In reply to Mason Chang [:mchang] from comment #13)
> > > Is there a way we cannot approximate the thhumbnail size based on the screen
> > > size? We're already guessing and getting bad results.
> > 
> > What we're doing right now isn't at all an approximation of the about:newtab
> > thumbnail size if I remember correctly. Or if it is, it's a very bad
> > approximation and we can definitely do better.
> 
> The problem is that we cache approximate page thumb nails based on screen
> size / 3, then explicitly scale them down to a specific size in CSS via
> newtab.css. This is very very slow. From comment 1, we can get a 213x
> performance rendering improvement by just caching the page thumb nail sizes
> to the same size as explicitly set in newtab.css. Hence, I wanted to fetch
> the explicitly set size in newtab.css and create thumbnails to that size.
> Sorry for the poor explanation before. 

I understood all that. However the main problem is not that we approximate (if you want to call it that), but that it's a particularly bad approximation. 1/3 of the screen size doesn't seem to have anything to do with the actual sizes displayed by any of our PageThumb consumers. We should be able to tweak that significantly.

> (In reply to Dão Gottwald [:dao] from comment #19)
> > (In reply to Jim Mathies [:jimm] from comment #18)
> > > As for
> > > ctrl-tab, I currently get a ratio of about 1/5 with 3 tabs displayed (from
> > > measuring a screenshot in photoshop).
> > 
> > That doesn't seem to make sense... The thumbnail size is determined
> > depending on the max. number of thumbnails (which is six):
> > http://hg.mozilla.org/mozilla-central/annotate/fb720c90eb49/browser/base/
> > content/browser-ctrlTab.js#l162
> 
> This also doesn't seem to be the case anymore. On my Thinkpad, I can get 8
> tiles. On my Macbook Pro, I get 12 tiles. On my Desktop with an external
> monitor, I get 15 tiles.

We were talking about Ctrl+Tab here, not about:newtab.

> What can we do so that we cache and store the thumbnail sizes to the
> explicitly set CSS size in newtab.css? Thanks!

I don't think we should capture at the size set in newtab.css, since that size is hardcoded independently from the screen size which means that on large screens, the thumbnails would need to be upscaled for consumers not hardcoding the size like about:newtab does.

Comment 23

[Dão Gottwald [:dao]]

Attachment: better approximation

Here's how heuristics that are more sensible than "1/3 of the screen size" could look like.

Comment 24

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Mason Chang [:mchang] from comment #20)
> > (In reply to Dão Gottwald [:dao] from comment #14)
> > > (In reply to Mason Chang [:mchang] from comment #13)
> > > > Is there a way we cannot approximate the thhumbnail size based on the screen
> > > > size? We're already guessing and getting bad results.
> > > 
> > > What we're doing right now isn't at all an approximation of the about:newtab
> > > thumbnail size if I remember correctly. Or if it is, it's a very bad
> > > approximation and we can definitely do better.
> > 
> > The problem is that we cache approximate page thumb nails based on screen
> > size / 3, then explicitly scale them down to a specific size in CSS via
> > newtab.css. This is very very slow. From comment 1, we can get a 213x
> > performance rendering improvement by just caching the page thumb nail sizes
> > to the same size as explicitly set in newtab.css. Hence, I wanted to fetch
> > the explicitly set size in newtab.css and create thumbnails to that size.
> > Sorry for the poor explanation before. 
> 
> I understood all that. However the main problem is not that we approximate
> (if you want to call it that), but that it's a particularly bad
> approximation. 1/3 of the screen size doesn't seem to have anything to do
> with the actual sizes displayed by any of our PageThumb consumers. We should
> be able to tweak that significantly.
> 
> > (In reply to Dão Gottwald [:dao] from comment #19)
> > > (In reply to Jim Mathies [:jimm] from comment #18)
> > > > As for
> > > > ctrl-tab, I currently get a ratio of about 1/5 with 3 tabs displayed (from
> > > > measuring a screenshot in photoshop).
> > > 
> > > That doesn't seem to make sense... The thumbnail size is determined
> > > depending on the max. number of thumbnails (which is six):
> > > http://hg.mozilla.org/mozilla-central/annotate/fb720c90eb49/browser/base/
> > > content/browser-ctrlTab.js#l162
> > 
> > This also doesn't seem to be the case anymore. On my Thinkpad, I can get 8
> > tiles. On my Macbook Pro, I get 12 tiles. On my Desktop with an external
> > monitor, I get 15 tiles.
> 
> We were talking about Ctrl+Tab here, not about:newtab.

Sorry, what's the difference between Ctrl+Tab and about:newtab?

> 
> > What can we do so that we cache and store the thumbnail sizes to the
> > explicitly set CSS size in newtab.css? Thanks!
> 
> I don't think we should capture at the size set in newtab.css, since that
> size is hardcoded independently from the screen size which means that on
> large screens, the thumbnails would need to be upscaled for consumers not
> hardcoding the size like about:newtab does.

Couldn't we use the nsIScreenManager to get the scaling factor and upscale once and cache that size?

Comment 25

[Vladan Djeric (:vladan)]

(In reply to Mason Chang [:mchang] from comment #24) 
> Sorry, what's the difference between Ctrl+Tab and about:newtab?

I found out Ctrl+Tab is a tab-preview feature hidden behind browser.ctrlTab.previews
On my monitor, it looks like it uses a smaller thumbnail to preview tab contents than the about:newtab thumbnails

Comment 26

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #24)
> > I don't think we should capture at the size set in newtab.css, since that
> > size is hardcoded independently from the screen size which means that on
> > large screens, the thumbnails would need to be upscaled for consumers not
> > hardcoding the size like about:newtab does.
> 
> Couldn't we use the nsIScreenManager to get the scaling factor and upscale
> once and cache that size?

There's no point in caching that. The problem with upscaling isn't performance but quality.

Comment 27

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Mason Chang [:mchang] from comment #24)
> > > I don't think we should capture at the size set in newtab.css, since that
> > > size is hardcoded independently from the screen size which means that on
> > > large screens, the thumbnails would need to be upscaled for consumers not
> > > hardcoding the size like about:newtab does.
> > 
> > Couldn't we use the nsIScreenManager to get the scaling factor and upscale
> > once and cache that size?
> 
> There's no point in caching that. The problem with upscaling isn't
> performance but quality.

Err sorry. When we get the snapshot, we have the snapshot at the desktop size. We could query the DPI scaling factor and newtab.css for a final size that is actually displayed during the newtab. We should cache this size. Why can we not cache the actual size used in newtab rather than a random number size? What other use cases are blocking this? Also, if ctrl-tab is smaller sizes than newtab, ctrl-tab will get faster too.:

Comment 28

[Avi Halachmi (:avih)]

ttaubert and myself discussed this about a year ago when we wanted to speedup the newtab page. Other than the factors mentioned mentioned, I think I have two more:

1. The newtab page can be zoomed in/out (possibly to show more thumbs on low res screen), so the thumbs size at the newtab page is not necessarily fixed even if it is fixed at the CSS.

This was a bit buggy until few weeks ago (the zoom level didn't "stick") but it got fixed and now behaves well.


2. IIRC when we experimented with exact (newtab page) thumbnail capture size, the overall image quality was worse than by using this "two steps" resize (first to 1/3 of the screen size, then to the final thumb size).

Though this could either be actually fixed, or worked around by first capturing 1/3 screen size, and then resizing it again to the thumb size and storing it at this size.

Comment 29

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #28)
> ttaubert and myself discussed this about a year ago when we wanted to
> speedup the newtab page. Other than the factors mentioned mentioned, I think
> I have two more:
> 
> 1. The newtab page can be zoomed in/out (possibly to show more thumbs on low
> res screen), so the thumbs size at the newtab page is not necessarily fixed
> even if it is fixed at the CSS.
> 
> This was a bit buggy until few weeks ago (the zoom level didn't "stick") but
> it got fixed and now behaves well.

Hmm, do you mean actual size is not fixed? If the CSS specifies a certain size, isn't it going to be that size? When a user zooms, Gecko has to upscale/downscale depending on the zoom, but the CSS size didn't change right? Or is the newtab page doing some JS to detect zooms?

> 
> 
> 2. IIRC when we experimented with exact (newtab page) thumbnail capture
> size, the overall image quality was worse than by using this "two steps"
> resize (first to 1/3 of the screen size, then to the final thumb size).
> 
> Though this could either be actually fixed, or worked around by first
> capturing 1/3 screen size, and then resizing it again to the thumb size and
> storing it at this size.

Was this on a high dpi device? The "correct" resolution is the newtab CSS size * hidpi system scaling size. e.g., on a retina display, it wouldn't be the 290x180px, but 2x that since retina displays are a 2x scaling.

Comment 30

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #29)
> Hmm, do you mean actual size is not fixed?

I meant the size on screen depends on the zoom level of the newtab page. So if we want to avoid any kinds of scaling, this could be a factor.

> Was this on a high dpi device?

No, IIRC that was on a 1366x768 screen, and the page we tested this with was the Apple home page (which had big fonts with thin lines), and it had much more visible aliasing when the capture was directly to the small size of the newtab thumbnails, but looked much nicer if the capture was 1/3 screen size, and then rendered at the thumb size at the newtab page.

Comment 31

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #30)
> (In reply to Mason Chang [:mchang] from comment #29)
> > Hmm, do you mean actual size is not fixed?
> 
> I meant the size on screen depends on the zoom level of the newtab page. So
> if we want to avoid any kinds of scaling, this could be a factor.
>

This will already be a problem with the current approach. If a user has a small screen then zooms in, we still have to upscale the image since the thumbnail will be small.
 
> > Was this on a high dpi device?
> 
> No, IIRC that was on a 1366x768 screen, and the page we tested this with was
> the Apple home page (which had big fonts with thin lines), and it had much
> more visible aliasing when the capture was directly to the small size of the
> newtab thumbnails, but looked much nicer if the capture was 1/3 screen size,
> and then rendered at the thumb size at the newtab page.

This seems like a very different bug. There really shouldn't be a difference between capturing the image at the right size versus capturing it at 1/3 size then scaling it to the thumbnail size.

Comment 32

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP newtab

I broke down and just hard coded the CSS values of the newtab snapshots. We then grab either the window or system scaling factor and use that as the newtab cached image sizes. On the plus side, on hidpi devices such as a retina macbook pro, the images look much better. The screen resolution reported by OS X on a 15" retina is actually only 1440 x 990. Then, everything is scaled by 2x.

With the previous approach, we'd make the thumbnails down to 1440 / 3 = 480, 990 /3 = 330. We specified a 280x180 px CSS Size, which is really 560x360 pixel resolution on a retina display. So we'd downscale the image, then upscale it again to actually display it.

> > No, IIRC that was on a 1366x768 screen, and the page we tested this with was
> > the Apple home page (which had big fonts with thin lines), and it had much
> > more visible aliasing when the capture was directly to the small size of the
> > newtab thumbnails, but looked much nicer if the capture was 1/3 screen size,
> > and then rendered at the thumb size at the newtab page.
> 

I found the bug. If you just specify the thumbnail size and change that, it also changes what we choose to capture at [1]. If we crop a smaller portion of the page than what we did before, which happens since the thumbnail sizes are smaller, then it's easier to see the aliasing. This patch should fix that, but needs more testing.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbUtils.jsm?offset=100#70

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8657143 [details] [diff] [review]
WIP newtab

>+      /***
>+       * THESE VALUES ARE DEFINED IN newtab.css and hard coded.
>+       * If you change these values, ALSO CHANGE THEM IN newtab.css
>+       */
>+      const newTabSetCSSWidth = 290 * scale;
>+      const newTabSetCSSHeight = 180 * scale;

Having a toolkit module depend on a browser stylesheet is bad. See my patch for a cleaner implementation.

Comment 34

[Mason Chang [Inactive] [:mchang]]

Attachment: snapshot.png

I've narrowed this down to [1]. Basically, when we render a document, then scale it in canvas, we get really ugly text on Cairo and D2d 11 backends in the resulting scaled image. 

Previously, newtab would do a relatively minor scale (e.g. going from 1024x576 to ~800x400 - I forget the numbers) and cache this big image. The text would be ok with such a small scale factor. Then, in html when we'd have <img src="cachedTile" width=290" height="180", that goes through image lib code since we're just rendering an image. The image lib code has some logic that can directly render the image at 290x180 nicely, so we get clean results.

Attached is a screenshot of terrible text when scaling down to the 290x180px directly in canvas. 

I think the proper solution is to figure out why scaling is so terrible with canvas.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?from=CanvasRenderingContext2D.cpp#4832

Comment 35

[Avi Halachmi (:avih)]

I _think_ the exploration might end here https://dxr.mozilla.org/mozilla-central/source/image/RasterImage.cpp#1545

Basically, it's more work to downscale with high quality, and most bilinear/bicubic downscalers would produce ugly results for large (down)scale factors.

That's because it needs either pre-filtering of the image, or using bigger sampling kernels.

Or, a much simpler way to do that if you only have simple downscalers - downscale in few passes where each pass downscales to ~40-70%. That's essentially what's been happening till now with the newtab thumbs and that's why it ended up with reasonable quality.

AFAIK the high quality downscale in gecko doesn't happen in a single pass, and possibly only happens for stuff which is displayed to screen directly. It first renders the images to screen with rough quality downscale, then refines the downscale in a background thread and then replaces the images with higher quality ones.

This quality switch is actually visible when opening a new tab, and typically finishes within a frame or two after the initial rendering of the low quality thumbs.

Comment 36

[Mason Chang [Inactive] [:mchang]]

Attachment: original image

Comment 37

[Mason Chang [Inactive] [:mchang]]

Attachment: reddit original

Comment 38

[Mason Chang [Inactive] [:mchang]]

Attachment: reddit bad

Comment 39

[Mason Chang [Inactive] [:mchang]]

From talking with Jeff, the problem isn't the downscaling. The problem was that rendering text at a low resolution just produces bad results because graphics fundamentally can't make good looking text at 4 pixels high. The images are actually sharper, but the sharpness looks worse to the human eye than blurry images. The alternate solution is to render the document at 2x the thumbnail size, to get good looking text, then downscale that to the thumbnail size and cache that image. The results on my Windows 7 machine look roughly the same as master and it's faster! I'll do more measurements tomorrow.

Try build for anyone whose interested - https://treeherder.mozilla.org/#/jobs?repo=try&revision=320cad24bfaa

Comment 40

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #39)
> From talking with Jeff, the problem isn't the downscaling. The problem was
> that rendering text at a low resolution just produces bad results because
> graphics fundamentally can't make good looking text at 4 pixels high.

Text seems to be a real problem too, yes.

> The images are actually sharper, but the sharpness looks worse to the human eye
> than blurry images.

I don't think the human eye prefers blur. The images are sharper indeed, but in a bad way - a way which doesn't take all source pixels into account and therefore ends up with aliasing - and IMO that's what the human eye doesn't like. Also, N-passes downscale typically ends up blurrier than it can be compared to a proper single pass downscale.

Compare the two reddit thumbnails you posted and look at the "FREE $100" ad at the right side. The text is part of the ad-image (and therefore not an issue of rendering text at really small font size) and it doesn't look nice not because it's sharp, but rather because it's "uneven" and aliased - due to the incorrect sampling.

OTOH, the 2-pass downscale thumb is also a bit blurrier than it could have been, but between these levels of too much aliasing and too much blur, subjectively I prefer the blur.

Comment 41

[Mason Chang [Inactive] [:mchang]]

Attachment: Cache page thumbs based on newtab css size and system DPI

Instead of using the screen size, we base what we cache based on the newtab.css sizes set in newtab, which is 2900x180 CSS pixels. We then scale the thumbnail size by the system or window DPI to choose as the final thumbnail size. On high DPI devices, this creates better looking images. On 100% scale systems with high resolution monitors, this creates much smaller images that render much quicker.

Unfortunately, we can't just render the page at the small thumbnail size due to a variety of constraints. Instead, we render the page at 2x the thumbnail size in a temporary canvas, then downscale that rendered page by 0.5 to get the final result which is at the thumbnail size.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 8660173 [details] [diff] [review]
Cache page thumbs based on newtab css size and system DPI

(In reply to Mason Chang [:mchang] from comment #41)
> On 100% scale systems with high resolution monitors, this creates much smaller
> images that render much quicker.

As noted earlier, this is bad for API consumers that adjust the thumbnail size based on the screen size. You should ensure that the thumbnail size doesn't drop below a reasonable share of the screen size, as done in attachment 8656062 [details] [diff] [review].

Comment 43

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #42)
> Comment on attachment 8660173 [details] [diff] [review]
> Cache page thumbs based on newtab css size and system DPI
> 
> (In reply to Mason Chang [:mchang] from comment #41)
> > On 100% scale systems with high resolution monitors, this creates much smaller
> > images that render much quicker.
> 
> As noted earlier, this is bad for API consumers that adjust the thumbnail
> size based on the screen size. You should ensure that the thumbnail size
> doesn't drop below a reasonable share of the screen size, as done in
> attachment 8656062 [details] [diff] [review].

I incorporated most of attachment 8656062 [details] [diff] [review]. If there is no set preference, we use the screen size, otherwise for firefox.js, we use the newtab size.

Comment 44

[Dão Gottwald [:dao]]

I'm aware of that. My point is that we should have a minimum thumbnail size based on the screen size that trumps the pixel size in corner cases.

Comment 45

[Jim Mathies [:jimm]]

Comment on attachment 8660173 [details] [diff] [review]
Cache page thumbs based on newtab css size and system DPI

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

::: browser/app/profile/firefox.js
@@ +1949,5 @@
> +// These are the thumbnail width/height set in about:newtab.
> +// If you change this, ENSURE IT IS THE SAME SIZE SET
> +// by about:newtab. These values are in CSS pixels.
> +pref("toolkit.pageThumbs.width", 280);
> +pref("toolkit.pageThumbs.height", 190);

I really do not like the idea of hard coded values tailored for internal use in a public api.

Also what are the side effects of a user going in and changing these, or an addon? This seems very brittle. I like Dao's suggestion of a minimum bounds vs. hard coded values. Also if a user goes in and makes these minimum values larger, newtab should be able to handle it.

For future reference, you should seek review from Dao, he owns the PageThumbs code, I just updated it for e10s.

Comment 46

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #44)
> I'm aware of that. My point is that we should have a minimum thumbnail size
> based on the screen size that trumps the pixel size in corner cases.

I really disagree with this, at least in the newtab case. Consider the 3 cases of displays
I have available to me:

Screen 1: Dell U2713H - Resolution: 2560 x 1440. DPI: 1.
Screen 2: Thinkpad T450s - Resolution: 1920x1080. DPI: 1.5
Screen 3: Retina 15" Macbook Pro - Resolution: 1440 x 900. DPI: 2

The target resolution of the images is 290px X 180px, already hard coded in the CSS. Depending on the system scale, the final size of the images should be:

Screen 1: 290x180.
Screen 2: 290 * 1.5 x 180 * 1.5 = 435x270.
Screen 3: 290 * 2 x 180 * 2 = 580 x 360.

Let's say we use the screen size, taking the divisor example of 3 as it stands today.
We get these image sizes:

Screen 1: 853x480 - too big and a 200x performance hit.
Screen 2: 640x360 - Still too big and have to downscale.
Screen 3: 480x300 - Too small. We downscale, then upscale again, producing ugly results.

Let's say we use 7 as the screen divisor in the example preference from attachment 8656062 [details] [diff] [review].
We'll produce the following images:

Screen 1: 365x205 - still too big.
Screen 2: 274x154 - Too small, even if we still scale by 1.5
Screen 3: 205x128 - Too small, even if we upscale by 2x.

At least for the newtab case, there is no "divisor" value that would work well here as a minimum.
We also see that on some devices, we get worse images and on other devices, we get large performance penalties. If we're already kind of hard coding newtab.css with specific sizes, we should create thumbnails to those sizes, at least for newtab.

Would it be better to carve out the existing places we call getThumbnailSize() for the newtab thumbnail images and instead have them call getNewtabThumbnailSize() within gecko? Then public API users could still use the thumbnail size based on screen size by calling getThumbnailSize().

Comment 47

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #46)
> Let's say we use 7 as the screen divisor in the example preference from
> attachment 8656062 [details] [diff] [review].
> We'll produce the following images:
> 
> Screen 1: 365x205 - still too big.
> Screen 2: 274x154 - Too small, even if we still scale by 1.5
> Screen 3: 205x128 - Too small, even if we upscale by 2x.
> 
> At least for the newtab case, there is no "divisor" value that would work
> well here as a minimum.

Which is why attachment 8656062 [details] [diff] [review] had toolkit.pageThumbs.minWidth and toolkit.pageThumbs.minHeight in addition to the divisor.

Comment 48

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #47)
> (In reply to Mason Chang [:mchang] from comment #46)
> > Let's say we use 7 as the screen divisor in the example preference from
> > attachment 8656062 [details] [diff] [review].
> > We'll produce the following images:
> > 
> > Screen 1: 365x205 - still too big.
> > Screen 2: 274x154 - Too small, even if we still scale by 1.5
> > Screen 3: 205x128 - Too small, even if we upscale by 2x.
> > 
> > At least for the newtab case, there is no "divisor" value that would work
> > well here as a minimum.
> 
> Which is why attachment 8656062 [details] [diff] [review] had
> toolkit.pageThumbs.minWidth and toolkit.pageThumbs.minHeight in addition to
> the divisor.

Yes and attachment 8656062 [details] [diff] [review] had it in Math.max(screenSize / divisor, preferenceValue). In screen 1, this will produce a result that is too big. If you go with a Math.min(), we can get something too small. Sorry for the bad explanation, but what exactly is so bad against using the exact value for newtab only then? Guessing based on screen size produces bad results.

Comment 49

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #48)
> (In reply to Dão Gottwald [:dao] from comment #47)
> > (In reply to Mason Chang [:mchang] from comment #46)
> > > Let's say we use 7 as the screen divisor in the example preference from
> > > attachment 8656062 [details] [diff] [review].
> > > We'll produce the following images:
> > > 
> > > Screen 1: 365x205 - still too big.
> > > Screen 2: 274x154 - Too small, even if we still scale by 1.5
> > > Screen 3: 205x128 - Too small, even if we upscale by 2x.
> > > 
> > > At least for the newtab case, there is no "divisor" value that would work
> > > well here as a minimum.
> > 
> > Which is why attachment 8656062 [details] [diff] [review] had
> > toolkit.pageThumbs.minWidth and toolkit.pageThumbs.minHeight in addition to
> > the divisor.
> 
> Yes and attachment 8656062 [details] [diff] [review] had it in
> Math.max(screenSize / divisor, preferenceValue). In screen 1, this will
> produce a result that is too big.

That's intended. Using only newtab's pixel values would otherwise give you thumnails that are too small for other consumers in that case.

Comment 50

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #49)
> (In reply to Mason Chang [:mchang] from comment #48)
> > (In reply to Dão Gottwald [:dao] from comment #47)
> > > (In reply to Mason Chang [:mchang] from comment #46)
> > > > Let's say we use 7 as the screen divisor in the example preference from
> > > > attachment 8656062 [details] [diff] [review].
> > > > We'll produce the following images:
> > > > 
> > > > Screen 1: 365x205 - still too big.
> > > > Screen 2: 274x154 - Too small, even if we still scale by 1.5
> > > > Screen 3: 205x128 - Too small, even if we upscale by 2x.
> > > > 
> > > > At least for the newtab case, there is no "divisor" value that would work
> > > > well here as a minimum.
> > > 
> > > Which is why attachment 8656062 [details] [diff] [review] had
> > > toolkit.pageThumbs.minWidth and toolkit.pageThumbs.minHeight in addition to
> > > the divisor.
> > 
> > Yes and attachment 8656062 [details] [diff] [review] had it in
> > Math.max(screenSize / divisor, preferenceValue). In screen 1, this will
> > produce a result that is too big.
> 
> That's intended. Using only newtab's pixel values would otherwise give you
> thumnails that are too small for other consumers in that case.

Yes, I understand, but what would it take so that we could have newtab, and only newtab, use the exact CSS pixel values. Guessing during newtab creation doesn't work. Would you prefer to carve out an exception for newtab ala comment 46? Can you please answer all questions in the need infos so we don't have to go back and forth or respond to pings on irc?

Comment 51

[Dão Gottwald [:dao]]

Storing thumbnails at different resolutions for different consumers seems over-engineered, I don't think we should do that.

Comment 52

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #51)
> Storing thumbnails at different resolutions for different consumers seems
> over-engineered, I don't think we should do that.

We already see that storing images for newtab, which is highly critical for firefox, produces both bad performance, up to 200x slower than the optimal resolution would be, AND produces worse results on hiDPI devices as we downscale the image THEN upscale it. We also store thumbnails at different resolutions already for different consumers. For example, if a user attaches a monitor, we'll already scale a different resolution versus if they use their laptop screen. This proposal actually streamlines it based on what is being used in newtab. We can avoid different resolutions for different consumers in general, but can we make an exception for newtab? Making rendering of the tiles of a newtab 210x faster, AND produce better results is a nice win. Please answer WHY this seems over engineered if we're already doing the things you say we shouldn't do.

Comment 53

[Dão Gottwald [:dao]]

"up to 200x slower" is a straw man, it refers to the status quo rather than what I proposed. It's not clear to me why you think this isn't an adequate compromise.

Comment 54

[Mason Chang [Inactive] [:mchang]]

The opinion of the graphics team is that we should cache the new tab images exactly at the size we want to render them to both improve performance and quality across a wide range of devices. If we can't use this for the entire thumbnail service, we need to special case newtab thumbnails. What do you think the best way to implement this is?

Comment 55

[Mason Chang [Inactive] [:mchang]]

(In reply to Dão Gottwald [:dao] from comment #53)
> "up to 200x slower" is a straw man, it refers to the status quo rather than
> what I proposed. It's not clear to me why you think this isn't an adequate
> compromise.

What I don't understand is, with your proposal in attachment 8656062 [details] [diff] [review], we will still have images that are too big for some contexts. 

At 2880x1440, a retina display that maximizes screen space, with a divisor of 7 = 411x205, which is still bigger. We still have to scale every single image on a tile.

Consider 4K displays, with a resolution of 4096 x 2160 pixels.

4096x2160 / 7 = 585 x 308. This is almost 2x bigger than we need it to be. 

To confirm performance, I remeasured the timings using attachment 8656062 [details] [diff] [review] on the original machine from comment 1. This is an Intel HD 4400, which from [1], Intel based GPUs are 65% of our windows user base. This was attached to a Dell U213H monitor at a resolution of 2560x1440. With attachment 865062, the resulting thumbnails become 366x206, much closer to the CSS size of 290x180. All timings are just rendering and drawing the images as measured in nsLayoutUtils::DrawImageInternal [2]. I browsed in full screen until newtab had 15 images, one of which was sponsored. 

At a resolution of 366x206, it took ~8.5 - 11.3 ms to render. At the CSS newtab resolution of 280x190 px, it took 1.25 - 1.31 ms to render, a 9x performance improvement. On this particular hardware, we have to allocate a new surface to render the downscaled version, which blocks the GPU driver. All browsers are affected by this on this Intel HD GPU, so it's best to minimize texture allocation. Without rendering the image at the exact size, we have to allocate a new texture and thus suffer this performance penalty.

I am still confused as to why rendering at the exact size of thumbnails, at least a special case for newtab, is not a good idea. It still provides a 9x performance improvement over the proposal in attachment 865062.

[1] http://people.mozilla.org/~danderson/moz-gfx-telemetry/www/#view=general
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?case=true&from=nsLayoutUtils.cpp#6243

Comment 56

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #55)
> At a resolution of 366x206, it took ~8.5 - 11.3 ms to render. At the CSS
> newtab resolution of 280x190 px, it took 1.25 - 1.31 ms to render, a 9x
> performance improvement.

Considering the perf hit we take today and since this would only apply in corner cases (i.e. huge low-DPI display), this seems acceptable to me.

> I am still confused as to why rendering at the exact size of thumbnails, at
> least a special case for newtab, is not a good idea. It still provides a 9x
> performance improvement over the proposal in attachment 865062.

Because the PageThumbs API serves more than just about:newtab. Maybe you can flesh out how exactly that API would change to satisfy different consumers, and then we can discuss that. However I would recommend wrapping up and landing the screen size divisor + min width/height fix first, since as I understand it, this would already be a huge step forward. We shouldn't let perfect be the enemy of good here.

Comment 57

[Mason Chang [Inactive] [:mchang]]

How can we make an exception for about:newtab?

Comment 58

[Mason Chang [Inactive] [:mchang]]

Attachment: One way to carve an exception for newtab

One possible way to carve an exception for newtab. It leaves PageThumbUtils.determineCropSize / getThumbnailSize as it stands today. It creates a new PageThumbUtils.getNewtabThumbnailSize() that is based on preferences that are set to the values in newetab.css. The current routes within Gecko that create thumbnails use getNewtabThumbnailSize() as a means to take a snapshot. The public API / addons would be unchanged.

Comment 59

[Dão Gottwald [:dao]]

(In reply to Mason Chang [:mchang] from comment #57)
> How can we make an exception for about:newtab?

I can't think of an elegant way, that's kind of why I dislike the idea. Consumers would need to tell PageThumbs what size they need and then PageThumbs would need to produce and store images at different sizes. Seems like a lot of effort for a smaller additional perf improvement in an edge case.

Comment 60

[Mason Chang [Inactive] [:mchang]]

Attachment: Use screen size as a minimum thumbnail size

Based off attachment 8656062 [details] [diff] [review], this uses the Max(screenSize / 7, newtab size). On low resolution monitors, this gets us to the correct CSS size, even on monitors up to 1920x1080. That means for most monitors less than 24", we should hit the correct size. On monitors > 24", we'll still scale and suffer performance penalties. From irc, dao thinks this is an edge case that we can ignore, which I disagree with.

(In reply to Dão Gottwald [:dao] from comment #59)
> (In reply to Mason Chang [:mchang] from comment #57)
> > How can we make an exception for about:newtab?
> 
> I can't think of an elegant way, that's kind of why I dislike the idea.
> Consumers would need to tell PageThumbs what size they need and then
> PageThumbs would need to produce and store images at different sizes. Seems
> like a lot of effort for a smaller additional perf improvement in an edge
> case.

Even using the minimum screen size, the engineering complexity is roughly the same. From attachment 8661318 [details] [diff] [review], we can just force Gecko callers to use the newtab size and other consumers can continue using the old path. Using a size based on the screen rect still produces smaller images on lower resolution monitors, so it would default to the CSS newtab size, which also still has to be added in the preference. Using a higher resolution monitor could produce better results for non-newtab, but then newtab still suffers.

Can we instead default newtab size and add another api for custom sizes instead? Then all consumers would default to newtab size and only those who really needed a different size could still have that availability?

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 8661372 [details] [diff] [review]
Use screen size as a minimum thumbnail size

(In reply to Mason Chang [:mchang] from comment #60)
> Even using the minimum screen size, the engineering complexity is roughly
> the same. From attachment 8661318 [details] [diff] [review], we can just
> force Gecko callers to use the newtab size and other consumers can continue
> using the old path. Using a size based on the screen rect still produces
> smaller images on lower resolution monitors, so it would default to the CSS
> newtab size, which also still has to be added in the preference. Using a
> higher resolution monitor could produce better results for non-newtab, but
> then newtab still suffers.
> 
> Can we instead default newtab size and add another api for custom sizes
> instead? Then all consumers would default to newtab size and only those who
> really needed a different size could still have that availability?

I would generally argue that PageThumbs shouldn't know about about:newtab, so if we change the API that would mean adding generic hooks for about:newtab to specify its needs.

I would recommend finishing and landing this patch and spinning off potential API changes to separate bugs.

>+// These are the thumbnail width/height set in about:newtab.
>+// If you change this, ENSURE IT IS THE SAME SIZE SET
>+// by about:newtab. These values are in CSS pixels.
>+pref("toolkit.pageThumbs.width", 280);
>+pref("toolkit.pageThumbs.height", 190);

rename to toolkit.pageThumbs.minWidth / minHeight for clarity

>--- a/browser/base/content/newtab/newTab.css
>+++ b/browser/base/content/newtab/newTab.css

>+/*
>+ * If you change the sizes here, make sure you
>+ * change the sizes in PageThumbUtils.jsm as well.
>+ */

The pref values need to reflect the .newtab-thumbnail dimensions from newTab.inc.css, so you can drop this rather confusing comment.

>--- a/browser/themes/shared/newtab/newTab.inc.css
>+++ b/browser/themes/shared/newtab/newTab.inc.css

>+/***
>+ * If you change the sizes here, change them in newtab.css
>+ * AND PageThumbUtils.jsm

mention the pref names here instead of PageThumbUtils.jsm

also, it's newTab.css with uppercase T, but note that there are multiple files with that name

Comment 62

[Mason Chang [Inactive] [:mchang]]

Attachment: Optimize page thumb sizes based on screen size

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 8661917 [details] [diff] [review]
Optimize page thumb sizes based on screen size

Looks good at a higher level. Tim originally wrote the PageThumbs module, I'll let him do the actual review.

Comment 64

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8661917 [details] [diff] [review]
Optimize page thumb sizes based on screen size

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

r=me with a satisfying answer to the Retina display question below.

::: toolkit/components/thumbnails/PageThumbUtils.jsm
@@ +33,3 @@
>     * @return The newly created canvas.
>     */
> +  createCanvas: function (aWindow, aWidth, aHeight) {

If they're optional params then maybe:

createCanvas(aWindow, aWidth = 0, aHeight = 0) {

That makes it clear just from looking at the function's signature.

@@ +50,4 @@
>     *
>     * @return The calculated thumbnail size or a default if unable to calculate.
>     */
> +  getThumbnailSize: function (aWindow) {

Please document the new parameter.

@@ +74,5 @@
> +       * on OS X.
> +       */
> +      if (AppConstants.platform == "macosx" && !aWindow) {
> +        scale = 2;
> +      }

Shouldn't we check that the user indeed has a Retina display? Or does it work anyway?

@@ +97,5 @@
>    },
>  
> +  /***
> +   * Given a browser window, this creates a snapshot of the content
> +   * and returns a canvas window with the resulting snapshot of the content

It returns a canvas, not a "canvas window", right?

@@ +110,5 @@
> +   * It's actually better to the eye to have small blurry text than sharp
> +   * jagged pixels to represent text.
> +   *
> +   * @params aWindow - the window to create a snapshot of.
> +   * @params aCanvas (optional) a destination canvas to draw the final snapshot to.

The actual parameter name differs from the one here.

@@ +113,5 @@
> +   * @params aWindow - the window to create a snapshot of.
> +   * @params aCanvas (optional) a destination canvas to draw the final snapshot to.
> +   * @return Canvas with a scaled thumbnail of the window.
> +   */
> +  createSnapshotThumbnail: function(aWindow, aDestCanvas) {

createSnapshotThumbnail(aWindow, aDestCanvas = null) {

@@ +149,5 @@
> +    snapshotCtx.save();
> +    snapshotCtx.scale(scale, scale);
> +    snapshotCtx.drawWindow(aWindow, 0, 0, contentWidth, contentHeight,
> +                   PageThumbUtils.THUMBNAIL_BG_COLOR,
> +                   snapshotCtx.DRAWWINDOW_DO_NOT_FLUSH);

The indentation here is off, please format that how we usually do it.

@@ +158,5 @@
> +
> +    // Part 2: Assumes that the snapshot is 2x the thumbnail size
> +    let finalCanvas = aDestCanvas
> +                        ? aDestCanvas
> +                        : this.createCanvas(aWindow, thumbnailWidth, thumbnailHeight);

let finalCanvas = aDestCanvas || this.createCanvas(aWindow, thumbnailWidth, thumbnailHeight);

Comment 65

[Mason Chang [Inactive] [:mchang]]

Attachment: Optimize page thumb sizes based on screen size

Carrying r+, updated with feedback from comment 64.

> Shouldn't we check that the user indeed has a Retina display? Or does it work anyway?

I couldn't find a way to check if the user has a Retina display based on the screen through JS. Usually, we generate thumbnails through [1], which has a window object so we can only get the 2x scaling from the window object. Once in a while, we create a thumbnail through [2], which preallocates a canvas without a window object, so we can't tell all the time if we're on a retina display. The current code will just upscale by 2x on macs, which will still work, it'll just be a tad slower on non-retina displays, but equal on retina displays. Is that satisfying? 

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js?offset=200#135
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbs.jsm#268

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mason Chang [:mchang] from comment #65)
> The current code will just upscale by 2x on macs, which will still
> work, it'll just be a tad slower on non-retina displays, but equal on retina
> displays. Is that satisfying? 

I think it still is the case that there are fewer users with a Retina display on Mac, those are expensive. So we would make those pay a penalty, somewhat at least, when capturing thumbnails.

Did you check if the image quality is what you want it to be after scaling it up and then down again on non-Retina Macs? And what's the time difference between scale=1 and scale=2 on those same machines?

Comment 67

[Mason Chang [Inactive] [:mchang]]

(In reply to Tim Taubert [:ttaubert] from comment #66)
> (In reply to Mason Chang [:mchang] from comment #65)
> > The current code will just upscale by 2x on macs, which will still
> > work, it'll just be a tad slower on non-retina displays, but equal on retina
> > displays. Is that satisfying? 
> 
> I think it still is the case that there are fewer users with a Retina
> display on Mac, those are expensive. So we would make those pay a penalty,
> somewhat at least, when capturing thumbnails.
> 
> Did you check if the image quality is what you want it to be after scaling
> it up and then down again on non-Retina Macs? And what's the time difference
> between scale=1 and scale=2 on those same machines?

I don't have access to a non retina mac so I can't say. The current code as it stands now would produce these images:

Old Macbook (not pro): screen resolution of 1280x800 -> 426x266.
Macbook Air (13"): Resolution 1440x900 -> 480x300.

In both cases, we'd initially render the document at a higher resolution of 560x360, which produces a better resolution than rendering the document at the lower resolution, so the image should have a better image quality. 

As for the time difference, this should actually be faster. We're only doing the scaling ONCE, to create the thumbnail at the proper size. Once we render each newtab, the images will be at the right size so there will be no scaling. The situation we have today is that each thumbnail is not sized at the newtab size, so we have to scale EACH tile on EVERY newtab load. A one time scaling operation to generate the thumbnail is ok IMHO for a fast rendering of newtab every time. Does that make sense?

Comment 68

[Mason Chang [Inactive] [:mchang]]

> As for the time difference, this should actually be faster. We're only doing
> the scaling ONCE, to create the thumbnail at the proper size. Once we render
> each newtab, the images will be at the right size so there will be no
> scaling. The situation we have today is that each thumbnail is not sized at
> the newtab size, so we have to scale EACH tile on EVERY newtab load. A one
> time scaling operation to generate the thumbnail is ok IMHO for a fast
> rendering of newtab every time. Does that make sense?

Scratch that, brain died. That's not right, it will be slower every time, but I don't have access to a non-retina display to get timings. Do you have a non-retina mac that you can try?

Comment 69

[Tim Taubert [:ttaubert] (inactive)]

Sorry, don't have a non-Retina machine either.

Comment 70

[Jim Mathies [:jimm]]

ServiceNow can sometimes supply a used system for something like this and have it to you in a short about of time.

Comment 71

[Mason Chang [Inactive] [:mchang]]

Attachment: Optimize page thumbs based on screen size

Asking for another review as I had to account for preallocated canvas' larger than the thumbnail size. Also had to incorporate scrollbar size as some platforms like Windows actually still have scrollbars with canvas whereas mac doesn't. 

I also tested this on a non-retina macbook pro, late 2011. After generating 15 tiles, 14/15 were at the proper newtab css size, and 1 was at the larger 2x size due to a lack of a window object. In this case, time spent in nsLayoutUtils::DrawImageInternal went from 51.74ms -> 19.74 ms, a 2.6x performance improvement. I think this is ok as the path to snapshot through browser-child seems to rarely happen.

Comment 72

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8665130 [details] [diff] [review]
Optimize page thumbs based on screen size

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

r=me with comments addressed

::: toolkit/components/thumbnails/PageThumbUtils.jsm
@@ +101,5 @@
> +  /***
> +   * Given a browser window, return the size of the content
> +   * minus the scroll bars.
> +   */
> +  getContentSize: function(aWindow) {

I think we can cache this value, no need to compute it every time we want to capture. OTOH it might not be very expensive to do that, would have to measure to be sure.

@@ +126,5 @@
> +
> +  /***
> +   * Given a browser window, this creates a snapshot of the content
> +   * and returns a canvas with the resulting snapshot of the content
> +   * at the thumbnail size. IT has to do this through a two step process:

Nit: It has to do ...

@@ +168,5 @@
> +    // If we've been given a large preallocated canvas, so
> +    // just render once into the destination canvas.
> +    if (aDestCanvas) {
> +      if ((aDestCanvas.width >= intermediateWidth) ||
> +          (aDestCanvas.height >= intermediateHeight)) {

Why not just a single condition? Nesting the conditional code further than needed doesn't seem useful.

Comment 73

[Mason Chang [Inactive] [:mchang]]

(In reply to Tim Taubert [:ttaubert] from comment #72)
> Comment on attachment 8665130 [details] [diff] [review]
> Optimize page thumbs based on screen size
> 
> Review of attachment 8665130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed
> 
> ::: toolkit/components/thumbnails/PageThumbUtils.jsm
> @@ +101,5 @@
> > +  /***
> > +   * Given a browser window, return the size of the content
> > +   * minus the scroll bars.
> > +   */
> > +  getContentSize: function(aWindow) {
> 
> I think we can cache this value, no need to compute it every time we want to
> capture. OTOH it might not be very expensive to do that, would have to
> measure to be sure.

We're not guaranteed that the width/height of the window will always be the same. I've seen the value diverge considerably over the course of a few snapshots. Also, we're calculating the scrollbar sizes right now, so there won't be any performance difference of snapshotting.

I fixed the other stuff.

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb71c7331ea9

Comment 75

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20696a216fbc

Comment 76

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb71c7331ea9
https://hg.mozilla.org/mozilla-central/rev/20696a216fbc

Comment 77

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 43.0a1 (2015-08-21) on ubuntu 14.04 LTS, 32 bit!

Build ID: 20150821030204
User Agent: Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0

The bug's fix is now verified on Latest Nightly 44.0a1 (2015-10-07)!

Build ID: 20151007030205
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[bugday-20151007]

Comment 78

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 43.0a1 (Build: 20150821030204)on 
windows 8.1 pro 64-bit.

Verified as fixed with Latest Firefox Beta 44.0b7 (Build ID: 20160107144911)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

As this bug is also verified on ubuntu (Comment 77),I am marking this as verified!