Page MenuHomePhabricator

[Analysis] Image size too large on previews
Closed, ResolvedPublic

Description

Background

Noticed during deployment of T187955: Page preview shows icon instead of thumbnail

Steps to reproduce

  1. Go to https://el.wikipedia.org/wiki/%CE%93%CF%81%CE%B1%CE%BC%CE%BC%CE%B9%CE%BA%CE%AE_%CE%AC%CE%BB%CE%B3%CE%B5%CE%B2%CF%81%CE%B1
  2. Hover over μαθηματικών

Observed behavior
Preview image is too large

Event Timeline

ovasileva created this task.Mar 1 2018, 4:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 1 2018, 4:47 PM
ovasileva triaged this task as High priority.Mar 1 2018, 4:48 PM
bearND added subscribers: Pchelolo, mobrovac, bearND.

Looks like someone purged or edited this since you made the screenshot. That particular page looks fine now.
Here's the summary: https://el.wikipedia.org/api/rest_v1/page/summary/%CE%9C%CE%B1%CE%B8%CE%B7%CE%BC%CE%B1%CF%84%CE%B9%CE%BA%CE%AC

"thumbnail": {
  "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/2/21/Euclid.jpg/320px-Euclid.jpg",
  "width": 320,
  "height": 268
}, "originalimage": {
  "source": "https://upload.wikimedia.org/wikipedia/commons/2/21/Euclid.jpg",
  "width": 806,
  "height": 675
},

I think the issue on that page is related to T187955 and was when it was on summary version 1.3.0 because with 1.3.0 it would provide a thumbnail URL that cannot be resized if the original image is < 1024px. With 1.3.0 it returned the same URL for thumbmail as the originalimage.

@Pchelolo @mobrovac
I think we need to enable the ensure_content_type_filter to at least 1.3.1 (or even better a later version) sooner rather than later. We're going to deploy 1.3.3 during today's deployment window in about an hour.
Note that between since 1.3.0 any updates have been incremental improvements. As I said earlier we should at least get it to 1.3.1 after MCS is deployed, 1.3.2 would be better. And if you feel the cluster can handle it 1.3.3.

As I said earlier we should at least get it to 1.3.1 after MCS is deployed, 1.3.2 would be better. And if you feel the cluster can handle it 1.3.3.

1.3.3 might be a bit too drastic - it's not about cluster load, it's more about client latencies. The cluster load as well of course.

We did most of the dumps on 1.3.1, so in order to avoid high client latencies I think we should go to 1.3.1

bearND added a comment.Mar 1 2018, 5:59 PM

@Pchelolo sounds good to me. I'll ping you on IRC once the MCS deploy is done.

For the record and clarity, enabling the ensure_content_type filter and requiring v1.3.1 means that all of the content for which we haven't run dumps will end up being v1.3.3.

bearND added a comment.Mar 1 2018, 6:15 PM

@mobrovac That's good and i think that will help us later once we increase the version number in the filter to 1.3.3.

@mobrovac That's good and i think that will help us later once we increase the version number in the filter to 1.3.3.

How so? Once the version in the filter is changed, everything will have to be re-rendered again, which is partially why I'm hesitant to go with 1.3.1 now in the first place, and one buggy instance on one page which we know has been fixed doesn't sound too convincing, TBH.

bearND added a comment.Mar 1 2018, 6:21 PM

Why would a page already on 1.3.3 have to be rerendered again if we set the filter to 1.3.3?

ovasileva renamed this task from Image size too large on previews to [Analysis] Image size too large on previews.Mar 6 2018, 5:12 PM

tagging with sprint board for analysis

phuedx added a subscriber: phuedx.Mar 6 2018, 5:26 PM

During the Web Team Task Grooming ritual, I proposed that the Page Summary API should return three images: an image at 320px, an image at 640px, and the original. The client should only be responsible for deciding which image is the most appropriate to display to the user and requesting it, not deriving what the image's URI is – specialised knowledge of our image serving infrastructure crossing an API boundary.

For the Wikimedia setup, we can rely on the fact that gzip will minimise the number of bytes sent to/received by the client as the majority of image these image URIs will contain (large) repeated fragments. For the third-party MediaWiki setup, we already rely on the PageImages API to return the most appropriate image for the size that we request.

Jdlrobson moved this task from Inbox to Next up on the User-Jdlrobson board.Mar 6 2018, 5:56 PM

I purged μαθηματικών and it's now showing correctly. Are there any other examples where this is occurring and caching is not responsible?

