Page MenuHomePhabricator

Do not show a broken thumbnail for small video thumbnails
Closed, ResolvedPublic5 Estimated Story Points

Description

(Spun out of T92457)
@Stephen points out the "originalimage" field of a page preview could be a video e.g. https://en.wikipedia.org/api/rest_v1/page/summary/Completing_the_square

{
  "type": "standard",
  "title": "Completing the square",
  "displaytitle": "Completing the square",
  "namespace": {
    "id": 0,
    "text": ""
  },
  "titles": {
    "canonical": "Completing_the_square",
    "normalized": "Completing the square",
    "display": "Completing the square"
  },
  "pageid": 303500,
  "thumbnail": {
    "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/3/3d/Completing_the_square.ogv/320px--Completing_the_square.ogv.jpg",
    "width": 320,
    "height": 240
  },
  "originalimage": {
    "source": "https://upload.wikimedia.org/wikipedia/commons/3/3d/Completing_the_square.ogv",
    "width": 640,
    "height": 480
  },
  "lang": "en",
  "dir": "ltr",
  "revision": "836342689",
  "tid": "0f82bedc-3fa5-11e8-8178-ff9ff9b54684",
  "timestamp": "2018-04-14T05:31:14Z",
  "description": "technique used to solve a quadratic equation.",
  "content_urls": {
    "desktop": {
      "page": "https://en.wikipedia.org/wiki/Completing_the_square",
      "revisions": "https://en.wikipedia.org/wiki/Completing_the_square?action=history",
      "edit": "https://en.wikipedia.org/wiki/Completing_the_square?action=edit",
      "talk": "https://en.wikipedia.org/wiki/Talk:Completing_the_square"
    },
    "mobile": {
      "page": "https://en.m.wikipedia.org/wiki/Completing_the_square",
      "revisions": "https://en.m.wikipedia.org/wiki/Special:History/Completing_the_square",
      "edit": "https://en.m.wikipedia.org/wiki/Completing_the_square?action=edit",
      "talk": "https://en.m.wikipedia.org/wiki/Talk:Completing_the_square"
    }
  },
  "api_urls": {
    "summary": "https://en.wikipedia.org/api/rest_v1/page/summary/Completing_the_square",
    "edit_html": "https://en.wikipedia.org/api/rest_v1/page/html/Completing_the_square",
    "talk_page_html": "https://en.wikipedia.org/api/rest_v1/page/html/Talk:Completing_the_square"
  },
  "extract": "In elementary algebra, completing the square is a technique for converting a quadratic polynomial of the form",
  "extract_html": "<p>In elementary algebra, <b>completing the square</b> is a technique for converting a quadratic polynomial of the form</p><dl><dd><span class=\"mwe-math-element\"><img src=\"https://wikimedia.org/api/rest_v1/media/math/render/svg/126c6935d3dd9f1c1da0c388ca2799be4f6f237c\" class=\"mwe-math-fallback-image-inline\" aria-hidden=\"true\" style=\"vertical-align:-0.505ex;width:12.629ex;height:2.843ex;\" /></span></dd></dl>"
}

Looking at our code, if thumbnail source does not contain "px-" in the title , it will fall back to the originalimage
https://upload.wikimedia.org/wikipedia/commons/thumb/3/3d/Completing_the_square.ogv/Completing_the_square.ogv.jpg

We'll need to guard against that from happening.

See also: https://phabricator.wikimedia.org/T92457#4122197

Developer notes

A test has been written here - https://gerrit.wikimedia.org/r/430665

QA instructions

  1. Hover over these two links with a retina monitor and make sure the page previews do not have images in them
  2. Hover over the same two links with a non-retina monitor and make sure the page previews do have images in them

Event Timeline

Jdlrobson created this task.May 3 2018, 8:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 3 2018, 8:05 PM

Change 430665 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Hypothetical test scenario

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

ovasileva triaged this task as Medium priority.Jun 5 2018, 9:15 AM
Jdlrobson renamed this task from Is it possible for a video to be used as a thumbnail in a page preview? to Do not show a broken thumbnail for small video thumbnails.Jun 12 2018, 4:48 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson set the point value for this task to 5.

