Bug 47512 – Add support for decoding WebP image

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Pascal Massimino]

Created attachment 70489 [details]
add decoding support for webp image

Hi,

this patch adds a sub-class of ImageDecoder that can handle
decoding of WebP images, which consists of VP8-compressed
data inside a RIFF container. It uses the light-weight decoding
library available from http://www.webmproject.org/code/.
Support should be disabled all across the board for now
(thanks to ENABLE_WEBP=0) unless i overlooked something.

WebP images are partially recognized thanks to the presence
of "RIFF" signature in the first 4-bytes. This is far from
perfect but should rule out most of spurious instantiations
of the decoding object.

I tried to change every configuration files i thought were
relevant by getting inspiration from the jpeg case. Hope
i didn't miss some!

thanks for the review,
Pascal

[ more info at http://code.google.com/speed/webp ]

[Adam Barth]

Comment on attachment 70489 [details]
add decoding support for webp image

View in context: https://bugs.webkit.org/attachment.cgi?id=70489&action=review

> WebKit/WebCore/ChangeLog:7
> +        Add support for WebP image decoding in ImageDecoder
> +	using library libwebp-decode library available from
> +	http://www.webmproject.org/code/

These tabs are going to case problems when landing the patch.

> WebKit/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

Can we have tests?  (This line will prevent this patch from landing.)

> WebKit/WebCore/platform/image-decoders/ImageDecoder.cpp:82
> +#if ENABLE(WEBP)
> +    // WEBP
> +    if (!memcmp(contents, "RIFF", 4))
> +        return new WEBPImageDecoder(premultiplyAlpha);
> +#endif

This signature isn't very good.  What about other things in RIFF containers?  Also, magic strings that are entirely ASCII characters are undesirable.  Is there really no better magic number for webp?

> WebKit/WebCore/features.pri:71
> +!contains(DEFINES, ENABLE_WEBP=.): DEFINES += ENABLE_WEBP=1

ENABLE_WEBP=1 ?  From your bug comment, I would have expected ENABLE_WEBP=0.

[Mark Rowe (bdash)]

Comment on attachment 70489 [details]
add decoding support for webp image

View in context: https://bugs.webkit.org/attachment.cgi?id=70489&action=review

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:33
> +

ENABLE isn’t the appropriate type of test for code such as this.  USE is a better choice:


/* ==== Policy decision macros: these define policy choices for a particular port. ==== */

/* USE() - use a particular third-party library or optional OS service */
#define USE(WTF_FEATURE) (defined WTF_USE_##WTF_FEATURE  && WTF_USE_##WTF_FEATURE)

> WebKit/WebCore/Configurations/FeatureDefines.xcconfig:127
> +FEATURE_DEFINES = $(ENABLE_LINK_PREFETCH) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_3D_CANVAS) $(ENABLE_3D_RENDERING) $(ENABLE_BLOB) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CLIENT_BASED_GEOLOCATION) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DATALIST) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DIRECTORY_UPLOAD) $(ENABLE_DOM_STORAGE) $(ENABLE_EVENTSOURCE) $(ENABLE_FILTERS) $(ENABLE_FILE_SYSTEM) $(ENABLE_FULLSCREEN_API) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_IMAGE_RESIZER) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INPUT_SPEECH) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_MATHML) $(ENABLE_METER_TAG) $(ENABLE_NOTIFICATIONS) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_PROGRESS_TAG) $(ENABLE_RUBY) $(ENABLE_SHARED_WORKERS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WEB_SOCKETS) $(ENABLE_WEB_TIMING) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XHTMLMP) $(ENABLE_XPATH) $(ENABLE_XSLT) $(ENABLE_WEBP);

Turning on functionality that requires a third-party library when that third-party library is not available is a sure way to break the build.  Please don’t.  Also… as mentioned above, WebP isn’t a feature in the FEATURE_DEFINES sense.

[Alexey Proskuryakov]

> WebP images are partially recognized thanks to the presence
> of "RIFF" signature in the first 4-bytes. This is far from
> perfect but should rule out most of spurious instantiations
> of the decoding object.