Do we know how long till we can assume all pages on all Wikipedia's as served by REST are using the new endpoint (e.g. not cached)?

bearND added a comment.Mar 7 2018, 2:36 AM

@Jdlrobson What matters more than caching is that the results are stored in Cassandra indefinitely, until a rerender gets triggered. That usually happens when the corresponding page or another page(template) included in that page gets edited. Exceptions are
(a) when Services runs a dump, which forces a rerender of all pages of a particular wiki or
(b) when a page gets retrieved while the ensure_content_type(version)_filter is enabled.
As far as dumps for summary 1.3.1 or higher. cs, en, fr, de, and es Wikipedias are done IIRC.
AFAIK cswiki dump for 1.3.4 (the latest) is complete, the one for enwiki is in progress.

Not sure when the ensure_content_type_filter gets enabled to catch other wikis. I believe it would require the summary endpoint to be considered stable enough as to not cause too much churn.

bearND added a comment.EditedMar 7 2018, 5:39 PM

@phuedx Keep in mind the more we change this endpoint the longer the roll-out can be delayed. We currently have

"thumbnail": {
  "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/0/0b/Cat_poster_1.jpg/320px-Cat_poster_1.jpg",
  "width": 320,
  "height": 210
},
"originalimage": {
  "source": "https://upload.wikimedia.org/wikipedia/commons/0/0b/Cat_poster_1.jpg",
  "width": 5935,
  "height": 3898
},

That's another use case for T66214: Define an official thumb API.

I see two options in the meantime:
(a) Keep as is and have the client change the URI as we did before (considering the originalimage.width as an exclusive upper bound). I know this is ugly but you can get that right away.
(b) Add another object property for a 640px sized thumbnail. What would you like to call the new thumbnail size? If we go that route this will take probably several weeks before this update is rolled out to all summaries.

What's the envisioned timeline for this?

@ovasileva @phuedx would you be okay with dealing with these on a case by case basis? The fix is simple and we can educate users to help address this issue (ie. go to page and action=purge) when encountered. I'd really prefer not to add more complexity to the client for something that can be easily rectified without our intervention. We want edits to care about how these look (both in the thumbnail and summary sense, right?)

That's another use case for T66214: Define an official thumb API.

Agreed. Given @Jdlrobson's comment above, I see no issue with waiting.

I see two options in the meantime:
(a) Keep as is and have the client change the URI as we did before (considering the originalimage.width as an exclusive upper bound). I know this is ugly but you can get that right away.

This is what we currently do.

I'd really prefer not to add more complexity to the client for something that can be easily rectified without our intervention.

☝️ There's already a lot of complexity in the codebase to deal with sizing thumbnails appropriately. By making the Page Summary service request thumbnails that we definitely want to and can use we can remove it!

@ovasileva @Jdlrobson finally got around to triaging this on the RI board.

For background:

We had a lot of discussion on this topic about a year or so ago. Essentially we had both iOS and Android looking for 2 different thumbnail sizes depending on the device and the context. At this point we only had the thumb size of 320. And iOS wanted a 640.

Requesting a different size was problematic for a number of reasons. For one, the rules for naming were inconsistent which makes it difficult, though not unmanageable to rename. The bigger issue was that there was no reliable way to "upsize" your image if you didn't know the size of the original. At the time, if you guessed a size that was bigger than the original you won't get anything back.

We discussed adding the 640 res for retina screens, but worried that this could ballon in to adding many different sizes to the summary over time. The compromise was that instead of providing specific sizes we instead provide the thumb and the original. With the plan being that clients who want a different size can reliably request a different size if they know the original. The down side is that each client must re-implement the naming rules.

However, this was always a stop gap solution. The real solution is T66214: Define an official thumb API

This has been on the back-burner while we worked on Reading Lists and PCS, but we were already opening this back up again later this quarter. Given that this issue is surfacing for you, the timing now seems right.

A few questions:

  1. Does the Thumb API proposal linked to above work for you as a solution?
  2. Are you able to either scale the image to fit OR use the original to request the size you need until that is implemented?
  3. If you are unable to wait, would you like to help get the Thumb API proposal implemented?

Thanks!

Jdlrobson closed this task as Resolved.Apr 19 2018, 4:48 PM

Per https://phabricator.wikimedia.org/T188636#4014538 I think the original problem here was the caching issue related to the roll out. This is resolved.

I think @phuedx was suggesting reducing the complexity in our codebase so I think we can wait for T66214 to do that. I've spun out T192571 to capture that.