Summary: | Add support for decoding WebP image | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pascal Massimino <pascal.massimino> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, fishd, gustavo, jamesr, pkasting, skyul, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 105915 | ||||||||||||||||||||
Attachments: |
|
Description
Pascal Massimino
2010-10-11 16:04:58 PDT
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. 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. > 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>. 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 > 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. 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 > 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)?
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. > 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.
(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. 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 [...] 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. > 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. (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. ... 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. 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
Created attachment 70802 [details]
fast/images/webp-image-decoding-expected.png
reference decoded layout for webp-image-decoding.html
Created attachment 70803 [details]
image sample for layout test
image sample for webp-image-decode.html layout test
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. 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. This patch is incorrectly based. Did you create it with webkit-patch upload ? Created attachment 70841 [details]
Mostly fixed patch
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. 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. (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. > 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.
> 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.
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
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.
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.
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 commit-queue@webkit.org... 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 (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 commit-queue@webkit.org... > 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 > 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. 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
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.
Attachment 71002 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4498013 Created attachment 71004 [details]
new patch with fixed style
Update from 71002, hopefully fixed.
Testing on the commit-queue.
Comment on attachment 71004 [details]
new patch with fixed style
Ok. Let's try landing this.
Committed r69942: <http://trac.webkit.org/changeset/69942> http://trac.webkit.org/changeset/69942 might have broken Qt Linux Release More skipping in http://trac.webkit.org/changeset/69945 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 Yeah, this test is not passing on chromium on any platform. (In reply to comment #43) > Yeah, this test is not passing on chromium on any platform. Roll out? (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? 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. 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! |