This sounds quite far from perfect indeed - all RIFF files have this signature, <http://www.fileformat.info/format/riff/corion.htm>.

[Pascal Massimino]

Hi,

i'm working on modifying the code to adress the comments (ENABLE_WEBP=0, USE(), tabs, layout tests...).
I'll submit a revised patch soon.

Meanwhile i just want to rebound on the signature issue: i was told that one should use the minimal number of bytes that would rule out most of bad cases. Since most of other image formats were only using 4 bytes (and even, only 2 for BMP format!), i thought about using the same length and only probe for "RIFF". The real probing would use 12 bytes and check for "RIFF????WEBP", but i was under the
impression this was too long a look ahead to ask for. Isn't it?

thanks,
Pascal

[Adam Barth]

> Meanwhile i just want to rebound on the signature issue: i was told that one should use the minimal number of bytes that would rule out most of bad cases.

Who told you that?  :)

Sniffing is quite a subtle topic.  If you'd like a flavor for some of the issues, you might want to read this paper:

http://www.adambarth.com/papers/2009/barth-caballero-song.pdf

> Since most of other image formats were only using 4 bytes (and even, only 2 for BMP format!), i thought about using the same length and only probe for "RIFF". The real probing would use 12 bytes and check for "RIFF????WEBP", but i was under the
> impression this was too long a look ahead to ask for. Isn't it?

Nope, that's not too long.  Can you tell me more about the ???? bytes?  What do the represent / what values can they take on?  That signature is starting to look a bit better.  Ideally, we'd have some non-ASCII characters in there too.

[Pascal Massimino]

Adam,

in the "RIFF????WEBP" signature, the ???? represent the little-endian'd size
of the following data, starting at offset 8.
Hence, it can be anything.

Pascal

[Adam Barth]

> Hence, it can be anything.

That's too bad.  Can we change the WEBP string to something that has non-ASCII characters (ideally, illegal in Unicode too)?

[Peter Kasting]

To inject some clarity here:

Adam's concerns have nothing to do with ImageDecoder::create(), or in fact the IamgeDecoder at all, and everything to do with the format in general and the consequences for code that has to sniff bytestreams to determine what's coming down the pipe.  Since that means debating changes to the WebP container format, it probably should be resolved before finalizing the work, and it should probably be debated in a more dedicated forum than this bug.  I don't know what that is, maybe Pascal can suggest something.

When it comes to the narrowly-scoped issue of ImageDecoder::create(), it is indeed fine to use the minimum possible determinant string.  Note, for example, how we use "BM" to mean a .bmp.

[Adam Barth]

> When it comes to the narrowly-scoped issue of ImageDecoder::create(), it is indeed fine to use the minimum possible determinant string.  Note, for example, how we use "BM" to mean a .bmp.

It's important to use the same signature everywhere.  Historically, different sniffing code has used different signatures, even for well-established image formats, such as JPEG and GIF.  As a result, there have been lots of vulnerabilities related to sneaking bytes that one entity thinks are a GIF but another entity does not (some examples are described in the paper I linked to above).

Currently, there's an effort underway in the IETF to standardize the signatures used for the popular image formats (and some other formats).  That will hopefully help with some of the existing problems.

[Peter Kasting]

