Page MenuHomePhabricator

Page preview shows icon instead of thumbnail
Closed, ResolvedPublic

Description

Page previews for some pages show a pixelated icon instead of the correct thumbnail.

Screen Shot 2018-02-21 at 3.36.07 PM.png (371×926 px, 207 KB)

First noticed on cswiki in T182321#3990934. I can also reproduce the same on a few enwiki pages. From the Recent Changes page:

https://en.wikipedia.org/wiki/Special:RecentChanges?hidebots=1&hidecategorization=1&hideWikibase=1&limit=50&days=7&damaging__likelybad_color=c4&damaging__verylikelybad_color=c5&urlversion=2
Bad thumbnail:

Strangely enough, it's not all of them. Here are some from RC that show the correct thumbnail for me:

My best explanation at this time for the ones showing the correct thumbnail is probably caching.

Event Timeline

E.g., when hovering over Timeline_of_the_far_future. The summary has the thumbnail
https://upload.wikimedia.org/wikipedia/commons/a/aa/Red_Giant_Earth_warm.jpg, which would have worked.
Instead it tries to download https://upload.wikimedia.org/wikipedia/commons/a/aa/320px-d_Giant_Earth_warm.jpg, which gets a 404.
Note the s/Red/320px-d/.

These images are coming back with a 404. When I visit an image (https://upload.wikimedia.org/wikipedia/commons/b/b2/228px-ristopherSinclair.jpg):

404 Not Found

The resource could not be found.

 File not found: /v1/AUTH_mw/wikipedia-commons-local-public.b2/b/b2/228px-ristopherSinclair.jpg

Possibly T124101.

Bad thumbnail:

Strangely enough, it's not all of them. Here are some from RC that show the correct thumbnail for me:

What the bad thumbnail list has in common is that the 'thumbnail' and 'originalimage' source properties both point to the same URL, for the original file. (This is expected in many cases.) The good list has different URLs, since a true thumbnail was found.

Is there client code assuming that the thumb file name will begin with 'nnnpx-' and swapping in a leading 'nnnpx-' segment in all cases?

Yes, it's in Popups repo: rest.js lines 117ff. (generateThumbnailData):

parts[ parts.length - 1 ] = width + 'px-' + filename;

Update:

lastPart.substr( lastPart.indexOf( 'px-' ) + 3 )

returns d_Giant_Earth_warm.jpg since lastPart.indexOf( 'px-' ) returns -1.

I think it should check first if the indexOf returns > -1 before considering changing the filename.

There's something I don't think we considered explicitly at work here as well, relating to thumb sizes. When we get metadata (inc. the page thumbnail) from the action API, we request a thumbnail at a size of 1024px. For mobile-sections, we then use the resulting URL to construct a set of smaller thumb URLs, if a thumb and not the original was returned.

In cases like https://commons.wikimedia.org/wiki/File:ChristopherSinclair.jpg we just get the original URL, since the original is smaller than the requested width.

In the summary endpoint, we're not doing any rewriting or thumbnail set creation, just passing through the thumb URL the MW API gives us. We could quickly make this scenario less likely to occur by requesting a smaller thumb size from the MW API, making it more likely a thumb URL will in fact be returned. I don't know that we ever decided explicitly what size the 'thumbnail' URL should be requested at. Maybe something like 320px or 480px makes more sense.

The client code still needs fixing, of course.

We might also consider filing a task to make the action API always return a true thumbnail URL, even if the requested size is larger than the original and a "thumbnail" of identical size to the original is returned. Better check to see if that's been considered. (Also, would not be a quick fix.)

Change 413276 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Allow mwapi.getMetadata callers to request a specific thumb size

https://gerrit.wikimedia.org/r/413276

Change 413276 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Allow mwapi.getMetadata callers to request a specific thumb size

https://gerrit.wikimedia.org/r/413276

We should change both the server and the client side.

  • On the server side, MCS should request smaller thumbnails for the summaries. The MCS patch is up ^ and will be deployed shortly. Once that is deployed @Pchelolo is going to run a dump of summaries over enwiki and cswiki (hopefully in parallel).
  • The client should remove the assumption of having px- in the thumbnail URL.

Change 413282 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Fix: don't assume thumbnail URLs contain pixel size

https://gerrit.wikimedia.org/r/413282

The client should remove the assumption of having px- in the thumbnail URL.

This is fixed in https://gerrit.wikimedia.org/r/413282.

For awareness, there were reports by users on the help desk and the VP/T of this issue.

If i recall correctly page previews does something to rewrite the thumbnail url to match a certain thumbnail size. Did anything change in the new summary spec relating to the thumbnail field...?

Whoops read the accompanying email. Looks like you did indeed work this out.

@Jdlrobson Yep. The change was that the new summary was requesting a larger thumbnail image (for reasons unbeknownst to me), which resulted in the original image URL being returned for the thumbnail field as well. That one doesn't have the px- piece in the filename.

@TheDJ Thanks for adding the links.

We never really gave much weight to the treatment of thumbnails in the new service and perhaps we should've. Of course, this needs to be fixed for now (likely with a SWAT), but, ideally, there shouldn't be special handling for thumbnails in the client.

IIRC we request a different thumbnail size depending on the pixel density of the client, which bracketed using $.devicePixelDensity. We'll request a thumbnail with its larger dimension being one of: 320px, 480px, or 640px.

I guess that we should figure out whether this is actually necessary, i.e. could we only request a thumbnail at 640px? If it is, then can the service deliver a set of thumbnail URLs with meaningful names, e.g.

{
  "thumbnails": {
    "small": "...",
    "medium": "…",
    "large": "..."
  }
}

so that the client maps device pixel ratio to a name instead of manipulating strings.

ovasileva raised the priority of this task from High to Unbreak Now!.Feb 22 2018, 10:32 AM

For awareness, there were reports by users on the help desk and the VP/T of this issue.

@TheDJ - thanks for noting! Do you have links to the reports? We'd like to post a quick update today

I guess that we should figure out whether this is actually necessary, i.e. could we only request a thumbnail at 640px? If it is, then can the service deliver a set of thumbnail URLs with meaningful names <snip /> so that the client maps device pixel ratio to a name instead of manipulating strings.

This is tracked in T66214: Define an official thumb API.

Change 413282 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Fix: don't assume thumbnail URLs contain pixel size

https://gerrit.wikimedia.org/r/413282

Went through a couple of recent changes pages (30-40 articles?) and all looks good. Is there any other way to reliably QA this?

ovasileva claimed this task.

Resolving for now!

This is still happening on some wikis

Screen Shot 2018-02-28 at 4.26.36 PM.png (472×405 px, 107 KB)

Screen Shot 2018-02-28 at 4.34.19 PM.png (461×402 px, 106 KB)

The fix was only merged Thursday. It has not been deployed yet.

I see it tagged with WMF-deploy-2018-02-27 (1.31.0-wmf.23). Doesn't that mean it should be available on all wikis this Thursday?

Yes, it's scheduled to roll out to the Group 2 wikis (inc. most Wikipedias) tomorrow. There are four SWAT windows before then, including one in about 25 minutes. You could probably get in on one.

Edit: sources:
https://wikitech.wikimedia.org/wiki/Deployments
https://wikitech.wikimedia.org/wiki/Deployments/One_week

I think we can wait another two days. I was just a bit confused because I thought we had planned to SWAT deploy this initially.

Train is delayed - let's SWAT this? Moving to to-do

Change 415564 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@wmf/1.31.0-wmf.22] Fix: don't assume thumbnail URLs contain pixel size

https://gerrit.wikimedia.org/r/415564

Change 415564 merged by jenkins-bot:
[mediawiki/extensions/Popups@wmf/1.31.0-wmf.22] Fix: don't assume thumbnail URLs contain pixel size

https://gerrit.wikimedia.org/r/415564

Mentioned in SAL (#wikimedia-operations) [2018-03-01T15:18:20Z] <zfilipin@tin> Synchronized php-1.31.0-wmf.22/extensions/Popups: SWAT: [[gerrit:415564|Fix: dont assume thumbnail URLs contain pixel size (T187955)]] (duration: 01m 14s)