Page MenuHomePhabricator

Feed Picture of Day and Featured article cards using incorrect image resolution
Closed, ResolvedPublic

Description

On clean install the issue is easily noticeable on the Pic of the day cards...

On the Featured articles for the last couple days it's a bit harder to notice, but will be just as dramatic as the Pic of the day for Featured articles with better quality images.

You can still use the the current Featured article for testing if you compare the Feed image to the Article view image:

Feed image:

Article view image:

Edit: found another screenshot of the issue with Featured article...
(probably will have to view the full sized image to see the blurriness)

Event Timeline

Mhurd created this task.Oct 28 2016, 9:11 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 28 2016, 9:11 PM
JMinor triaged this task as Medium priority.Oct 28 2016, 10:33 PM
Mhurd updated the task description. (Show Details)Oct 28 2016, 10:43 PM
Mhurd updated the task description. (Show Details)
Fjalapeno claimed this task.Nov 1 2016, 4:55 PM

This is happening on the MCS

For any item in the feed that isn’t a recommendation or a nearby, we no longer have the ability to request a specific image size - the MCS returns only a single image URL / size.

I’m not sure if this is a change for the MCS (a regression where it is severing low res images), of if this is normally the size.

Investigating that now.

After finding out the cause, we can also look into reusing our existing code for web view images for creating an image URL with a larger size and do that here as well.

This does not appear to be a regression, its just the size of the image that is returned from the MCS

Going to discus with @bearND if we should up the resolution in the MCS or just let clients create their own URLs for larger sizes

@bearND @Mholloway @Dbrant

It looks as though the image resolution for featured articles and POTD in the feed are capped at a width of 320. This is half the width of images we display for iOS (640). The images now look blurry on high density/retina screens. Is it common for you to display images on android at 320?

Trying to think of the correct solution for this issue. Do we:

  1. Return an array of images for different resolutions 320, 640, etc…
  2. Bump the current image resolution to 640
  3. Leave as is and expect clients to manipulate URLs to get an appropriate size for the platform / screen size
  4. Allow clients to send an image width parameter to the service to specific the minimum size (and fragmenting the cache)

Thoughts?

It would be easy enough to bump to 640 in the service. As I understand it, rewriting URLs downward in size is safe, rewriting upward is not.

I'm not a huge fan of responding with a bunch of nearly identical URLs for different sizes but could live with it.

Like the idea of fragmenting the cache with a width param even less.

Mholloway added a comment.EditedNov 1 2016, 8:19 PM

I'll bet the change in the TFA image resolution happened when that response segment got changed to be composed in RESTBase.

Are you sure there's been a change in the POTD size, though? That should still be requested at 640:

https://github.com/wikimedia/mediawiki-services-mobileapps/blob/master/lib/feed/featured-image.js#L17-L41

(mwapi.CARD_THUMB_FEATURE_SIZE is 640, see /lib/mwapi.js)

So here's what's going on:

The thumb size provided everywhere we we use a RESTBase summary (TFA, most-read articles, news links) is set to 320 here:

https://github.com/wikimedia/restbase/blob/master/v1/summary.yaml#L121

Better loop in Services to see how they'd prefer to handle clients being able to vary this size.

@Mholloway ahh thanks for looking into it.

Yeah i think that for those specific “large” items in the feed (TFA and POTD), the 640 makes sense. For everything else the 320 is fine (and honestly maybe too big - since we show them as much smaller)

So here's what's going on:
The thumb size provided everywhere we we use a RESTBase summary (TFA, most-read articles, news links) is set to 320 here:
https://github.com/wikimedia/restbase/blob/master/v1/summary.yaml#L121
Better loop in Services to see how they'd prefer to handle clients being able to vary this size.

You're correct, the exact thump URI is used from the summary endpoint and it's 320 px.

There's been some discussion on creating an official thumb API (See T66214) that would give the clients a clear way to set the image size, but the discussion there has been stalled unfortunately.

