New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't send inline WebP to Chrome/36 on iOS #969

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 15 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

What steps will reproduce the problem?
1. Visit 
http://modpagespeed.com/rewrite_images.html?PageSpeedWebpRecompressionQuality=40
 on Chrome on desktop (& refresh)
2. Visit the same site on Chrome on iOS (& refresh)
3. Look for the missing image of a yellow 'caution' sign


Note that webp conversion and inlining are both on by default in 1.8.  Most 
photos are too large to inline without an aggressive compression setting (e.g. 
40) and so this won't show up frequently except with small thumbnails.

Original issue reported on code.google.com by jmara...@google.com on 11 Aug 2014 at 1:53

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 11 Aug 2014 at 1:53

  • Changed title: Chrome on iOS does not display inlined webp correctly
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

A workaround for this problem can be added to your pagespeed.conf or .htaccess 
file:

SetEnvIf User-Agent CriOS mps_disable_webp
RequestHeader set ModPagespeedFilters -convert_jpeg_to_webp env=mps_disable_webp

Note, this problem and the above workaround only affect Chrome on iOS.

Original comment by jmara...@google.com on 11 Aug 2014 at 4:44

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

For more details, see the updated bug repro test site: 
http://www.modpagespeed.com/test/chrome_ios_inline_webp/cr402435.html and the 
workaround demo: 
http://www.modpagespeed.com/test/chrome_ios_inline_webp/mps/workaround.html

Original comment by jmara...@google.com on 11 Aug 2014 at 4:45

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

[deleted comment]
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

See bug on Chrome bug queue: 
https://code.google.com/p/chromium/issues/detail?id=402514

Original comment by jmara...@google.com on 11 Aug 2014 at 5:35

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

As the Chrome bug is not a blocker for Chrome 37, we need to release a better 
temporary fix in mod_pagespeed itself.

The fix must (like the workaround) allow webps to inlined on non-iOS devices.  
Evidently the Google Search Appliance also works fine on iOS, and it's only 
Chrome that fails, so we should make sure to leave everything working optimally 
on GSA.

My GSA user-agent on iPad is: Mozilla/5.0 (iPad; CPU OS 7_1_2 like Mac OS X) 
AppleWebKit/537.51.1 (KHTML, like Gecko) GSA/4.1.0.31802 Mobile/11D257 
Safari/9537.53


Note that the UA hack (CriOS) in the "SetEnvIf" workaround would not match the 
GSA one, which is perfect.


Now, the temporary fix can go in two directions:

  1. A webp image, if small enough to inline, should be reverted to a jpeg for CriOS and inlined that way if the jpeg is also small enough

OR

  2. A webp image should be rendered as an external .webp on CriOS even if it's small enough to inline.


I think #1 is better (inline as jpeg if small enough) because a slightly larger 
inline jpeg probably offers a faster rendering experience for users than an 
external webp.  Note that if the site's priority was optimizing for bandwidth, 
then they should be using "ModPagespeedRewriteLevel OptimzeForBandwidth" which 
doesn't have inlining in the first place.

However #2 is acceptable, if easier to implement, because the Chrome team has 
accepted http://crbug.com/402514 so a fix will come sometime in the future.


Careful consideration needs to be made while fixing this bug on the impact to 
downstream caching 
(https://developers.google.com/speed/pagespeed/module/downstream-caching)

Original comment by jmara...@google.com on 11 Aug 2014 at 5:46

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Clarification on option #1 on "reverted to a jpeg".  What I meant was, that we 
should go back to the original source format of the image and attempt to 
convert that to an optimized jpeg.  We might need to do another metadata cache 
lookup as if the query was coming from a non-webp-capable UA.

If the webp was lossless then it cannot be transcoded to a jpeg and should be 
left whatever it was (png or gif), provided that's small enough to inline.  
There may be other nuances like animation or transparency or something that 
need to be considered.  Note that lossless webp conversion is not on by default.

If all that adds too much complexity, then #2 is probably easier.

Original comment by jmara...@google.com on 11 Aug 2014 at 5:51

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Option 1 is a little messy to implement now, but can be cleaned up after using 
Scanline Reader/Writer throughout image compression and replacing 
SingleRewriteContext with (mutliple)RewriteContext (this is part of the 
refactoring I'm doing). But I've a concern about the extra memory and 
computation cost. 

Here are something about our current behavior:
1. Cache key for the rewritten image has one bit for whether it's WebP and one 
bit for whether UA is mobile. The key doesn't encode whether the UA is iOS.
2. The image is rewritten in the low priority thread but inlining is done in 
the high priority thread.

So for all of the images which may be converted to WebP and inlined and for 
mobile UA, we have to rewrite twice, one for WebP format and one for PNG/JPEG, 
and save both to cache. We can't wait until inlining failed to start the second 
rewrite nor have the second rewrite just for iOS. If downstream caching is used 
and supports WebP and inlining, we need to rewrite the images twice, too.

Does the extra computation and memory outweigh the benefits of inlining?

Original comment by hui...@google.com on 11 Aug 2014 at 9:54

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

OK let's stick with option 2, especially for 1.8 support (which needs to be 
patched to that branch).

I think this decision can be made in HtmlResourceSlot::Render, or thereabouts.

Original comment by jmara...@google.com on 11 Aug 2014 at 10:00

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Note: a patch for this has been submitted to the Chrome repo: 
https://chromereviews.googleplex.com/73037013/

However, I do not know the timeline for a Chrome release incorporating the fix.

Original comment by jmara...@google.com on 11 Aug 2014 at 11:34

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Following http://crbug.com/402514, it looks like:

M35 and below: not applicable
M36: will stay broken
M37: will temporarily disable including "webp" in the Accept header
M38 and later: fixed

I'm going to send an announcement today, suggesting site owners disable webp 
for Chrome 36 on iOS (Apache) or entirely (Nginx) until M36 is no longer common.

Original comment by jefftk@google.com on 15 Aug 2014 at 1:32

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Announced:

https://groups.google.com/forum/#!topic/mod-pagespeed-announce/kSK2-gDATIo
https://groups.google.com/forum/#!topic/ngx-pagespeed-announce/5USP9Gi_Q00

Original comment by jefftk@google.com on 15 Aug 2014 at 2:37

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jefftk@google.com on 18 Aug 2014 at 1:05

  • Changed title: Don't send inline WebP to Chrome/36 on iOS
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Fixed in https://code.google.com/p/modpagespeed/source/detail?r=4160

Original comment by jmara...@google.com on 18 Sep 2014 at 4:24

  • Added labels: Milestone-v32, release-note
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 18 Sep 2014 at 4:24

  • Changed state: Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment