mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
126 stars 41 forks source link

Optimize image used for full size theme previews (not thumbnail) #8256

Closed eviljeff closed 3 years ago

eviljeff commented 3 years ago

addons-frontend displays the full size theme preview in search results, so will display up to 25 of them per page. Jpegs are generally smaller so would result in a significant download saving for the user. Drawback - potentially ugly artefacts that could put users off installing the theme, because they think that's what their Firefox would look like too.

In addition to just saving as jpeg we have some other options to consider (copied over from mozilla/addons#8244):

eviljeff commented 3 years ago

Investigation+matrix discussion outcome: http://aka-andy.com/theme_preview_comparison.html (just jpeg vs. pngcrush'd png at the time of writing). tl;dr there are no easy wins with just saving as jpeg.

Jpegs (lower quality more so) tend to produce "fuzzy" UI elements in the preview, but pngs are large (especially the themes that are built with jpegs). Potentially we could use mozjpeg options to avoid some of the ugliest artefacts around UI elements - but those options will result in larger file sizes - in some cases it could make the filesize larger than the PNG, but maybe we don't care since the previews it would happen to are already fairly small in size. The other alternative is generating and serving an svg based preview file.

An svg preview would work by rendering the images of the theme into a single flat image (i.e. flattening potentially multiple images into 1, in a fixed position, and cropping to the size of the preview). Then the svg would contain the UI elements to be rendered by the browser.

For bonus file size efficiency we could take into account the source image format in the theme - render down the images into a jpg if the source is a jpeg; png if a png. This way we wouldn't end up with larger jpegs from pngs that look terrible, or huge pngs from jpegs (like now, in most cases).

eviljeff commented 3 years ago

Summary of how we generate the theme previews

Currently:

  1. the manifest* metadata is processed to get the colors and background positioning settings
  2. background images are extracted from the xpi to blobs
  3. a template context is built with colors, background positioning settings, and the background images encoded as base64 strings, (with appropriate defaults for the background positioning)
  4. render sizes are added to the context
  5. a django template is used with the context to render the svg in memory
  6. the svg is rasterized with rsvg-convert. The tool only works with files so the svg is saved to a temporary file beforehand, and the output is a png file (the png file is the full size theme preview).
  7. the png file is resized/resaved to a png or jpeg file for the thumbnail.
  8. (the png file is sent through pngcrush and colors are extracted from the thumbnail with extract_colors_from_image)
  9. steps 4 to 8 are repeated for all the renderings (we currently have two - one that Firefox uses in addon manager at 680w with a png thumbnail; and AMOs rendering at 720w with a jpeg thumbnail)

SVG based preview patch:

Based on the earlier investigation, WIP patch (with some TODOs, and without tests) https://github.com/eviljeff/olympia/commit/09693fcf1bea8150dd53fb3cee61975b339f42f2. The process is largely the same as above, except we're rendering twice - first with transparent colors to hide the UI, then again with the result of the first render as the single background. So steps 4-8 are replaced with:

  1. render sizes are added to the context; context keys for the UI colors are replaced with rgb(0,0,0,0) to hide the UI.
  2. a django template is used with the context to render the svg in memory. Then:
    1. the svg is rasterized with rsvg-convert. The svg is saved to a temporary file beforehand, and the png file output is also a temporary file.
    2. the png file is resaved as a jpeg and loaded into a blob to be used as the background
    3. a new context is built with the original manifest colors, and the jpeg background blob, encoded as a base64 string.
    4. the svg we've built in memory is saved to a file (this is the full size theme preview)
  3. the svg is rasterized with rsvg-convert again for the thumbnail. The output is a png file (in my patch this is saved as the 'original' size image, in case we want to rescale the thumbnail again, without having to re-render from scratch).
  4. the png file is resized/resaved to a png or jpeg file for the thumbnail.
  5. (the png file is sent through pngcrush and colors are extracted from the thumbnail with extract_colors_from_image)

*manifest data itself is already available at this point in an upload flow so it's just passed along to celery as a kwarg to the task; for recreation tasks we parse the xpi beforehand.

ioanarusiczki commented 3 years ago

@eviljeff I submitted some themes - as an .xpi package and using the theme wizard.

In both cases I've noticed that the icons and previews are not extracted correctly on the frontend. I'll give 2 examples:

They look like that in search results or on shelves too. At install, the browser shows them as it should.

eviljeff commented 3 years ago

@ioanarusiczki fixed in mozilla/addons-server#16852

ioanarusiczki commented 3 years ago

@eviljeff At a quick check with the examples above it looks good now. These are the themes: https://addons-dev.allizom.org/en-US/firefox/addon/purple-arrow-test-2/ https://addons-dev.allizom.org/en-US/firefox/addon/the-moon-cat-test2/ https://addons-dev.allizom.org/en-US/firefox/addon/theme-generated-from-wizard/

how the previews work

I think it should be fine, I'm going to make a couple more theme uploads using various models just to be sure.

eviljeff commented 3 years ago

That opening the svg link in a new tab doesn't show the background content appears a bit weird. It's CSP related though - an svg loaded in a <img> tag works as expected because the svg is treated purely as an image; once you try to load it directly it's treated as an xml document instead, and we block inline content (i.e. the background image) for security in documents. I'm not aware of any use-case for being able to load the svg in a tab so I'd say it's fine.

ioanarusiczki commented 3 years ago

@eviljeff

I don't know a use case for opening the .svg into a new tab either.

I've been testing again with some new uploads today and checked the icons on the frontend:

-> on the detail pages or search results the icons look good now. The version-previews have a .svg format, size 720x92

-> the icons on shelves, on the user profile, more themes section are generated from .jpg version-previews (720x92) with a quality reduced to 35, this has been done in the previous performance issues.

-> I checked the collections and noticed that the icons here are generated from .jpg files as for shelves. Should this be the expected? I say it because if I compare the images below - the one from collections is visibly at lower quality.

The theme into a search result: https://addons-dev.allizom.org/en-US/firefox/search/?q=Themes%20again%20with%20wizard

The theme added to a collection: (with a lower quality) https://addons-dev.allizom.org/en-US/firefox/collections/10641574/Themes-again-with-wizard/?page=1&collection_sort=-added

Another example (here it's not that obvious, so it also depends on the graphics): https://addons-dev.allizom.org/en-US/firefox/search/?q=black%20rain%20animated%20tested into a collection: https://addons-dev.allizom.org/en-US/firefox/collections/10641574/Black-Rain-Animated/?page=1&collection_sort=-added

eviljeff commented 3 years ago

-> I checked the collections and noticed that the icons here are generated from .jpg files as for shelves. Should this be the expected? I say it because if I compare the images below - the one from collections is visibly at lower quality.

The theme into a search result: https://addons-dev.allizom.org/en-US/firefox/search/?q=Themes%20again%20with%20wizard

The theme added to a collection: (with a lower quality) https://addons-dev.allizom.org/en-US/firefox/collections/10641574/Themes-again-with-wizard/?page=1&collection_sort=-added

That looks like a frontend problem - it's displaying a thumbnail image at full size rather than a full size image. Can you log an issue on frontend? (I don't think it's regression from this work - it's more likely from the frontend changes to display thumbnails)