We talked about this and there's several ways to approach this - a whitelist or blacklist in the client or a more correct way would be to remove the originalimage code altogether and make this change in the summary endpoint.

After poking around the summery endpoint, I realized that if we were to fix this issue there, I'm not sure what the correct response should be. The endpoint proxies the value from pageImages api, which, as seen here: https://en.wikipedia.org/w/api.php?action=query&format=json&prop=pageimages&titles=Completing_the_square&redirects=1&converttitles=1&piprop=thumbnail%7Cname%7Coriginal
returns the same non-image for "original".

bearND added a subscriber: bearND.Jun 14 2018, 2:50 PM

@Jdlrobson I'm not sure what you'd want to change in the PCS page/summary output. The two image URLs shows in the description work fine for me:

@bearND the issue is not with the original field, but the way we use it under a certain circumstances in page previews.

SO the idea would be to work out why we do that and stop doing that - adding that logic to the summary endpoint if necessary.

I think, the underlying problem is (need to confirm via MTC) if you request a thumbnail size from mediawiki's api that is bigger than the actual thumbnail, the mediawiki api returns the original API. For instance if we request a 320px thumbnail of a video that's actually 200px by 200px the video URI is returned. So @Jdrewniak, we'd want in this case a 200px thumbnail to be sent (which would be an additional API request) or no image at all (some logic in the summary endpoint to strip it).

Jdrewniak moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

For instance if we request a 320px thumbnail of a video that's actually 200px by 200px the video URI is returned.

Hm.. I don't see the API doing that? Using the same example as before, api.php?prop=pageimages&titles=Completing_the_square&...pithumbsize=700 responds as follows:

"thumbnail": {
  "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/3/3d/Completing_the_square.ogv/640px--Completing_the_square.ogv.jpg",
  "width": 640, "height": 480
  },
"original": { ..},

Notice how it does provide a usable image in the "thumbnail" field despite the request asking for a 700px thumbnail of a 640x480 video. Both the PageImages API and core's prop=imageinfo API behave like this.

As far as I know, there are no scenarios in which an API client would need to manually use the original url instead of the provided thumbnail url. This is unsafe in most cases. Especially for videos and other non-image file types. Although even for image file types, various types are not save for viewing (SVG, TIFF, etc.)

There is exactly one case in which it is safe (namely, the cases where MediaWiki performs an optimisation per T67383 for the handful of image-types that are safe for viewing), and for that case, MediaWiki API already responds with a "thumbnail" url that matches the original, this is one so that clients don't have think about it :)

Change 430665 abandoned by Jdlrobson:
Hypothetical test scenario

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

Change 441316 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] Ensure popup thumbnail images are a supported format

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

Change 441316 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Ensure popup thumbnail images are a supported format

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

Should not be a noticeable change but you may want to do a review of several page previews and check image thumbnails are rendering as expected.

@Jdlrobson what type of issue might be occurring with the rendering? where should I test this?

@alexhollender yeah I don't really have a list of pages that have videos as the page-image of an article... I'll see what I can do...