So, we have a few options here:

  • Revive the official thumb API discussion
  • Reconstruct the URIs on the client. The thumbnail URI follows an 'unofficial' structure, so replacing 320px with 640px in the URI will allow you to fetch larger images. It's an extremely ugly option, so I'm not really proposing to use it of course
  • Make summary endpoint emit the images in several predefined sizes like thumbnail, thumbnail-xs, thumbnail-xl etc and allow clients use the appropriate one.

Option 1 is the best long-term, but option 3 could also be an acceptable short-term solution.

GWicke added a comment.EditedNov 1 2016, 8:49 PM

@Mholloway, we'd love to provide an API that officially supports changing the size in the client. See T66214 for the discussion on that. It's been slow going so far.

Without such an API, we'll always struggle to square caching with the need to accommodate random thumb sizes. Right now, the only work-around I can think of would be to add one more resolution to the default response, or to rewrite the thumb URL for a large thumb (hacky).

bearND added a project: Thumbor.EditedNov 1 2016, 8:55 PM

As an aside: It would be great to have Thumbor available for this, so we would have more flexibility on the client side. A service shouldn't have to make assumptions about a client's display dimensions.

bearND added a comment.Nov 1 2016, 9:09 PM

In the short term we should get TFA and POTD thumbnails back to 640. Currently those looks ugly on Android as well.

Maybe we could have MCS still provide the thumbnail URL and RB not override it from the merged in data if it exists?
More generically, any property which already exists in the resulting object should not be overridden.

bearND added a comment.Nov 1 2016, 9:45 PM

Maybe we could have MCS still provide the thumbnail URL and RB not override it from the merged in data if it exists?
More generically, any property which already exists in the resulting object should not be overridden.

On second thought that might go counter to our cache invalidation strategy.

  • Make summary endpoint emit the images in several predefined sizes like thumbnail, thumbnail-xs, thumbnail-xl etc and allow clients use the appropriate one.

This sounds like it would work. The drawback with this is that the apps would need to change to use the other thumbnail-* properties for the two entries, and we would have a proliferation of extra thumbnail properties we don't need in most of the cases.

How about instead we would allow a query parameter, like size=640 to the summary endpoint. Then MCS could use that for tfa and potd. The responses don't need to be stored in Cassandra since clients would not need to use this directly, only indirectly via the aggregated feed endpoint (for TFA and POTD). This changes of course if clients would start to use it. Maybe we could somehow hide this query parameter from being used outside the datacenter?

How about instead we would allow a query parameter, like size=640 to the summary endpoint. Then MCS could use that for tfa and potd. The responses don't need to be stored in Cassandra since clients would not need to use this directly, only indirectly via the aggregated feed endpoint (for TFA and POTD). This changes of course if clients would start to use it. Maybe we could somehow hide this query parameter from being used outside the datacenter?

That would actually slow down the feed endpoint significantly. Now we're fetching the summaries from Varnish, so they share cached entries with normal requests for summaries. Since we've switched to this approach, the latency of the feed decreased to stable 200ms, so sharing the cached entries works quite well. Since these additional summaries with the size parameter will not be purged, we can't really cache them at all, so we will get a severe penalty on latency.

bearND added a comment.Nov 1 2016, 9:55 PM

Fair enough. Would it be expensive to cache these as well?

@bearND It's not about caching, it's about purging. The purge load is already quite high, and purging these in addition to the normal summary would double the amount of purges for this endpoint. And in practice only a tine portion of those additional purges would be required since only a tiny portion of articles will be cached with the size parameter. When we get XKey it's a different story, but right now we don't have it yet.

bearND added a comment.Nov 2 2016, 3:12 AM

Ok, how do we proceed then? If anyone has a better idea I'd like to read it.

That would actually slow down the feed endpoint significantly. Now we're fetching the summaries from Varnish, so they share cached entries with normal requests for summaries.

Let's backtrack a bit. This is only for the first request every 5 minutes. Maybe we can live with that until Thumbor (I assume it's the same as T66214) is live?

@bearND Actually, there's another solution, even more hacky but it would have no bad consequences except a giant TODO in RESTBase code and our urgent need to proceed on T66214:

  • Reconstruct the URIs in RESTBase for the featured image piece. Effectively that would give you the 640px image and avoid and loss on latency.