(In reply to comment #9)
> > When it comes to the narrowly-scoped issue of ImageDecoder::create(), it is indeed fine to use the minimum possible determinant string.  Note, for example, how we use "BM" to mean a .bmp.
> 
> It's important to use the same signature everywhere.

It's not going to be a huge deal to change this code.  But the code assumes that it sits underneath other sniffing/parsing code that has already made a determination that this bytestream should be fed to the ImageDecoder.  So it seems like the important bit is for that layer of code to obey the sniffing algorithm rules, and this code is an implementation detail behind it.

If I am wrong and somehow the return code here is fed back to the higher-level sniffing algorithm, i.e. this code is an active participant in sniffing, then I agree it's necessary for it to use the more detailed signatures.

[Pascal Massimino]

Mark, 
(In reply to comment #2)
> (From update of attachment 70489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70489&action=review
> 
> > WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:33
> > +
> 
> ENABLE isn’t the appropriate type of test for code such as this.  USE is a better choice:
> 
> 
> /* ==== Policy decision macros: these define policy choices for a particular port. ==== */
> 
> /* USE() - use a particular third-party library or optional OS service */
> #define USE(WTF_FEATURE) (defined WTF_USE_##WTF_FEATURE  && WTF_USE_##WTF_FEATURE)


i am a little lost finding what configuration file i should change to make use of this macro.
I see some WTF_FEATURE flags being defined in config.h, but that's probably not the file
i should be touching. Could you please point me to the files i should be updating? I've reverted
all the ENABLE_WEBP already.

thanks,
Pascal

 [...]

[Pascal Massimino]

Adam,

(In reply to comment #9)
> > When it comes to the narrowly-scoped issue of ImageDecoder::create(), it is indeed fine to use the minimum possible determinant string.  Note, for example, how we use "BM" to mean a .bmp.
> 
> It's important to use the same signature everywhere. 


There is a WebPGetInfo(*) function for validating the header in the library which is exactly meant
for that: central call point for sniffing data. I didn't use it here because, as said, it requires
30 bytes of data in order to go into great details validating everything that can be.
Should i use it instead (for instance, disguised as a static member bool WEBPDecoder::Validate(data, data_size)? 
I'd pretty much go advertising this function as the only one to call by sniffers.

thanks,
Pascal

(*) http://review.webmproject.org/gitweb?p=libwebp.git;a=blob;f=src/webp/decode.h;h=6ecaa00598db122489dbdc69207e93b8feb991ed;hb=HEAD

 Historically, different sniffing code has used different signatures, even for well-established image formats, such as JPEG and GIF.  As a result, there have been lots of vulnerabilities related to sneaking bytes that one entity thinks are a GIF but another entity does not (some examples are described in the paper I linked to above).
> 
> Currently, there's an effort underway in the IETF to standardize the signatures used for the popular image formats (and some other formats).  That will hopefully help with some of the existing problems.

[Adam Barth]

> It's not going to be a huge deal to change this code.  But the code assumes that it sits underneath other sniffing/parsing code that has already made a determination that this bytestream should be fed to the ImageDecoder.

I don't believe that's correct.  If you point an <img src="..."> at an URL, the bytes you get back to the server will come to this function.

Now, you might say that the result is indistinguishable because the image library will reject files that don't have the proper format, but that assumes the image library is correct and/or bug free.

> There is a WebPGetInfo(*) function for validating the header in the library which is exactly meant
> for that: central call point for sniffing data. I didn't use it here because, as said, it requires
> 30 bytes of data in order to go into great details validating everything that can be.

Unfortunately, this function won't be available everywhere that needs to sniff for WebP.  That's why we use signatures that boil down to a simple mask-and-compare.

> Should i use it instead (for instance, disguised as a static member bool WEBPDecoder::Validate(data, data_size)? 

It's better to use a standardized signature instead so that everyone who touches the byte stream agrees about whether or not it should be treated as WebP.

I don't mean to make a big deal out of the signature.  It's just a detail in the grand scheme of things, but it's a detail I'd like to get right before we paint ourselves into a corner.

[Peter Kasting]

(In reply to comment #13)
> > It's not going to be a huge deal to change this code.  But the code assumes that it sits underneath other sniffing/parsing code that has already made a determination that this bytestream should be fed to the ImageDecoder.
> 
> I don't believe that's correct.  If you point an <img src="..."> at an URL, the bytes you get back to the server will come to this function.
> 
> Now, you might say that the result is indistinguishable because the image library will reject files that don't have the proper format, but that assumes the image library is correct and/or bug free.

You're getting ahead of me.  I don't know what correct result you're expecting me to argue our behavior is indistinguishable from.

I thought the thrust of your concerns was the sniffing algorithm that determines a MIME type from a series of bytes.  The code in ImageDecoder is not determining a MIME type.  It is also not determining whether something is an image.  It is being called by code asserting that the bytestream in question _is_ an image, and being asked to best-effort decode it.  That's a different task entirely.  (One notable point is that we purposefully support certain invalid images so we can at least partially decode them.  Now, I don't believe any of the "invalid" things we allow happen to be changes in what you've marked as the signature bytes, but the point remains that the purpose of this code seems different than the purpose of code you care about.)

After that point, if you still insisted this code were subject to the comments in http://tools.ietf.org/html/draft-abarth-mime-sniff-05#section-6, I would say that it is the combined duty of the entire ImageDecoder subsystem to correctly validate images, and that which part is implemented in ImageDecoder::create() versus GIFImageDecoder.cpp versus GIFImageReader.cpp is not properly your concern as long as the result matches your criteria.  ...Which isn't really even a sane concept to discuss because there is far more to a valid image than just the first few signature bytes, so I'm not sure what "matches your criteria" even means.

You need to come by my desk and talk to me about the practical concerns you have because this conversation is not helping me understand them.

[Adam Barth]

... with my "spec lawyer" hat on:

http://www.whatwg.org/specs/web-apps/current-work/#the-img-element

"The user agents should apply the image sniffing rules to determine the type of the image, with the image's associated Content-Type headers giving the official type."

which refers to http://www.whatwg.org/specs/web-apps/current-work/#content-type-sniffing:-image

"The rules for sniffing images specifically and the rules for distingushing if a resource is text or binary are also defined in the Media Type Sniffing specification. Both sets of rules return a MIME type as their result. [MIMESNIFF]"

which refers to http://tools.ietf.org/html/draft-abarth-mime-sniff-05#section-6

so, at least from HTML5's point of view, the behavior of this code in specified by draft-abarth-mime-sniff.  Now, that document says:

   "User agents MAY support additional types if necessary, by implicitly
   adding to the above table."

which is what we're doing in this patch.  However, at some point, presuming WebP becomes popular, we'll want to add the WebP signature to that table.  We'll save ourselves a lot of pain later if we all use the same high-quality signature from the beginning.

I've been talking with Pascal directly about what might be a good signature to use.  Apparently, we can use RIFF????WEBPVP8\0x20 currently, which is probably ok.  I've asked whether we can tweak the format to be RIFF????WEBPVP8\0 (replacing the last space character with a null byte), which would be an improvement from a sniffing perspective.

[Pascal Massimino]

Created attachment 70801 [details]
updated patch

updated patch

 - remove ENABLE() in favor of USE()
 - changed the signature to RIFF????WEBPVP
 - added a DEPS to fetch libwebp/ library from chrome_svn
 - added layout test

binary files (.png, .webp) are following

[Pascal Massimino]

Created attachment 70802 [details]
fast/images/webp-image-decoding-expected.png

reference decoded layout for webp-image-decoding.html

[Pascal Massimino]

Created attachment 70803 [details]
image sample for layout test

image sample for webp-image-decode.html layout test

[Adam Barth]

Comment on attachment 70801 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70801&action=review

Mostly a bunch of nits.  I can clean them up as I land the patch.  I'm not super cheesed about the sniffing code, but I have a patch out for review that makes this code more sane.  I'll just roll a more sane version into that patch.

> WebKit/WebCore/platform/image-decoders/ImageDecoder.cpp:86
> +        unsigned length =
> +            copyFromSharedBuffer(header, webpExtraMarker, data,
> +                                 webpExtraMarkeroffset);

This should all be on one line.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:92
> +    // FIXME: support for progressive decoding.

Add support for ...

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:108
> +    if (!WebPDecodeBGRInto(data_uint8, data_size,
> +                           &rgb[0], height * stride, stride))

One line.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:118
> +            buffer.setRGBA(x, y,
> +                           src[bytesPerPixel * x + 2],
> +                           src[bytesPerPixel * x + 1],
> +                           src[bytesPerPixel * x + 0],
> +                           0xff);

One line.  WebKit never met a line that was too long.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:36
> +namespace WebCore {

Missing blank line after this line.  Also, code inside a namespace should not be indented.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:47
> +        // return false in case of decoding failure.       

return -> Returns

> WebKit/LayoutTests/fast/images/webp-image-decoding.html:3
> +<img src="resources/test.webp">

You'll probably need to add this test to a bunch of skipped lists since very few ports have support for this library.

[Adam Barth]

I should also say that Pascal and I discussed the sniffing signature issue in some detail.  There doesn't seem to be a way to get a non-ASCII character into the signature without digging far into the file or adding more bytes to every image.  That's somewhat unfortunate, however, I think the signature we have in this patch is probably going to be ok.

[Adam Barth]

This patch is incorrectly based.  Did you create it with webkit-patch upload ?

[Adam Barth]

Created attachment 70841 [details]
Mostly fixed patch

[Adam Barth]

So, I spent the last two hours fixing all the nit-picky WebKit process related stuff for this patch, but, in the end, I couldn't get the patch to work at all.  I don't really understand why.  I don't have any other way to view WebP images, so it's possible the test image is corrupt in some way.

In any case, I've attached a "fixed" (non-working) patch.  Hopefully that will be helpful.

[Adam Barth]

I think I figured out why the patch doesn't work.  Notice this comment in WebCore.gyp:

            # Skia image-decoders are also not used on mac.  CoreGraphics
            # is used directly instead.

I think that means we don't use this code on Mac, which means Chromium Mac wouldn't be able to decode WebP images.

[Peter Kasting]

(In reply to comment #24)
> I think that means we don't use this code on Mac, which means Chromium Mac wouldn't be able to decode WebP images.

That's correct.  This is a first step.  Decoding on Mac will be significantly trickier since we can't just go patch CG.  I recommended that whole patch and discussion wait until we have agreement on this one.

[Adam Barth]

> That's correct.  This is a first step.  Decoding on Mac will be significantly trickier since we can't just go patch CG.  I recommended that whole patch and discussion wait until we have agreement on this one.

Oh, I think we have agreement on this one.  I'll try landing it from my linux box.

[Adam Barth]

> I'll try landing it from my linux box.

Doesn't work on Linux either.  Maybe WebP isn't actually enabled?  In any case, I'm done trying to get it to work.

[Pascal Massimino]

Created attachment 70906 [details]
updated patch with WEBP=1 enabled in webkit/chromium and skip-test entry for MAC

Adam,

here is the updated patch. Compilation is always
on now, for Webkit/chromium.

thanks!
Pascal

[Adam Barth]

Comment on attachment 70906 [details]
updated patch with WEBP=1 enabled in webkit/chromium and skip-test entry for MAC

ok.  I bet we'll need to add this test to more skipped lists though.  Also, we probably don't need webp-image-decoding-expected.png.  The test is a text test.

[Adam Barth]

Comment on attachment 70906 [details]
updated patch with WEBP=1 enabled in webkit/chromium and skip-test entry for MAC

I suspect this won't land as written, but we can give it a spin.  If the automatic commit fails, we'll either need a patch that can land (e.g., with the necessary changes to the skipped lists) or someone will need to land it by hand.

[WebKit Commit Bot]

Comment on attachment 70906 [details]
updated patch with WEBP=1 enabled in webkit/chromium and skip-test entry for MAC

Rejecting patch 70906 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70906]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=70906&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47512&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 70906 from bug 47512.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4442063

[Pascal Massimino]

(In reply to comment #31)
> (From update of attachment 70906 [details])
> Rejecting patch 70906 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70906]" exit_code: 2
> Cleaning working directory
> Updating working directory
> Logging in as [e-mail]...
> Fetching: https://bugs.webkit.org/attachment.cgi?id=70906&action=edit
> Fetching: https://bugs.webkit.org/show_bug.cgi?id=47512&ctype=xml
> Processing 1 patch from 1 bug.
> Processing patch 70906 from bug 47512.
> Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/4442063

Adam,

could it be it failed because of the missing binary blob for 'test.webp (and also the unneeded
webp-image-decoding-expected.png) ?

And: what about the "OOPS!" comment still present in the ChangeLog patches?
Is is a problem for the scripts?

thanks!
Pascal

[Adam Barth]

> could it be it failed because of the missing binary blob for 'test.webp (and also the unneeded
> webp-image-decoding-expected.png) ?

Possibly.  It's also likely it failed because of the change to test_expectations.txt.  That file has a lot of churn, especially at the end.

> And: what about the "OOPS!" comment still present in the ChangeLog patches?
> Is is a problem for the scripts?

Nope, the tools know how to replace that text with the real reviewer.

These problem are all easily solvable by a committer who's interested in landing your patch by hand.

[Pascal Massimino]

Created attachment 71002 [details]
updated patch against fresh checkout, for bot to test on all platforms

Hi,

trying to submit this patch to the commit-queue, see if it applies cleanly.
I removed the test_expectations part.
The new 'test.webp' binary file might not be patched in correctly...

thanks!
Pascal

[WebKit Review Bot]

Attachment 71002 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:81:  data_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:82:  data_uint8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 71002 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4498013

[Pascal Massimino]

Created attachment 71004 [details]
new patch with fixed style

Update from 71002, hopefully fixed.
Testing on the commit-queue.

[Adam Barth]

Comment on attachment 71004 [details]
new patch with fixed style

Ok.  Let's try landing this.

[Adam Barth]

Committed r69942: <http://trac.webkit.org/changeset/69942>

[WebKit Review Bot]

http://trac.webkit.org/changeset/69942 might have broken Qt Linux Release

[Adam Barth]

More skipping in http://trac.webkit.org/changeset/69945

[Adam Barth]

I'm not entirely sure how to read the dashboard, but it looks like this patch doesn't work on Chromium Linux either:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&useWebKitCanary=true&tests=fast%2Fimages%2Fwebp-image-decoding.html%2Cfast%2Fjs%2Fbreak-ASI.html%2Csvg%2Fdynamic-updates%2FSVGFEFloodElement-dom-flood-opacity-attr.html%2Csvg%2Fdynamic-updates%2FSVGFEFloodElement-svgdom-flood-opacity-prop.html

[James Robinson]

Yeah, this test is not passing on chromium on any platform.

[Dimitri Glazkov (Google)]

(In reply to comment #43)
> Yeah, this test is not passing on chromium on any platform.

Roll out?

[Adam Barth]

(In reply to comment #44)
> (In reply to comment #43)
> > Yeah, this test is not passing on chromium on any platform.
> 
> Roll out?

Seems to passing on build.webkit.org:

http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r69982%20(6951)/results.html

Maybe we just need to enable the feature downstream?

[James Robinson]

WTF_USE_WEBP=1 is being set in WebKit/chromium/features.gypi, which means that chromium's DumpRenderTree is built with that macro set.  However in chromium land src/build/feature_overrides.gypi does not set WTF_USE_WEBP at all, which means that test_shell (and chrome) are being built with WEBP turned off at compile time.

This is bad - we should be testing with the same build configuration that we ship, ideally.  Pascal, do you plan to turn WEBP on for all of chrome (via feature_overrides.gypi)?  If it's not ready to turn on yet for whatever reason we should probably turn it off for DRT as well, or perhaps control it via a runtime flag rather than a compile-time flag.

[Pascal Massimino]

Hi,

thanks Adam for landing this patch!

(In reply to comment #46)
> WTF_USE_WEBP=1 is being set in WebKit/chromium/features.gypi, which means that chromium's DumpRenderTree is built with that macro set.  However in chromium land src/build/feature_overrides.gypi does not set WTF_USE_WEBP at all, which means that test_shell (and chrome) are being built with WEBP turned off at compile time.
> 
> This is bad - we should be testing with the same build configuration that we ship, ideally.  Pascal, do you plan to turn WEBP on for all of chrome (via feature_overrides.gypi)?  If it's not ready to turn on yet for whatever reason we should probably turn it off for DRT as well, or perhaps control it via a runtime flag rather than a compile-time flag.

James tested the feature_overrides.gypi change and verified that it makes the test_shell test pass. Further verifications are ongoing at Chromium level. I think we're done for the modifs at Webkit level for now. Thanks a lot!