Here a list of pages to check that Jan generated. I will go through a handful of these on production and identify ones that are broken (e.g. on retina screens we're showing a broken image in the page preview). Then I will look at those in staging and verify that:

  • on non-retina monitor an image is included in the page preview
  • on retina monitor no image is included in the page preview

just to follow up on that comment, here are the articles with non-standard page_images that I could query from the data dumps:

page_image query
+------------------------------------------------------------------------+-------------------------------------------------+
| pp_value                                                               | page_title                                      |
+------------------------------------------------------------------------+-------------------------------------------------+
| Andy_Griffith_Show_Opening.ogg                                         | The_Andy_Griffith_Show                          |
| The_Tragically_Hip_EP.bmp                                              | The_Tragically_Hip_(EP)                         |
| Omni_logo_blue.pdf                                                     | The_Omni_Group                                  |
| Guide_to_the_Literature_of_Photography_and_Related_Subjects.pdf        | Microprinting                                   |
| MagnoliaBibleCollegeSeal.bmp                                           | Magnolia_Bible_College                          |
| Schuman_Declaration.ogg                                                | Schuman_Declaration                             |
| PrinceWhyYouWannaTreatMeSoBad.bmp                                      | Why_You_Wanna_Treat_Me_So_Bad?                  |
| Seizure_clip_from_Dennō_Senshi_Porygon.ogg                             | Dennō_Senshi_Porygon                            |
| Moldovenism_-_Garbuz.ogv                                               | Moldovenism                                     |
| Brushfire-Records-Logo.bmp                                             | Brushfire_Records                               |
| Kiwi_Pro_Wrestling.bmp                                                 | Kiwi_Pro_Wrestling                              |
| HOT3_logo_2010.bmp                                                     | Hot_3                                           |
| ScullyDustJeS.xcf                                                      | Je_Souhaite                                     |
| Three_dollar_bill_cinema_logo.pdf                                      | Seattle_Lesbian_&_Gay_Film_Festival             |
| When_I'm_Cleaning_Windows,_from_Keep_Your_Seats,_Please_(1936).ogv     | When_I'm_Cleaning_Windows                       |
| Ws_Smash_Shot.Png                                                      | WebSphere_sMash                                 |
| Mr._Men_The_Complete_Collection_(2010),_bottom_cover.pdf               | List_of_Mr._Men                                 |
| Total_Control.bmp                                                      | Total_Control_(Yo-Yo_album)                     |
| Santa_Fe_Reporter_Logo.pdf                                             | Santa_Fe_Reporter                               |
| Little_Miss_The_Complete_Collection_(2012),_bottom_cover.pdf           | List_of_Little_Miss_characters                  |
| CSLS_Logo.pdf                                                          | California_Southern_Law_School                  |
| Logo_of_company.pdf                                                    | GovTrack                                        |
| National_Philatelic_Society_logo.bmp                                   | National_Philatelic_Society                     |
| FICS_Logo_photo.bmp                                                    | International_Federation_of_Sports_Chiropractic |
| Off_the_Ropes.bmp                                                      | Off_the_Ropes                                   |
| Bilasipara_College_logo.webp                                           | Bilasipara_College                              |
| 2010_MEAC.bmp                                                          | 2010_MEAC_Men's_Basketball_Tournament           |
| Dirt_album_cover.bmp                                                   | Dirt_(Kids_in_Glass_Houses_album)               |
| Gang_CD_Cover.bmp                                                      | Gang_(song)                                     |
| Toxyn.bmp                                                              | Toxyn                                           |
| Rostropovich-Vishnevskaya_Foundation_logo.pdf                          | Rostropovich-Vishnevskaya_Foundation            |
| Haq_tv.TIF                                                             | Haq_TV                                          |
| Thumbay_Hospital_Logo.pdf                                              | Thumbay_Hospital                                |
| Silver_City_Daily_Press_and_Independent_front_page,_March_11,_2015.pdf | Silver_City_Daily_Press_and_Independent         |
| SunSmart_CMYK.pdf                                                      | SunSmart                                        |
| Sugar_Girls_cover.TIF                                                  | The_Sugar_Girls                                 |
| Ranaghat_College_logo.webp                                             | Ranaghat_College                                |
| Maharani_Kasiswari_College.webp                                        | Maharani_Kasiswari_College                      |
| Chaarfutiya_Chhokare.jPG                                               | Chaarfutiya_Chhokare                            |
| Earth_Law_Center_Logo.jpeg.pdf                                         | Earth_Law_Center                                |
| Logo_GIROS.TIF                                                         | Italian_Group_for_Research_on_Hardy_Orchid      |
| Fresh_patch_logo.pdf                                                   | Fresh_Patch                                     |
| Nuqat_logo.pdf                                                         | Nuqat                                           |
| Aslaug_Haviland.pdf                                                    | Aslaug_Haviland                                 |
| Marvel_Studios_New_Logo_And_Fanfare_July_2016.webm                     | Music_of_the_Marvel_Cinematic_Universe          |
| Informal_Talks_Logo.webp                                               | Informal_Talks                                  |
| City_Greenways_Plan_Map.pdf                                            | Vancouver_Greenway_Network                      |
| Checkeredpastep.webp                                                   | Checkered_Past_(EP)                             |
| PlayMagnus.webp                                                        | Play_Magnus                                     |
| Bhawanipur_Anchalik_College_logo.webp                                  | Bhawanipur_Anchalik_College                     |
| Vricon_logo.pdf                                                        | Vricon                                          |
| The_World_Children's_Winners_Games_logo.pdf                            | The_World_Children's_Winners_Games              |
| DimitriJChandris1.pdf                                                  | Dimitri_Chandris                                |
| Crazy_Ex-Girlfriend_cast_promo.webp                                    | List_of_Crazy_Ex-Girlfriend_characters          |
| We_Don't_Give_a_Damn.ogv                                               | We_Don't_Give_a_Damn                            |
| Transference.webp                                                      | Transference_(video_game)                       |
+------------------------------------------------------------------------+-------------------------------------------------+
56 rows in set (1.97 sec)

This is the query I'm using after importing the page_props and latest_pages tables from enwiki.

SELECT  a.pp_value, b.page_title FROM page_props AS a JOIN page AS b ON a.pp_page = b.page_id  WHERE
a.pp_propname="page_image" AND 
a.pp_value NOT LIKE '%.jpg' AND 
a.pp_value NOT LIKE '%.JPG' AND 
a.pp_value NOT LIKE '%.Jpg'  AND 
a.pp_value NOT LIKE '%.JPEG' AND 
a.pp_value NOT LIKE '%.jpeg' AND 
a.pp_value NOT LIKE '%.Jpeg' AND
a.pp_value NOT LIKE '%.png' AND 
a.pp_value NOT LIKE '%.PNG' AND 
a.pp_value NOT LIKE '%.GIF' AND 
a.pp_value NOT LIKE '%.gif' AND 
a.pp_value NOT LIKE '%.svg' AND 
a.pp_value NOT LIKE '%.SVG' AND 
a.pp_value NOT LIKE '%.tiff' AND 
a.pp_value NOT LIKE '%.tif';

however out of all those, the only broken thumbnails showing up for me are

This is probably because I'm on a hi-res monitor and the originals aren't being shown because they are too small anyway. More broken thumbnails might appear on a lower-res monitor.

Vvjjkkii renamed this task from Do not show a broken thumbnail for small video thumbnails to 7ndaaaaaaa.Jul 1 2018, 1:12 AM
Vvjjkkii removed alexhollender as the assignee of this task.
Vvjjkkii raised the priority of this task from Medium to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii edited subscribers, added: alexhollender; removed: gerritbot, Aklapper.
CommunityTechBot renamed this task from 7ndaaaaaaa to Do not show a broken thumbnail for small video thumbnails.Jul 2 2018, 1:14 AM
CommunityTechBot assigned this task to alexhollender.
CommunityTechBot lowered the priority of this task from High to Medium.
CommunityTechBot set the point value for this task to 5.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot edited subscribers, added: gerritbot, Aklapper; removed: alexhollender.

@Jdrewniak none of these pages are showing broken thumbnails for me (on my low res monitor, or my retina screen). Unable to test so far. Any ideas?

Jdrewniak added a comment.EditedJul 4 2018, 1:05 PM

@alexhollender I don't know 🤷‍♂️ The change doesn't seem to be deployed on enwiki, but even the previews that were broken for me last week seem to be fixed now* ...

last weekthis week

*I think "fixed" can be considered showing no image instead of a broken image.

Even the one that we found broken months back seems ok now:

last monthnow

note: these are from en.wikipedia.org

Correction. This fix is deployed everywhere now, so the changes are correct and caused by this patch. I was just confused because of the timestamps on Special:Version...

timezones...

alexhollender updated the task description. (Show Details)Jul 4 2018, 4:11 PM

Looks good to me. Added QA instructions, sending off to Anthony.

Looks good to me on production. I tested on a MAC with a retina screen and did not see a thumbnail on the links. I tested on a Chromebook without retina and the thumbnails were present. Behaving as expected.

ovasileva closed this task as Resolved.Jul 6 2018, 11:55 AM