Although I really don't like that because it's a dirty hack, it seems like the most easy short-term solution

bearND added subscribers: Tgr, brion.Nov 2 2016, 5:16 AM

@Pchelolo Adjusting the URL to increase the size can be problematic. Downwards adjustments tend to be less of an issue. The apps have run into issues where the mangling the URL to use a larger thumbnail than the original size caused 400s. See T118978: Some lead image URLs get 400 response. The solution was to ask MW API for the largest image we need and then adjust downwards from there: https://gerrit.wikimedia.org/r/#/c/254105/2/lib/mwapi.js

I don't see how we would fit this approach into our current system where we have the summary responses already exposed publicly. If we increased the width for all summary responses then suddenly most thumbnails would be much bigger than before and much bigger than needed.

I think @bearND's original proposal of MCS sending RB the title and thumbnail (if it differs from the one in summary) looks like the most sensible one for the time being. This wouldn't mess with caching, but it would influence the latency of the aggregated feed (as MCS would need more time to return the result).

I think @bearND's original proposal of MCS sending RB the title and thumbnail (if it differs from the one in summary) looks like the most sensible one for the time being. This wouldn't mess with caching, but it would influence the latency of the aggregated feed (as MCS would need more time to return the result).

It would mess with caching, because the URI of the article of the day image would be controlled by MCS now and won't be purged. So, if someone vandalises the article's of the day picture it would be served much much longer (up to 1 hour) since it would be fetched from cassandra in RESTBase

GWicke added a comment.EditedNov 2 2016, 3:13 PM

The issue with client-side scaling is that the math needed to calculate the resulting dimensions becomes very imprecise if all you have is the dimensions of a very small thumb. If we provided original image dimensions as well (x, y) (or a precise aspect ratio), then calculating the exact thumb dimensions after client-side size selection would become feasible no matter which thumb URL is provided by default. We'll need this information even in the new thumb API.

So, perhaps we could move towards client-side size selection iteratively:

  1. Provide original image dimensions along with a default thumb URL & thumb dimensions. Re-scale as needed client-side, by (hacky, for now) rewriting the thumb URL.
  2. Introduce cleaner thumb URLs that reduce the rewriting part in 1) to a simple append of the width.

It would mess with caching, because the URI of the article of the day image would be controlled by MCS now and won't be purged. So, if someone vandalises the article's of the day picture it would be served much much longer (up to 1 hour) since it would be fetched from cassandra in RESTBase

If you could share the exact command line I could use to force a rerender and purge of the aggregated feed for "today" then that wouldn't be so bad. Extra points if anyone else here could do it as well.

We'll need this information even in the new thumb API.

Currently the apps don't really use the thumbnail or original image dimensions. I agree that it would be useful to know the dimensions of the original image if the clients would have to impose an upper bound for the thumbnail to be requested -- if the clients would want to manipulate the thumbnail URLs directly.

It would mess with caching, because the URI of the article of the day image would be controlled by MCS now and won't be purged. So, if someone vandalises the article's of the day picture it would be served much much longer (up to 1 hour) since it would be fetched from cassandra in RESTBase

If you could share the exact command line I could use to force a rerender and purge of the aggregated feed for "today" then that wouldn't be so bad. Extra points if anyone else here could do it as well.

Hm... Actually feed endpoint doesn't support no-cache yet. I've created T149857 to tackle this, will do after I finish currently pending stuff.

Currently the apps don't really use the thumbnail or original image dimensions.

One exception: in the Android app we do use the original image dimensions to check against a defined minimum size (64x64) before showing in the gallery.

bearND added a comment.Nov 3 2016, 2:40 AM

Right. I should have qualified that with "in the explore feed".

Anyways, the task I've created doens't answer the question we have here: what to do before we have a good public API for thumbnailing. (BTW, @bearND @Mholloway I have a question on T149857 I would really appreciate your input on)

To sum up the discussion, here's the options we have:

  1. Enrich the summary with thumbnail_xs, thumbnail_xl etc properties. Pros: simple, doesn't affect caching Cons: ugly API forever, increased payload to send to clients
  2. Do hacky URI rewriting in RESTBase. Pros: good for caching Cons: Hacky, Proved to be error prone
  3. Make MSC return the thumbnail and not override it in RESTBase. Pros: Simple. Cons: Might require manual content purging

