artox/incubator-pagespeed-ngx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Basic support for rewriting <img srcset>

Browse Source
This commit is contained in:
Maks Orlovich
2016-06-29 17:48:59 -04:00
parent 8ab12c589a
commit 428b7e044e
9 changed files with 140 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
install/mod_pagespeed_test/image_rewriting/srcset.html
+9
View File
@@ -0,0 +1,9 @@
<html>
<head>
<title>Test for srcset rewriting</title>
</head>
<body>
<img src="../../mod_pagespeed_example/images/Puzzle.jpg"
srcset="../../mod_pagespeed_example/images/Puzzle.jpg 1x, ../../mod_pagespeed_example/images/Cuppa.png 2x"><br/>
</body>
</html>
net/instaweb/rewriter/image_rewrite_filter.cc
+85 -32
View File
@@ -52,6 +52,7 @@
#include "net/instaweb/rewriter/public/rewrite_options.h"
#include "net/instaweb/rewriter/public/server_context.h"
#include "net/instaweb/rewriter/public/single_rewrite_context.h"
#include "net/instaweb/rewriter/public/srcset_slot.h"
#include "net/instaweb/util/public/property_cache.h"
#include "pagespeed/controller/central_controller.h"
#include "pagespeed/controller/expensive_operation_callback.h"
@@ -380,15 +381,23 @@ const char* MessageForInlineResult(InlineResult inline_result) {
class ImageRewriteFilter::Context : public SingleRewriteContext {
public:
enum class Place {
kCss,
kFetch,
kHtmlAttr,
kSrcset,
kNonCssNested,
};
Context(int64 css_image_inline_max_bytes,
ImageRewriteFilter* filter, RewriteDriver* driver,
RewriteContext* parent, ResourceContext* resource_context,
bool is_css, int html_index, bool in_noscript_element,
Place place, int html_index, bool in_noscript_element,
bool is_resized_using_rendered_dimensions)
: SingleRewriteContext(driver, parent, resource_context),
css_image_inline_max_bytes_(css_image_inline_max_bytes),
filter_(filter),
is_css_(is_css),
place_(place),
html_index_(html_index),
in_noscript_element_(in_noscript_element),
is_resized_using_rendered_dimensions_(
@@ -425,7 +434,7 @@ class ImageRewriteFilter::Context : public SingleRewriteContext {
int64 css_image_inline_max_bytes_;
ImageRewriteFilter* filter_;
bool is_css_;
Place place_;
const int html_index_;
bool in_noscript_element_;
bool is_resized_using_rendered_dimensions_;
@@ -561,12 +570,12 @@ void ImageRewriteFilter::Context::Render() {
CachedResult* result = output_partition(0);
bool rewrote_url = false;
ResourceSlot* resource_slot = slot(0).get();
if (is_css_ || !has_parent()) {
if (place_ == Place::kCss || !has_parent()) {
InlineResult inline_result;
if (is_css_) {
if (place_ == Place::kCss) {
rewrote_url = filter_->FinishRewriteCssImageUrl(
css_image_inline_max_bytes_, result, resource_slot, &inline_result);
} else { // html
} else if (place_ == Place::kHtmlAttr) {
// We use manual rendering for HTML, as we have to consider whether to
// inline, and may also pass in width and height attributes.
HtmlResourceSlot* html_slot = static_cast<HtmlResourceSlot*>(
@@ -587,9 +596,10 @@ void ImageRewriteFilter::Context::Render() {
}
}
if (Driver()->options()->Enabled(RewriteOptions::kInlineImages)) {
if (Driver()->options()->Enabled(RewriteOptions::kInlineImages) &&
(place_ == Place::kCss || place_ == Place::kHtmlAttr)) {
// Show the debug message for inlining only when this option has been
// enabled.
// enabled. We also don't inline srcset images.
// Note: Although debug message is saved to the cached_result, it is
// *not* cached because the cached_result has already been stored in
// the cache previously. This is good because the exact debug message
@@ -1406,6 +1416,29 @@ const ContentType* ImageRewriteFilter::ImageToContentType(
return content_type;
}
void ImageRewriteFilter::ComputePreserveUrls(
const RewriteOptions* options, ResourceSlot* slot) {
// Note that in RewriteOptions::Merge we turn off image_preserve_urls
// when merging into a configuration that has explicitly
// enabled cache_extend_images.
//
// Consider a hosting provider that turns on "optimize for
// bandwidth" mode, and then a site enables resize_images
// explicitly. That should override the image-url-preservation
// default that was set at root. Note that explicitly turning on
// RecompressImages doesn't mean we'll want to override
// image_preserve_urls rewrite URLs here, since we can still get
// the benefit of recompression via IPRO. But we make an
// exception for inlining and image-resizing directives since
// those can only be done via url-rewriting.
if (options->image_preserve_urls() &&
!options->Enabled(RewriteOptions::kResizeImages) &&
!options->Enabled(RewriteOptions::kResizeToRenderedImageDimensions) &&
!options->Enabled(RewriteOptions::kInlineImages)) {
slot->set_preserve_urls(true);
}
}
void ImageRewriteFilter::BeginRewriteImageUrl(HtmlElement* element,
HtmlElement::Attribute* src) {
scoped_ptr<ResourceContext> resource_context(new ResourceContext);
@@ -1450,34 +1483,45 @@ void ImageRewriteFilter::BeginRewriteImageUrl(HtmlElement* element,
Context* context = new Context(0 /* No CSS inlining, it's html */,
this, driver(), NULL /*not nested */,
resource_context.release(),
false /*not css */, image_counter_++,
Context::Place::kHtmlAttr, image_counter_++,
noscript_element() != NULL,
is_resized_using_rendered_dimensions);
ResourceSlotPtr slot(driver()->GetSlot(input_resource, element, src));
context->AddSlot(slot);
// Note that in RewriteOptions::Merge we turn off image_preserve_urls
// when merging into a configuration that has explicitly
// enabled cache_extend_images.
//
// Consider a hosting provider that turns on "optimize for
// bandwidth" mode, and then a site enables resize_images
// explicitly. That should override the image-url-preservation
// default that was set at root. Note that explicitly turning on
// RecompressImages doesn't mean we'll want to override
// image_preserve_urls rewrite URLs here, since we can still get
// the benefit of recompression via IPRO. But we make an
// exception for inlining and image-resizing directives since
// those can only be done via url-rewriting.
if (options->image_preserve_urls() &&
!options->Enabled(RewriteOptions::kResizeImages) &&
!options->Enabled(RewriteOptions::kResizeToRenderedImageDimensions) &&
!options->Enabled(RewriteOptions::kInlineImages)) {
slot->set_preserve_urls(true);
}
ComputePreserveUrls(options, slot.get());
driver()->InitiateRewrite(context);
}
void ImageRewriteFilter::BeginRewriteSrcSet(HtmlElement* element,
HtmlElement::Attribute* srcset) {
// TODO(morlovich): This needs to go through a factory method like other
// slots do, once more than one filter uses it, so they share, otherwise
// they would randomly overwrite each other.
RefCountedPtr<SrcSetSlotCollection> slot_collection(
new SrcSetSlotCollection(driver(), this, element, srcset));
for (int i = 0; i < slot_collection->num_image_candidates(); ++i) {
SrcSetSlot* slot = slot_collection->slot(i);
if (slot == nullptr) {
continue;
}
scoped_ptr<ResourceContext> resource_context(new ResourceContext);
EncodeUserAgentIntoResourceContext(resource_context.get());
Context* context = new Context(0 /* No CSS inlining, it's html */,
this, driver(), nullptr /*not nested */,
resource_context.release(),
Context::Place::kSrcset, image_counter_++,
noscript_element() != nullptr,
false /*not resizing with rendered dim */);
context->AddSlot(RefCountedPtr<ResourceSlot>(slot));
ComputePreserveUrls(driver()->options(), slot);
driver()->InitiateRewrite(context);
}
}
bool ImageRewriteFilter::FinishRewriteCssImageUrl(
int64 css_image_inline_max_bytes, const CachedResult* cached,
ResourceSlot* slot, InlineResult* inline_result) {
@@ -2051,6 +2095,13 @@ void ImageRewriteFilter::EndElementImpl(HtmlElement* element) {
BeginRewriteImageUrl(element, attributes[i].url);
}
if (element->keyword() == HtmlName::kImg) {
HtmlElement::Attribute* srcset = element->FindAttribute(HtmlName::kSrcset);
if (srcset != nullptr) {
BeginRewriteSrcSet(element, srcset);
}
}
}
const UrlSegmentEncoder* ImageRewriteFilter::encoder() const {
@@ -2073,7 +2124,8 @@ RewriteContext* ImageRewriteFilter::MakeRewriteContext() {
EncodeUserAgentIntoResourceContext(resource_context);
return new Context(0 /*No CSS inlining, it's html */,
this, driver(), NULL /*not nested */,
resource_context, false /*not css */,
resource_context,
Context::Place::kFetch,
kNotCriticalIndex,
false /*not in noscript */,
false /*not resized by rendered dimensions*/);
@@ -2101,7 +2153,8 @@ RewriteContext* ImageRewriteFilter::MakeNestedRewriteContextForCss(
}
Context* context = new Context(css_image_inline_max_bytes,
this, NULL /* driver*/, parent,
cloned_context, true /*is css */,
cloned_context,
Context::Place::kCss,
kNotCriticalIndex,
false /*not in noscript */,
false /*not resized by rendered dimensions*/);
@@ -2119,8 +2172,8 @@ RewriteContext* ImageRewriteFilter::MakeNestedRewriteContext(
}
Context* context = new Context(
0 /*No Css inling */, this, NULL /* driver */, parent, resource_context,
false /*not css */, kNotCriticalIndex, false /*not in noscript */,
false /*not resized by rendered dimensions*/);
Context::Place::kNonCssNested, kNotCriticalIndex,
false /*not in noscript */, false /*not resized by rendered dimensions*/);
context->AddSlot(slot);
return context;
}
net/instaweb/rewriter/image_rewrite_filter_test.cc
+14
View File
@@ -1326,6 +1326,20 @@ TEST_F(ImageRewriteTest, ImgTag) {
RewriteImage("img", kContentTypeJpeg);
}
TEST_F(ImageRewriteTest, ImgSrcSet) {
AddFileToMockFetcher("a.png", kBikePngFile, kContentTypePng, 100);
AddFileToMockFetcher("b.png", kCuppaPngFile, kContentTypePng, 100);
options()->EnableFilter(RewriteOptions::kRecompressPng);
rewrite_driver()->AddFilters();
ValidateExpected(
"srcset",
"<img src=\"a.png\" srcset=\"a.png 1x, b.png 2x\">",
"<img src=\"xa.png.pagespeed.ic.0.png\" "
"srcset=\"xa.png.pagespeed.ic.0.png 1x, xb.png.pagespeed.ic.0.png 2x\">");
}
TEST_F(ImageRewriteTest, ImgTagWithComputeStatistics) {
options()->EnableFilter(RewriteOptions::kComputeStatistics);
RewriteImage("img", kContentTypeJpeg);
net/instaweb/rewriter/public/image_rewrite_filter.h
+3
View File
@@ -229,6 +229,9 @@ class ImageRewriteFilter : public RewriteFilter {
const ContentType* ImageToContentType(const GoogleString& origin_url,
Image* image);
void BeginRewriteImageUrl(HtmlElement* element, HtmlElement::Attribute* src);
void BeginRewriteSrcSet(HtmlElement* element, HtmlElement::Attribute* srcset);
void ComputePreserveUrls(const RewriteOptions* options, ResourceSlot* slot);
RewriteResult RewriteLoadedResourceImpl(Context* context,
const ResourcePtr& input_resource,
net/instaweb/rewriter/public/srcset_slot.h
+5
View File
@@ -75,6 +75,9 @@ class SrcSetSlotCollection : public RefCounted<SrcSetSlotCollection> {
HtmlElement* element() { return element_; }
RewriteDriver* driver() { return driver_; }
int begin_line_number() const { return begin_line_number_; }
int end_line_number() const { return end_line_number_; }
// This serializes everything back to the attribute.
// (Which is sadly quadratic, but the size should be small enough that it's
// more practical than trying to coordinate).
@@ -96,6 +99,8 @@ class SrcSetSlotCollection : public RefCounted<SrcSetSlotCollection> {
RewriteDriver* driver_;
HtmlElement* element_;
HtmlElement::Attribute* attribute_;
int begin_line_number_;
int end_line_number_;
DISALLOW_COPY_AND_ASSIGN(SrcSetSlotCollection);
};
net/instaweb/rewriter/resource_slot.cc
+2
View File
@@ -110,6 +110,8 @@ HtmlResourceSlot::HtmlResourceSlot(const ResourcePtr& resource,
// that and simplify the code?
url_relativity_(
GoogleUrl::FindRelativity(attribute->DecodedValueOrNull())),
// Note: these need to be deep-copied in case we run as a detached
// rewrite, in which case element_ may be dead.
begin_line_number_(element->begin_line_number()),
end_line_number_(element->end_line_number()) {
}
net/instaweb/rewriter/responsive_image_filter_test.cc
+9 -2
View File
@@ -526,12 +526,19 @@ TEST_F(ResponsiveImageFilterTest, Debug) {
"<!--ResponsiveImageFilter: Not adding srcset because of "
"data-pagespeed-no-transform attribute.-->");
// Note that here the regular rewrite images filter touches the srcset,
// and spams a whole bunch of debug comments. (It's also unable to rewrite
// b.png)
ValidateExpected(
"with_srcset",
"<img src=a.jpg width=100 height=100 srcset='a.jpg 1x, b.png 2x'>",
StrCat("<img src=", EncodeImage(100, 100, "a.jpg", "0", "jpg"),
" width=100 height=100 srcset='a.jpg 1x, b.png 2x'>"
" width=100 height=100 srcset='",
EncodeImage(-1, -1, "a.jpg", "0", "jpg"), " 1x, b.png 2x'>",
"<!--Image does not appear to need resizing.-->"
"<!--Image has no transparent pixels, is sensitive to "
"compression noise, and has no animation.-->"
"<!--Image does not appear to need resizing.-->"
"<!--Resized image from 1023x766 to 100x100-->"
"<!--ResponsiveImageFilter: Not adding srcset because image "
"already has one.-->"));
net/instaweb/rewriter/srcset_slot.cc
+7 -4
View File
@@ -38,7 +38,11 @@ namespace net_instaweb {
SrcSetSlotCollection::SrcSetSlotCollection(
RewriteDriver* driver, CommonFilter* filter, HtmlElement* element,
HtmlElement::Attribute* attribute)
: driver_(driver), element_(element), attribute_(attribute) {
: driver_(driver), element_(element), attribute_(attribute),
// Note: these need to be deep-copied in case we run as a detached
// rewrite, in which case element_ may be dead.
begin_line_number_(element->begin_line_number()),
end_line_number_(element->end_line_number()) {
StringPiece input(attribute->DecodedValueOrNull());
ParseSrcSet(input, &candidates_);
@@ -163,12 +167,11 @@ void SrcSetSlot::Render() {
}
GoogleString SrcSetSlot::LocationString() const {
HtmlElement* element = parent_->element();
return StrCat(parent_->driver()->id(), ": ",
"candidate image ", IntegerToString(index_), " of srcset at ",
IntegerToString(element->begin_line_number()),
IntegerToString(parent_->begin_line_number()),
"-",
IntegerToString(element->end_line_number()));
IntegerToString(parent_->end_line_number()));
}
} // namespace net_instaweb
pagespeed/automatic/system_tests/responsive_images.sh
+6
View File
@@ -16,3 +16,9 @@ fetch_until $URL 'grep -c srcset=' 2 # And only two srcsets (for Puzzle.jpg).
# Make sure all Puzzle URLs are rewritten.
fetch_until -save $URL 'grep -c [^x]Puzzle.jpg' 0
check egrep -q 'xPuzzle.jpg.pagespeed.+srcset="([^ ]*images/([0-9]+x[0-9]+)?xPuzzle.jpg.pagespeed.ic.[0-9a-zA-Z_-]+.jpg [0-9.]+x,?)+"' $FETCH_FILE
start_test rewrite_images can rewrite srcset itself
URL=$TEST_ROOT/image_rewriting/srcset.html?PageSpeedFilters=+rewrite_images,+debug
fetch_until -save $URL 'grep -c xPuzzle.*1x.*xCuppa.*2x' 1