Closed Bug 1531933 Opened 8 months ago Closed 6 months ago

Optimize image sizes & lazy load

Categories

(Firefox :: New Tab Page, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 68
Iteration:
68.2 - Apr 1 - 14
Tracking Status
firefox68 --- fixed

People

(Reporter: gsuntop, Assigned: gsuntop)

References

(Depends on 1 open bug)

Details

(Keywords: github-merged)

Attachments

(2 files)

Pull smaller image sizes where appropriate. Currently we are pulling the same image size for all components regardless of display dimension. This may involve on the fly rewriting of the resize parameter for image URLs.

Blocks: 1534775
Priority: P2 → P1
Blocks: 1535749
Assignee: nobody → gsuntop
Severity: normal → enhancement
No longer blocks: 1512725

@Mathijs: Does our image CDN give other options beside sizing? Does it let us control image formats?

Is there documentation of the parameters somewhere?

Blocks: 1512725
No longer blocks: pocket-newtab-68
Flags: needinfo?(mathijs)

Per Daniel: "The image cache uses thumbor under the hood, you can use all the parameters that are available here: https://thumbor.readthedocs.io/en/latest/usage.html the old /direct way works too"

Flags: needinfo?(mathijs)
See Also: → 1535693
Depends on: 1538019
No longer blocks: 1512725
Iteration: 68.1 - Mar 18 - 31 → 68.2 - Apr 1 - 14
Depends on: 1542034
Summary: Use smaller image sizes where applicable → Use proper image sizes

Currently all images are 450px. Upcoming PR will request 1x or 2x image of appropriate size based on rendered width and screen density.

Blocks: 1543756
Keywords: github-merged
Summary: Use proper image sizes → Optimize image sizes & lazy load
Duplicate of this bug: 1535749
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

@gvn, I've noticed two things that need a bit of discussion before I can mark this issue as verified.

First one: Are we ok with some of the images looking blurry in some cases? Is seems as though the list with images sections that are among the first to load have really blurry images, but if I scroll down the page I can see the same images but they are clearer.

Image 1 (left is latest nightly, right is older nightly before the patch)
Image 2 (same as above)

Second one: It looks like some images are loaded pretty late and you can catch a glimpse of a blank space that loads and image. Is this behavior something that we want to keep?

Link to screen recording

Flags: needinfo?(gsuntop)

First one: Are we ok with some of the images looking blurry in some cases? Is seems as though the list with images sections that are among the first to load have really blurry images, but if I scroll down the page I can see the same images but they are clearer.

Could you grab the image source when you see this happening? I haven't noticed this behavior and that would help me investigate. Also, are you using a high density display? If you refresh after about 10-20 seconds does the image sharpen? There are cases I've seen where if you are the first person to hit an optimized image route it times out due to server processing time. In that case it will fall back to a non optimized image, but subsequent views should be fine. The good news is this only will ever happen to one person and only one time.

Second one: It looks like some images are loaded pretty late and you can catch a glimpse of a blank space that loads and image. Is this behavior something that we want to keep?

This is expected behavior for lazy loaded images. We are loading images as they scroll into the viewport to improve initial loading performance. This also prevents network requests for images that are never scrolled into view.

Flags: needinfo?(gsuntop)

It's easily reproducible (on dev-test-all at least) on a new profile, but I've also managed to reproduce on my regular long standing profile. It looks like it happens more on new images that need to be loaded.

This is the source of the image:
<img src="https://img-getpocket.cdn.mozilla.net/80x0/filters:format(jpeg):quality(60):no_upscale():strip_exif()/https%3A%2F%2Fstatic01.nyt.com%2Fimages%2F2019%2F04%2F18%2Fstyle%2F15astrologybiz-1%2F15astrologybiz-1-facebookJumbo.jpg" srcset="https://img-getpocket.cdn.mozilla.net/160x0/filters:format(jpeg):quality(60):no_upscale():strip_exif()/https%3A%2F%2Fstatic01.nyt.com%2Fimages%2F2019%2F04%2F18%2Fstyle%2F15astrologybiz-1%2F15astrologybiz-1-facebookJumbo.jpg 2x">

I'm using a regular 1080p display. The images look a lot better on a retina display tbh, even though they have the same image source.

Refreshing doesn't seem to do anything in my case, and sometimes it looks like an image that looked good will load blurry. Looking at the source more closely, it seems that when the image looks good the source is 280x146 and when it's blurry the source is 80x42.

@gvn, it seems like this got more complicated, do we want to move this into a different bug and track it there?

Flags: needinfo?(gsuntop)

One thing to bear in mind is that srcset is being used to conditionally load in a 2x resolution image for high pixel density (aka Retina) screens. Therefore on the 1080p you'll likely get a lower res image by design (I'm not sure what size the screen is, which is also needed to determine pixel density).

I'm fine keeping this issue within this bug, but if you'd like to open a new one that is also fine with me.

I'll do some more research on this today...

Flags: needinfo?(gsuntop)

I realized that the strategy I was using was a bit too aggressive in some cases and have created a PR that should improve image quality. Some aspect ratios were getting pulled at too small of a resolution before.

Reopening the bug as the new PR will be exported in a different bug.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Looks like the original landing caused bug 1545455. Generally additional work happens in new bugs otherwise tracking gets tricky.

Regressions: 1545455

Ok, creating a new bug: 1545503

Blocks: 1545503
Status: REOPENED → RESOLVED
Closed: 7 months ago6 months ago
Resolution: --- → FIXED
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.