I might've forgotten other discussed and not completely ruled out possibilities, but it seems we have those 3 options to choose from. Ideas?

I'd add

  1. Send a single, large thumbnail (the largest any platform requires, currently 640) and require clients to do (hacky) downward rewriting if desired.

This was the idea originally[1], before we moved to feed composition in RESTBase. The reason for not sending a set of thumbnails of various sizes, as we do in the mobile-sections endpoints, was that page summaries typically appear in the feed something like 50+ times (once for the TFA, a few times for the news, and 50ish for the most-read articles), so we'd be sending a *lot* of very similar thumb URLs.

It's not an ideal solution but seemed to be workable. Pros: simple, good for caching; Cons: Hacky, possibly error-prone. Basically same as (2) but with the risk on the client instead of the server.

As for the other options, I'd favor (2) or (3) over (1).

[1] https://gerrit.wikimedia.org/r/#/c/294399/

Mholloway added a comment.EditedNov 3 2016, 5:17 PM

I'm going to argue against my own point: the difference is that before, we were only requesting 640px for the POTD (which we still are and doesn't use the summary endpoint and isn't relevant here) and the TFA thumb. Changing universally to 640 in the RB summary request definition would change them to 640 for everything. If rewriting on the client goes wrong for whatever reason, that's a lot of extra data used when the images are downloaded.

So I'm personally leaning more toward (1) either having RB do the rewriting just for the TFA thumb URL (Petr's option (2)), or (2) have MCS separately get the thumb URL it wants and send it along with the featured article title for which RB will merge in a summary.

So in practice with the latter, from the MCS /page/featured endpoint, we'd respond with, e.g.,

{
  "title": "Norse-American_medal",
  "thumbnail": {
    "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/b/bb/Norse_American_Centennial_Sloop_1925_Issue-2c.jpg/640px-Norse_American_Centennial_Sloop_1925_Issue-2c.jpg",
    "width": 640,
    "height": 572
  },
},

And then RB could merge in the page summary for our title but leave the thumbnail property alone? Is that correct?

bearND added a comment.Nov 3 2016, 8:49 PM

I actually prefer option 4 now. It looks like a better thumbnail API is still far off. A service should not have to make too many fine-grained assumptions about the resolution a client wants to display images in.
We just need to give the clients, esp. the Android app some time to add code to make the downward adjustments for the various places we use the summary data for and 640px is too big: link previews, various feed portions. For the feed I'm thinking that even for 'In the news' we might want a larger thumbnail when the user drill down to a news story. So, the only place in the feed we would want to adjust resolution downwards from 640px to 320px is the trending portion.

The clients would need to ensure that the adjustment is only downward, not upward, due to T118978:

  • The Android code base already has an ImageUrlUtil class which does that. Currently, it's only used for news items. We would need to use it for link previews and the trending feed card.
  • iOS would want to implement something similar as Android.
  • Sounds like the web team is not using the summary endpoint yet. They just need to be aware of the change for their upcoming Hovercards feature.

Create a PR for RESTBase to bump the size to 640px: https://github.com/wikimedia/restbase/pull/706

Deployment would require broader testing.

Pchelolo claimed this task.Nov 4 2016, 9:22 PM
Pchelolo edited projects, added Services (doing); removed Services (watching).
Pchelolo added a subscriber: Fjalapeno.
Tgr added a comment.Nov 4 2016, 10:13 PM

Size rewriting is safe as long as the size is at least one full pixel smaller (in width) than the original image size. One option is to send

  • the original dimensions
  • the full-sized URL
  • a random thumbnail (say, half the original size)

The client can then calculate needed width (client height * original width / original height, rounded) and if it's smaller than the original width, rewrite the thumb URL, otherwise use the full URL.
(Or you can omit the full URL if you don't mind the small additional quality loss of scaling down 1px on server side before scaling up on client side when the original is smaller than or equal to the requested size.)

bearND added a subscriber: phuedx.Nov 8 2016, 4:38 PM

@phuedx Can you confirm my assumption is correct?

  • Sounds like the web team is not using the summary endpoint yet. They just need to be aware of the change for their upcoming Hovercards feature.

This would mean that you would have to modify the thumbnail URI to reduce the thumbnail width on your end to 320px or lower since RB could return thumbnail URLs of up to 640px width. Is that ok with you?

@JoeWalsh Great. So iOS already has code in place to handle an increase in the default thumbnail size. Is this already used for thumbnails in link previews and Explore feed cards?

So, then there's the Android app that would need to be updated to be able to deploy the medium-term solution proposed in T150062. This still would take at least several weeks to rollout since we would probably want to give a couple of weeks after Android released to production. (Probably somewhere in the range of mid December to mid January.)

What's the short-term solution for this? We are looking for a solution that would not require any changes on the app side.

From the options summarized by @Pchelolo in T149450#2767184

  1. Enrich the summary with thumbnail_xs, thumbnail_xl etc properties. Pros: simple, doesn't affect caching Cons: ugly API forever, increased payload to send to clients

1b. I would like to add another option which is a variation of #1. Enrich the summary with a thumbnail_l property for the 640px case but consider it "private". Then the aggregated feed endpoint in RB would use thumbnail_l instead of thumbnail for TFA and POTD. Ideally, we wouldn't expose thumbnail_l publicly. The idea is that apps should not use the thumbnail_l variant directly because it's a temporary hack we'd like to get rid of in the future.

  1. Do hacky URI rewriting in RESTBase. Pros: good for caching Cons: Hacky, Proved to be error prone

For this to be feasible the service would need to know the dimensions of the original image, and basically implement what iOS did in the links @JoeWalsh provided above. Since we don't have that that would require RB to make an action API request or change the summary endpoint to include the original image dimensios as well.

  1. Make MSC return the thumbnail and not override it in RESTBase. Pros: Simple. Cons: Might require manual content purging

I like that this is simple to do. With T149857 already being deployed I supposed this seems like a decent candidate to me. @Pchelolo Is it possible for me and documented how to purge content when the need arises?

@JoeWalsh do we need to know the original size of the image to safely rewrite images to 640? We can alter the response to return that.

(I know for POTD we can safely assume it is 640, but not for TFA)

@Fjalapeno we do need to know the size of the image to safely re-write - any size equal to or larger than the actual size will fail out.

@Fjalapeno we do need to know the size of the image to safely re-write - any size equal to or larger than the actual size will fail out.

@bearND given this, would you like to add these sizes to the response as a stop gap so clients can safely request what they need?

Gilles added a subscriber: Gilles.Nov 9 2016, 6:36 PM

@bearND given this, would you like to add these sizes to the response as a stop gap so clients can safely request what they need?

The thumbnail object already has a width and height attribute for the dimensions of the thumbnail. I guess we could add an owidth and oheight or similar. (original_width and original_height?) Or should we create an image object one level higher with width and height?

@phuedx Can you confirm my assumption is correct?

Your assumption is correct.

This would mean that you would have to modify the thumbnail URI to reduce the thumbnail width on your end to 320px or lower since RB could return thumbnail URLs of up to 640px width. Is that ok with you?

Sure. IIRC we have code in MobileFrontend that does this in another context.

The Picture of the day has been fixed starting from November 17, older content will be updated after the next RESTBase deploy.

The Article of the day issue has been addressed with https://github.com/wikimedia/restbase/pull/715 - that's a temporary hack until the proper thumb API is designed.

The temp hack has been deployed.

Mholloway added a comment.EditedDec 2 2016, 4:26 PM

A new task for the proposal to include canonical image info in the restbase summary along with the thumb info is here:

T152163: Update RESTBase page summary endpoint to return canonical "image" info for each "thumbnail"

I'm not sure why are we keeping this task open? What's left to be done here?

Gilles moved this task from Backlog to Radar on the Thumbor board.Dec 6 2016, 2:16 PM
Fjalapeno closed this task as Resolved.Dec 6 2016, 7:01 PM

I think we can close this - if anyone one wants to re-open feel free