1294490 - (WebP) Implement WebP image support

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[Andrew Osmond [:aosmond] (he/him)]

Implement WebP decoding, pref'd off by default.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder and tests.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Implement telemetry for WebP decoder.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd6a53932b35

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v2

Shrink the copied libwebp tree significantly so that we only take what we need.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v2

Add script to facilitate updating libwebp.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder and tests. v2

Refresh.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Implement telemetry for WebP decoder. v2

Refresh.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Fix Android builds.

Should fix the Android build.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c5442e251c

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

So this bug inherits the "WebP" alias from bug 856375 :)

Comment 13

[Ralph Corderoy]

Perhaps the CC list from the old bug can be copied to this one so the dribble of "CC added" emails stops before another 180 have turned up.

Comment 14

[Dave Garrett]

(In reply to Ralph Corderoy from comment #13)
> Perhaps the CC list from the old bug can be copied to this one so the
> dribble of "CC added" emails stops before another 180 have turned up.

Mass adding CCs is rather bad Bugzilla etiquette. If they wish to CC, it's up to them. Voting is also another way to track here, which is less spamy. If bugspam from CC adds is something you'd rather not get, go to your preferences here and disable it (at least for bugs other than your own). Interact with enough bugs here and it's eventually a necessity.

That said, this is off-topic, and this bug is now for implementation. Marking comment and this reply as off-topic.

Comment 15

[Mark Straver]

If you want to do this, then you have to make sure you advertise WebP support in your request headers for images, otherwise your usage (and resulting telemetry) will be off (servers won't send WebP unless they know it's supported by the client).

Suggested:
pref("image.http.accept","image/webp,image/png,image/*;q=0.8,*/*;q=0.5");

Comment 16

[sjw]

This would conflict with bug 572650.
Web Devs should use features like <source>/srcset.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

(In reply to sjw from comment #16)
> This would conflict with bug 572650.
> Web Devs should use features like <source>/srcset.

It wouldn't give out more entropy if all Firefoxes from a certain release on would send that out, no?

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v3

Remove src/dsp/cpu.c as we implement the necessary symbols ourselves.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v3

Fix Android builds by replacing src/dsp/cpu.c with src/moz/cpu.cpp which controls if NEON/SSE/etc is used.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder and tests. v3

Fix handling of non-0xFF alpha if using SurfaceFormat::B8G8R8X8.

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v1

Add support for build switch --with-system-webp to mirror other image decoder libraries.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Mark Straver from comment #15)
> If you want to do this, then you have to make sure you advertise WebP
> support in your request headers for images, otherwise your usage (and
> resulting telemetry) will be off (servers won't send WebP unless they know
> it's supported by the client).
> 
> Suggested:
> pref("image.http.accept","image/webp,image/png,image/*;q=0.8,*/*;q=0.5");

Mark, in this initial set of patches, WebP support is disabled by default (image.webp.enabled pref in part 3). I imagine we would turn it on in a follow up bug when ready for primetime, and we can consider if we want to favour webp images then :).

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97f5e4cc98a4

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Implement telemetry for WebP decoder. v3

Refresh against tree.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v2

Add library version checks.

Comment 26

[Mark Straver]

(In reply to Andrew Osmond [:aosmond] from comment #22)
> I imagine we would turn it on in a
> follow up bug when ready for primetime, and we can consider if we want to
> favour webp images then :).

Of course! I'm just making a point that servers who serve WebP in general will NOT do this unless you have the support explicitly listed in the http-accept header (to prevent web compat issues). If you don't include this header (I suggest you link it to the pref for enabling WebP, in that case), the WebP support won't get much exposure at all in your testing and you'd have a mostly untested (in the wild) image support until you consider it "ready for prime time".

You can do what you want of course, but I think it'd be smart to get as much testing exposure as possible here considering image support is pivotal for the web.

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Detect WebP MIME type from content sniffing. v1

Add ability to detect a WebP image from the content rather than relying solely on the extension. Add test cases for content sniffing.

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8792064 [details] [diff] [review]
Part 1. Add libwebp to source tree. v3

I expect this one is a rubber stamp given it is *just* an unmodified subset of libwebp 1.5.1 source code :).

Comment 29

[Gervase Markham [:gerv]]

(In reply to Mark Straver from comment #26)
> Of course! I'm just making a point that servers who serve WebP in general
> will NOT do this unless you have the support explicitly listed in the
> http-accept header (to prevent web compat issues). 

Can you name some popular WebP-serving servers which make the decision based on the Accept: header? If there are high-traffic sites out there which do this, it may be worth it. But this form of content negotiation is basically dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good evidence to add it to the Accept: header.

Gerv

Comment 30

[Patrick McManus [:mcmanus]]

(In reply to Gervase Markham [:gerv] from comment #29)
\
> this, it may be worth it. But this form of content negotiation is basically
> dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good
> evidence to add it to the Accept: header.

factually that is not true that conneg is dead - the reference is an opinion piece and I've disagreed strongly in the past and still do.

content negotiation is used for webp (https://www.igvita.com/2013/05/01/deploying-webp-via-accept-content-negotiation/), and (relatedly) brotli and makes a lot of sense for incremental deployment of new encodings.

with webp in particular, this is the expected way to do it (and is chrome compat - which should always be a goal right now) and I would rather strongly want us to add it.

Comment 31

[Gervase Markham [:gerv]]

I agree conneg has a shot at working if everyone so far who's implemented the standard has simultaneously implemented the header, and if sites are therefore using it for detection. If that's true, I have no objection to adding it.

Are we planning to register ourselves as the default WebP viewer if nothing else is registered? This is to support the "Save As..." use case.

Gerv

Comment 32

[Patrick McManus [:mcmanus]]

(In reply to Gervase Markham [:gerv] from comment #31)

> Are we planning to register ourselves as the default WebP viewer if nothing
> else is registered? This is to support the "Save As..." use case.
> 
> Gerv

I have no idea how to do that.. any idea who to ni?

btw - great to see this feature.

Comment 33

[Yusuf Goolamabbas]

(In reply to Gervase Markham [:gerv] from comment #29)
> (In reply to Mark Straver from comment #26)
> > Of course! I'm just making a point that servers who serve WebP in general
> > will NOT do this unless you have the support explicitly listed in the
> > http-accept header (to prevent web compat issues). 
> 
> Can you name some popular WebP-serving servers which make the decision based
> on the Accept: header? If there are high-traffic sites out there which do
> this, it may be worth it. But this form of content negotiation is basically
> dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good
> evidence to add it to the Accept: header.
> 
> Gerv

Gerv, I've seen a few blog posts describing how to configure nginx to serve webp conditionally using the Accept header

http://www.lazutkin.com/blog/2014/02/23/serve-files-with-nginx-conditionally/
https://optimus.keycdn.com/support/configuration-to-deliver-webp/

Comment 34

[Daniel Mota Leite]

On my company, we started to use thumbor to resize and optimize images and it have one option to return webp if the browser announce that support it.

So is chrome request a test.jpg file, thumbor will fetch the image, check if webp is supported and if yes, it will deliver the webp format for that url. If no support is announce, if will send the jpg format.

The idea is that 10% in many images will save a lot bandwidth in the end.

Until support is very common, i would say that announce the support is a good idea

Comment 35

[Gervase Markham [:gerv]]

rstrong fixed bug 452254, so I suspect he knows how to register for filetypes in Windows :-)

Gerv

Comment 36

[Mark Straver]

(In reply to Gervase Markham [:gerv] from comment #29)
> Can you name some popular WebP-serving servers which make the decision based
> on the Accept: header? If there are high-traffic sites out there which do
> this, it may be worth it. But this form of content negotiation is basically
> dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good
> evidence to add it to the Accept: header.

I know at least some CDNs do this based on image/webp announcement of support. e.g. cloudinary does this. Personally I also noticed this on start.me. Not in the habit of checking this for sites visited though, but it seems to be at least a growing trend for bandwidth reasons.

Comment 37

[Robert Strong (they/them - no direct email)]

(In reply to Gervase Markham [:gerv] from comment #35)
> rstrong fixed bug 452254, so I suspect he knows how to register for
> filetypes in Windows :-)
Yep. There is also bug 1197191.

Comment 38

[Ilya Grigorik]

(In reply to Patrick McManus [:mcmanus] from comment #30)
> (In reply to Gervase Markham [:gerv] from comment #29)
> \
> > this, it may be worth it. But this form of content negotiation is basically
> > dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good
> > evidence to add it to the Accept: header.
> 
> factually that is not true that conneg is dead - the reference is an opinion
> piece and I've disagreed strongly in the past and still do.
> 
> content negotiation is used for webp
> (https://www.igvita.com/2013/05/01/deploying-webp-via-accept-content-
> negotiation/), and (relatedly) brotli and makes a lot of sense for
> incremental deployment of new encodings.
> 
> with webp in particular, this is the expected way to do it (and is chrome
> compat - which should always be a goal right now) and I would rather
> strongly want us to add it.

Strong +1 to all of the above.

In addition to the sites and CDNs already mentioned above, I'll also add:
- Lots of Google properties rely on Accept headers. Not all, but we're filing bugs as we find them.
- ^ Edge team helped us find many of these edge cases within Google and across the web at large -- their UA string "misled" lots of UA sniffing sites to serve WebP and they did a lot of outreach work to fix that and drive use of Accept. For example, Pagespeed: http://www.christianheilmann.com/2015/06/08/ua-sniffing-issue-outdated-mod-pagespeed-sending-webp-images-to-microsoft-edge/

In short, please advertise "image/webp" in Accept.

Comment 39

[Karl Dubost💡 :karlcow]

We had an issue with a Web site and WebP after digging into the code. We found out that the site was using PageSpeed module on Apache. So the module does indeed Content-Negotiation for WebP AND user-agent sniffing. See Bug 1222509

And yes I can confirm that user agent sniffing is always a kind of future-fail strategy. It requires Web sites to adjust their libraries with new versions and sometimes it is just not happening for budget, schedule, non-maintenance, etc.

Comment 40

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792079 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder. v3

Review of attachment 8792079 [details] [diff] [review]:
-----------------------------------------------------------------

I'll let Mike take the build review

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792509 [details] [diff] [review]
Part 6. Detect WebP MIME type from content sniffing. v1

Review of attachment 8792509 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/imgLoader.cpp
@@ +2515,5 @@
>                                !memcmp(aContents, "\000\000\002\000", 4))) {
>      aContentType.AssignLiteral(IMAGE_ICO);
>  
> +  // WebPs always begin with RIFF, a 32-bit length, and WEBP.
> +  } else if (aLength >= 12 && !nsCRT::strncmp(aContents, "RIFF", 4) &&

It seems like using memcpy is here is more appropriate than strncmp

Comment 42

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792064 [details] [diff] [review]
Part 1. Add libwebp to source tree. v3

Review of attachment 8792064 [details] [diff] [review]:
-----------------------------------------------------------------

What was the reason for to using our cpu detection vs theirs?

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #42)
> Comment on attachment 8792064 [details] [diff] [review]
> Part 1. Add libwebp to source tree. v3
> 
> Review of attachment 8792064 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What was the reason for to using our cpu detection vs theirs?

It doesn't build on Android because it was missing the android headers, presumably because we don't link in that dependency. All other uses in gecko were converted to use our headers for ARM, however the mozilla/arm.h header uses C++ namespaces, so I can't simply just include it into the cpu.c file. I can probably work around that if preferred, but just replacing the file seemed easier since it was so simple.

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792067 [details] [diff] [review]
Part 3. Implement WebP decoder and tests. v3

Review of attachment 8792067 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsWebPDecoder.cpp
@@ +119,5 @@
> +  }
> +
> +  // Decoding animations is currently unsupported
> +  if (features.has_animation) {
> +    return Transition::TerminateFailure();

Shouldn't we just not decode the animation instead of failing to decode completely?

@@ +191,5 @@
> +  }
> +
> +  for (int row = mLastRow; row < lastRow; row++) {
> +    const uint8_t* src = rowStart + row * stride;
> +    auto result = mPipe.WritePixelsToRow<uint32_t>([&]() -> NextPixel<uint32_t> {

We should decide the transparency of the image purely on the metadata and not worry about trying to detect it based on the image contents. I don't think it's worth the performance cost of the detection vs the savings if images say they are opaque and are actually not.

Comment 45

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8792066 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding. v3

Review of attachment 8792066 [details] [diff] [review]:
-----------------------------------------------------------------

It's probably worth adding a comment to cpu.cpp with some information about "Fix Android builds by replacing src/dsp/cpu.c with src/moz/cpu.cpp which controls if NEON/SSE/etc is used."

Comment 46

[Vittorio Giovara]

Any reason for not using the ffmpeg/libav webp decoder, instead of pulling in another dependency?

In other words, what features would you like to have implemented in order to use it?

Comment 47

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #44)
> Comment on attachment 8792067 [details] [diff] [review]
> Part 3. Implement WebP decoder and tests. v3
> 
> Review of attachment 8792067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/decoders/nsWebPDecoder.cpp
> @@ +119,5 @@
> > +  }
> > +
> > +  // Decoding animations is currently unsupported
> > +  if (features.has_animation) {
> > +    return Transition::TerminateFailure();
> 
> Shouldn't we just not decode the animation instead of failing to decode
> completely?
> 

WebPIAppend will return VP8_STATUS_UNSUPPORTED_FEATURE if we attempt to decode animated images. We would need to add explicit support using the animated API. I'll look at how much work that is to see if it is a Part 7 or a new bug.

> @@ +191,5 @@
> > +  }
> > +
> > +  for (int row = mLastRow; row < lastRow; row++) {
> > +    const uint8_t* src = rowStart + row * stride;
> > +    auto result = mPipe.WritePixelsToRow<uint32_t>([&]() -> NextPixel<uint32_t> {
> 
> We should decide the transparency of the image purely on the metadata and
> not worry about trying to detect it based on the image contents. I don't
> think it's worth the performance cost of the detection vs the savings if
> images say they are opaque and are actually not.

If we use B8G8R8X8 because the header said it is opaque, and libwebp lied, whether that be due to an encoding error, bug or maliciously crafted image, we will end up having non-0xFF alphas which will trip skia up.

Comment 48

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Vittorio Giovara from comment #46)
> Any reason for not using the ffmpeg/libav webp decoder, instead of pulling
> in another dependency?
> 
> In other words, what features would you like to have implemented in order to
> use it?

A good question, one I don't have a good answer for right now. We don't use ffmpeg/libav for any of the image decoding right now, not just WebP, and our forks of said code in the repo appear to be built with minimal feature sets.

Comment 49

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andrew Osmond [:aosmond] from comment #47)
> 
> If we use B8G8R8X8 because the header said it is opaque, and libwebp lied,
> whether that be due to an encoding error, bug or maliciously crafted image,
> we will end up having non-0xFF alphas which will trip skia up.

I think we can assume that libwebp will always return appropriate data in the alpha bytes. Chrome seems to make this assumption. If for some reason this assumption is not true, I'd rather fix upstream.

Comment 50

[Mark Straver]

AFAIK libwebp always returns and alpha channel (it exclusively works in RGBA) with opaque images simply having alpha always set to 255. Treat all WebP decoded images as having transparency, and you should be set?

Comment 51

[Mike Hommey [:glandium]]

Comment on attachment 8792066 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding. v3

Review of attachment 8792066 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/moz.build
@@ +35,5 @@
>  
>  if not CONFIG['MOZ_SYSTEM_PNG']:
>      external_dirs += ['media/libpng']
>  
> +if not CONFIG['MOZ_SYSTEM_WEBP']:

MOZ_SYSTEM_WEBP is not defined anywhere

::: media/libwebp/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['src']

There's a level of indirection here that doesn't seem very useful. Either move media/libwebp/src/* to media/libwebp, or add media/libwebp/src to external_dirs instead of media/libwebp

::: media/libwebp/src/dsp/moz.build
@@ +31,5 @@
> +    'yuv_sse2.c',
> +]
> +
> +if CONFIG['CPU_ARCH'] == 'arm' and CONFIG['BUILD_ARM_NEON']:
> +    SOURCES['dec_neon.c'].flags += ['-mfpu=neon']

CONFIG['NEON_FLAGS'] instead of ['-mfpu=neon']

Comment 52

[Mike Hommey [:glandium]]

Comment on attachment 8792080 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v2

Review of attachment 8792080 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +2299,5 @@
> +dnl ========================================================
> +dnl system WebP Support
> +dnl ========================================================
> +MOZ_ARG_WITH_STRING(system-webp,
> +[  --with-system-webp[=PFX]

libwebp comes with pkg-config files, so it would be better to use PKG_CHECK_MODULES. See e.g. --with-system-libvpx.

Even better, this should go into toolkit/moz.configure and use python checks instead of autoconf. See build/moz.configure/ffi.configure

@@ +2313,5 @@
> +fi
> +if test -z "$WEBP_DIR" -o "$WEBP_DIR" = no; then
> +    MOZ_SYSTEM_WEBP=
> +else
> +    AC_CHECK_LIB(webp, WebPINewDecoder, [MOZ_SYSTEM_WEBP=1 MOZ_WEBP_LIBS="-lwebp"],

With a PKG_CHECK_MODULES, you could skip this.

@@ +2318,5 @@
> +    [MOZ_SYSTEM_WEBP= MOZ_WEBP_CFLAGS= MOZ_WEBP_LIBS=])
> +fi
> +if test "$MOZ_SYSTEM_WEBP" = 1; then
> +    AC_TRY_COMPILE([ #include <webp/decode.h> ],
> +                   [ #if WEBP_ABI_IS_INCOMPATIBLE(WEBP_DECODER_ABI_VERSION, $MOZWEBP)

Why care about ABI compatibility? If you build against a given version, as long as the *API* is compatible, there's no problem. It /seems/ webp is guaranteeing API stability, so it looks to me the whole ABI compat block is unnecessary.

Comment 53

[Timur Davletshin]

BTW, Palemoon supports WebP for several versions already.

Comment 54

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v4 [carries r=jmuizelaar]

Relocates source files from media/libwebp/src to media/libwebp and adds files required for animated image decoding.

Comment 55

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v4

Incorporate feedback from comment 45 and comment 51. Adds building animated image support as well.

Comment 56

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v4

Incorporate feedback from comment 44. Add animated image support. Broke out test additions into a separate patch as it is growing.

WIP as it appears to suffer from intermittent decode failures for animated WebP images when visiting http://cloudinary.com/blog/animated_webp_how_to_convert_animated_gif_to_webp_and_save_up_to_90_bandwidth which a refresh solves. Still investigating.

Comment 57

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Implement telemetry for WebP decoder. v4 [carries r=jmuizelaar]

Refresh against other patches.

Comment 58

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v3

Incorporate feedback from comment 52. Add linking in for libwebpdemux as well, required for animated images.

Comment 59

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Detect WebP MIME type from content sniffing. v2 [carries r=jmuizelaar]

Incorporated feedback from comment 41.

Comment 60

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Implement WebP tests. v1

Broken out of Part 3. Added gtests and mochitests for animated images.

Comment 61

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8795740 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder. v4 [carries r=jmuizelaar]

Carry r+ lost in patch update collision :).

Comment 62

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v5

Fix a bug where we successfully parsed the WebP header but we did not get enough data to begin parsing first frame for animated images when decoding the metadata.

Comment 63

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v6

Some minor refactoring to clean up return values of ReadMultiple/ReadSingle.

Comment 64

[Mike Hommey [:glandium]]

Comment on attachment 8795741 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v3

Review of attachment 8795741 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/webp.configure
@@ +14,5 @@
> +
> +system_webpdemux = pkg_check_modules('MOZ_WEBPDEMUX', 'libwebpdemux >= 0.5.1',
> +                                     when=use_system_webp)
> +
> +set_config('MOZ_SYSTEM_WEBP', system_webp and system_webpdemux)

While this is likely something that will work in the future, this is not supported as of writing and won't do the right thing. You need something like:
depends(system_webp, system_webpdemux)(lambda a,b: a and b)

That being said, the libwebpdemux pkg-config file has a "Requires: libwebp", so checking both is kind of redundant, and will lead to duplicate flags between MOZ_WEBP_* and MOZ_WEBPDEMUX_*.

But you can also check for both in one pkg_check_modules call:
pkg_check_modules('MOZ_WEBP', 'libwebp >= 0.5.1 libwebpdemux >= 0.5.1')

::: moz.configure
@@ +169,5 @@
>      return backends
>  
>  set_config('BUILD_BACKENDS', build_backends)
>  
> +include('build/moz.configure/webp.configure')

Cf. previous review, this should go into toolkit/moz.configure (and no need to include a separate file)

Comment 65

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v4

(In reply to Mike Hommey [:glandium] from comment #64)
> Comment on attachment 8795741 [details] [diff] [review]
> Part 5. Add --with-system-webp switch to build. v3
> 
> Review of attachment 8795741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/moz.configure/webp.configure
> @@ +14,5 @@
> > +
> > +system_webpdemux = pkg_check_modules('MOZ_WEBPDEMUX', 'libwebpdemux >= 0.5.1',
> > +                                     when=use_system_webp)
> > +
> > +set_config('MOZ_SYSTEM_WEBP', system_webp and system_webpdemux)
> 
> While this is likely something that will work in the future, this is not
> supported as of writing and won't do the right thing. You need something
> like:
> depends(system_webp, system_webpdemux)(lambda a,b: a and b)
> 
> That being said, the libwebpdemux pkg-config file has a "Requires: libwebp",
> so checking both is kind of redundant, and will lead to duplicate flags
> between MOZ_WEBP_* and MOZ_WEBPDEMUX_*.
> 
> But you can also check for both in one pkg_check_modules call:
> pkg_check_modules('MOZ_WEBP', 'libwebp >= 0.5.1 libwebpdemux >= 0.5.1')
> 

Done.

> ::: moz.configure
> @@ +169,5 @@
> >      return backends
> >  
> >  set_config('BUILD_BACKENDS', build_backends)
> >  
> > +include('build/moz.configure/webp.configure')
> 
> Cf. previous review, this should go into toolkit/moz.configure (and no need
> to include a separate file)

My bad, done.

Comment 66

[Mike Hommey [:glandium]]

Comment on attachment 8800624 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v4

Review of attachment 8800624 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/library/moz.build
@@ +233,5 @@
>      OS_LIBS += CONFIG['MOZ_PNG_LIBS']
>  
> +if CONFIG['MOZ_SYSTEM_WEBP']:
> +    OS_LIBS += CONFIG['MOZ_WEBP_LIBS']
> +    OS_LIBS += CONFIG['MOZ_WEBPDEMUX_LIBS']

There is no MOZ_WEBPDEMUX_LIBS anymore.

::: toolkit/moz.configure
@@ +769,5 @@
> +
> +use_system_webp = depends_if('--with-system-webp')(lambda _: True)
> +
> +system_webp = pkg_check_modules('MOZ_WEBP', 'libwebp >= 0.5.1 libwebpdemux >= 0.5.1',
> +                                when=use_system_webp)

when='--with-system-webp' should work. In which case you don't need the depends_if above.

Comment 67

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v5

Incorporate latest feedback and fix to work with latest build changes.

Comment 68

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v7

Rebase against tree.

Comment 69

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Implement WebP tests. v2

Rebase against tree.

Comment 70

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v5 [carries r=jmuizelaar]

Upgrade to libwebp-0.5.2.

Comment 71

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v5 [carries r=glandium]

Update MOZCHANGES to reflect lib upgrade.

Comment 72

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Implement WebP tests. v3

Refresh against tree.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10b72bdbd8b8bc148d8f1da25930fe387cbe8a75

Comment 73

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v6

Update version check.

Comment 74

[Stijn de Witt]

Great to see you guys hard at work at adding in support for WebP! I can't wait for the day we'll get better images on the web.

I have been googling around for a/the official vision of the Firefox team about WebP support in Firefox... Is there one and if so could you post a link? From my current understanding the work you guys are doing here will end up behind a flag and be disabled by default is that true? Thanks for all your hard work!

Comment 75

[xzyfer]

The closest thing I'm aware of is https://platform-status.mozilla.org/ which appears to be not be actively kept up to date.

Comment 76

[Stijn de Witt]

Unfortunately it gives no results for 'webp'. 
I think it would be good if Mozilla put out a statement on the current status of their ideas on webp. I have read through a lot of info online, but it seems Mozilla has changed it's stance on WebP multiple times and I have a hard time separating the history from the current situation.

Comment 77

[Milan Sreckovic [:milan] (needinfo for best results)]

Mozilla wants to do what's good for the web.  An image format that only some browsers support is not, so WebP support in Firefox is as much of a question of WebP support in Edge/Safari, though things are never that simple.

Comment 78

[Maverick]

I'm all in favor of WebP (as much as i was in favor of Apng) because we really need a better-than-gif image for the web of today.

What i do have as a concern is in fact 
"what's the cost (in terms of privacy) of having an image format that is mainly developed by company_name_here"?

Imagine this horror movie à-là-hollywood were Mozilla also adopts webp: Most surely the biggest resistance to it's adoption will be gone. We'll end up having a file format that everyone uses for *everything* (animated or not) and then tomorrow Google decides to change the spec (or something else) and adds some way of tracking or some other thing not so good to monetize it... I'm not sure this is technically possible but if it is, how can we, then, solve the problem?

Don't get me wrong: i'm not one of those guys that say that google is evil! I really don't believe that. They are a company that, like any other for-profit company, needs to make money to survive.


So, while i'm all in favor of webp, i started to think twice about it... If there is no problem whit this, then go for it!

Comment 79

[Luke]

> Google decides to change the spec (or something else) and adds some way of tracking or some other thing not so good to monetize it... I'm not sure this is technically possible but 


The bug tracker is not the appropriate place for story telling and imaginary scenarios. None of your concerns are relevant to the spec as it stands today. The forum is the appropriate medium for this type of discussion. Not here.

Comment 80

[Mike Hommey [:glandium]]

Comment on attachment 8823667 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v6

Review of attachment 8823667 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/moz.configure
@@ +849,5 @@
> +
> +system_webp = pkg_check_modules('MOZ_WEBP', 'libwebp >= 0.5.2 libwebpdemux >= 0.5.2',
> +                                when='--with-system-webp')
> +
> +set_config('MOZ_SYSTEM_WEBP', depends_if(system_webp)(lambda _: True))

set_config('MOZ_SYSTEM_WEBP', system_webp) should work.

Comment 81

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Mike Hommey [:glandium] from comment #80)
> Comment on attachment 8823667 [details] [diff] [review]
> Part 5. Add --with-system-webp switch to build. v6
> 
> Review of attachment 8823667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/moz.configure
> @@ +849,5 @@
> > +
> > +system_webp = pkg_check_modules('MOZ_WEBP', 'libwebp >= 0.5.2 libwebpdemux >= 0.5.2',
> > +                                when='--with-system-webp')
> > +
> > +set_config('MOZ_SYSTEM_WEBP', depends_if(system_webp)(lambda _: True))
> 
> set_config('MOZ_SYSTEM_WEBP', system_webp) should work.

That syntax makes the build unhappy:

> 0:00.12 /usr/bin/make -f client.mk -s configure
> 0:00.44 cd /home/aosmond/dev/mozilla/obj-x86_64-pc-linux-gnu
> 0:00.44 /home/aosmond/dev/mozilla/configure
> 0:00.50 Reexecuting in the virtualenv
> 0:00.60 Adding configure options from /home/aosmond/dev/mozilla/.mozconfig
> 0:00.60   --with-system-webp
> 0:00.60   --with-ccache=/usr/bin/ccache
> 0:00.60   --enable-debug
> 0:00.61   CC=/usr/lib/icecc/bin/gcc
> 0:00.61   CXX=/usr/lib/icecc/bin/g++
> 
> ...
> 
> 0:01.99 checking for libwebp >= 0.6.0 libwebpdemux >= 0.6.0... yes
> 0:01.99 checking MOZ_WEBP_CFLAGS... -I/usr/local/include
> 0:02.00 checking MOZ_WEBP_LIBS... -L/usr/local/lib -lwebpdemux -lwebp
> 
> ...
> 
>  0:02.91 creating ./config.data
> 0:03.05 
> 0:03.08 
> 0:03.13 Creating config.status
> 0:03.18 Traceback (most recent call last):
> 0:03.18   File "/home/aosmond/dev/mozilla/configure.py", line 124, in <module>
> 0:03.18     sys.exit(main(sys.argv))
> 0:03.18   File "/home/aosmond/dev/mozilla/configure.py", line 34, in main
> 0:03.18     return config_status(config)
> 0:03.18   File "/home/aosmond/dev/mozilla/configure.py", line 119, in config_status
> 0:03.18     return config_status(args=[], **encode(sanitized_config, encoding))
> 0:03.18   File "/home/aosmond/dev/mozilla/python/mozbuild/mozbuild/config_status.py", line 118, in config_status
> 0:03.18     source=source, mozconfig=mozconfig)
> 0:03.18   File "/home/aosmond/dev/mozilla/python/mozbuild/mozbuild/backend/configenvironment.py", line 151, in __init__
> 0:03.18     serialize(self.substs[name])) for name in self.substs if self.substs[name]]))
> 0:03.18   File "/home/aosmond/dev/mozilla/python/mozbuild/mozbuild/backend/configenvironment.py", line 149, in serialize
> 0:03.18     raise Exception('Unhandled type %s', type(obj))
> 0:03.18 Exception: ('Unhandled type %s', <class 'mozbuild.util.ReadOnlyNamespace'>)
> 0:03.20 *** Fix above errors and then restart with\
> 0:03.20                "/usr/bin/make -f client.mk build"
> 0:03.20 client.mk:379: recipe for target 'configure' failed
> 0:03.20 make: *** [configure] Error 1

Comment 82

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v6 [carries r=jmuizelaar]

Upgrade to libwebp-0.6.0.

Comment 83

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v6 [carries r=glandium]

Update build files and MOZCHANGES to reflect new library version.

Comment 84

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v7

Rebase and update library version requirement.

Comment 85

[Mozilla Marsu]

(In reply to Milan Sreckovic [:milan] from comment #77)
> Mozilla wants to do what's good for the web.  An image format that only some
> browsers support is not, so WebP support in Firefox is as much of a question
> of WebP support in Edge/Safari, though things are never that simple.

To do what's good for the web and to enable web devs to migrate to webp, I hope this is switched on by default, not hidden in some about:config flag. Mozilla is moving away from proprietary standards, even breaking their addon ecosystem and risking obsolescence, so getting Chrome compatibility in the image formats support should be a given. Thanks to Andrew for his persistent work!

Comment 86

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v7.1

Rebase.

Comment 87

[u595893]

(In reply to Andrew Osmond [:aosmond] from comment #86)
> Created attachment 8839972 [details] [diff] [review]
> Part 3. Implement WebP decoder. v7.1
> 
> Rebase.

Has this been landed or builds to test?

on Desktop also most sites are preferring this & on mobile it's worse as Firefox mobile has no webP support but sites still give 
webP in pages and it appears as Firefox is misbehaving.

Would like this to be fixed as it is open-source and will improve us towards building a better web.

Apple has announced that it has plans to support Webp on Safari soon. 

edge/platform/status/webpimageformat states that EDGE has it as planned but that's not the point,
the point is users are facing issues + mozilla stands for open-source & users rights 

So now it's a question of when rather than why of if.

Comment 88

[Mike Taylor [:miketaylr]]

Hi shellye, if you know about sites that are serving webP to Firefox for Android, it would be useful to report those as new bugs (or at least leave them here in comments). Thanks.

Comment 89

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

(In reply to shellye from comment #87)
> (In reply to Andrew Osmond [:aosmond] from comment #86)
> > Created attachment 8839972 [details] [diff] [review]
> > Part 3. Implement WebP decoder. v7.1
> > 
> > Rebase.
> 
> Has this been landed or builds to test?
> 
> on Desktop also most sites are preferring this & on mobile it's worse as
> Firefox mobile has no webP support but sites still give 
> webP in pages and it appears as Firefox is misbehaving.
> 
> Would like this to be fixed as it is open-source and will improve us towards
> building a better web.
> 
> Apple has announced that it has plans to support Webp on Safari soon. 
> 
> edge/platform/status/webpimageformat states that EDGE has it as planned but
> that's not the point,
> the point is users are facing issues + mozilla stands for open-source &
> users rights 
> 
> So now it's a question of when rather than why of if.

Was there a specific request and/or question or action for myself here, :shellye?

Comment 90

[u595893]

(In reply to Desigan Chinniah [:cyberdees] [:dees] [London - GMT] from comment #89)
> (In reply to shellye from comment #87)
> > (In reply to Andrew Osmond [:aosmond] from comment #86)
> > > Created attachment 8839972 [details] [diff] [review]
> > > Part 3. Implement WebP decoder. v7.1
> > > 
> > > Rebase.
> > 
> > Has this been landed or builds to test?
> > 
> > on Desktop also most sites are preferring this & on mobile it's worse as
> > Firefox mobile has no webP support but sites still give 
> > webP in pages and it appears as Firefox is misbehaving.
> > 
> > Would like this to be fixed as it is open-source and will improve us towards
> > building a better web.
> > 
> > Apple has announced that it has plans to support Webp on Safari soon. 
> > 
> > edge/platform/status/webpimageformat states that EDGE has it as planned but
> > that's not the point,
> > the point is users are facing issues + mozilla stands for open-source &
> > users rights 
> > 
> > So now it's a question of when rather than why of if.
> 
> Was there a specific request and/or question or action for myself here,
> :shellye?

actually more of a request of prioritizing this before 57 photon which will showcase new firefox &
as using firefox on android and some sites give webp and which is confusing, even my friends ask me why firefox breaks the page and works in chrome :( so if it could be implemented soon then firefox user base can grow as right now they think firefox is half baked

most of the sites are mentioned here.

most of them use

# http config block
map $http_accept $webp_ext {
    default "";
    "~*webp" ".webp";
}

# server config block
location ~* ^/wp-content/.+\.(png|jpg)$ {
    add_header Vary Accept;
    try_files $uri$webp_ext $uri =404;
}

But still give webp version for firefox

Comment 100

[Astley Chen (inactive)]

On Firefox, youtube.com without log-in renders the logo and icons in a very blur way as it takes PNG as bitmap atlas to be the background.
The problem doesn't exist after log-in since the site turns to use SVG to render them.

Compared to Chrome, webp is used whether you've logged in or not.

PNG: //s.ytimg.com/yts/imgbin/www-hitchhiker-vfl-Nn88d.png

Webp: //s.ytimg.com/yts/imgbin/www-hitchhiker-2x-vfl-xrsMu.webp

Comment 101

[James Zern]

Just for reference there will be a behavior change in chrome due to a bug preventing webp from looping once [1]. The origin of this is that GIF files are looped once more than their stored count [2].
It would be good if the two browsers could behave the same. Given a quick look it seems webp will obey the stored count here, so maybe there isn't anything to be done.

[1] crbug.com/649264
[2] crbug.com/592735

Comment 102

[Timothy Nikkel (:tnikkel)]

(In reply to James Zern from comment #101)
> Just for reference there will be a behavior change in chrome due to a bug
> preventing webp from looping once [1]. The origin of this is that GIF files
> are looped once more than their stored count [2].
> It would be good if the two browsers could behave the same. Given a quick
> look it seems webp will obey the stored count here, so maybe there isn't
> anything to be done.
> 
> [1] crbug.com/649264
> [2] crbug.com/592735

We actually changed our gif behaviour recently to match chrome. Sounds like we should change it back.

Comment 103

[James Zern]

I don't want to hijack the bug discussion, but one point. With my change for webp the two will diverge again. We might need to have a followup conversation about how to handle gif loop count. I don't know the current state of Edge, Safari.

Comment 104

[Anton Kochkov]

The WebP support was requested 7 years ago, according to the first bug: https://bugzilla.mozilla.org/show_bug.cgi?id=600919 Mozilla announced that it would support WebP a year ago. It's clear that even if announced Mozilla still tries to slow down the merge.

Comment 105

[Timothy Nikkel (:tnikkel)]

Where did Mozilla announce they would support webp?

Comment 106

[Mike Hommey [:glandium]]

Googling says this bug being open was taken an an announcement and made the news a year ago.

Comment 107

[Jai Gupta]

One of my projects is dependant on WebP. May we know if Firefox has a priority for WebP or if we should not be expecting WebP within one year from now.

Comment 108

[Djfe]

while someone was working on implementing this (as far as I can see by the patches),
bug reports are never announcements

at least if no deadline is defined/tracking flag has been set


@jai
There is libwebpjs and at least one polyfill on github that provides webp support

you can use that, in-case you need webp now. :)


I have no information on whether someone wants to finallize this feature.
I'm only following the report ^^
(I took, that someone was working on this as a good sign, too, by the way
seems like it doesn't always work out)

Comment 109

[Jai Gupta]

Thank you Djfe. We are already using libwebpjs but what we really require is the ability to extract images from canvas as webp. If png > webp is done via js, it is very slow.

Comment 110

[Djfe]

you can generate png, send them to the server and do the encode to webp on the server-side.

Comment 111

[Ralph Corderoy]

This bug isn't the place to suggest workarounds;  that's just noise for 
all the subscribers.  Google knows of lots of workarounds.

The occasional ping to point out that the Mozilla Foundation isn't 
strapped for cash and should properly fund development of its flagship
product so that long existing, de facto, image standards like WebP are
supported if Firefox is to remain worthy of being called a graphical
browser is fine.  :-)

Comment 112

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mike Hommey [:glandium] from comment #106)
> Googling says this bug being open was taken an an announcement and made the
> news a year ago.

Indeed, there is a reason for "experimental" in the summary, but perhaps that is too subtle of a difference.  It is implemented as experimental.  One browser supporting their company's proprietary image format does not make it a standard.  This may change, of course.

Comment 113

[xanderbertison]

I've managed to shrink the total download size of multiple websites by 50% by using the picture element to detect webp support.

This is supported in both Chrome and Opera now, allowing our users on these advanced platforms to enjoy faster page download speeds. When will Firefox adopt this modern format and allow it's users to experience the benefits this provides?

Please merge soon!

Comment 114

[Anton Kochkov]

(In reply to Milan Sreckovic [:milan] from comment #112)
> (In reply to Mike Hommey [:glandium] from comment #106)
> > Googling says this bug being open was taken an an announcement and made the
> > news a year ago.
> 
> Indeed, there is a reason for "experimental" in the summary, but perhaps
> that is too subtle of a difference.  It is implemented as experimental.  One
> browser supporting their company's proprietary image format does not make it
> a standard.  This may change, of course.

WebP is not a proprietary image format, it's an open and free image format. That's the a huge difference.
Moreover, it solves a problem of not only compressing images, it also solves a problem of still alive GIF format,
which is very ineffective for storing animations, but there are not much alternatives. Using short WebM videos
is just a hackaround because of the poor WebP support among browsers.

Comment 115

[Ralph Corderoy]

> One browser supporting their company's proprietary image format

WebP is supported by more than Chrome, e.g. Opera, and UC Browser.
http://caniuse.com/#feat=webp reckons 73% of users have a browser
that supports WebP.

Comment 116

[Maverick]

May we not forget there is also APNG that is supported by ALL major browsers (except for Edge, naturally)

Comment 117

[Stijn de Witt]

> Indeed, there is a reason for "experimental"

I guess that reason is that we don't care that JPG is now 25 (!!) years old and gives us really bad compression and large file sizes, but is still the only file format suitable for photos that has cross-browser support?

Guys please. The web NEEDS this. It needs it so, so bad!

We are waiting and waiting and waiting year after year but for some reason it seems impossible to get browser makers to agree on a common file format for literally more than a decade.

Mac / iOS now supports HEIF:
http://caniuse.com/#feat=heif

Chrome / Opera support WebP:
http://caniuse.com/#feat=webp

Edge and Firefox are on the fence...

BPG was completely ignored by everyone it seems and we have had more promising formats but no support...

There is this consortium that's very promising but.... tick, tock, time is running out. We are serving terabytes of wasted data every day to power the web. Making image files eat up only half the space they do now would be the single most important development to hit the web platform in years. Yes, more important even than HTML5 was!

You are in a unique position here. If Firefox supported webp it would bring the support (measured in #of users) to a level that would basically force Microsoft and Apple to follow suit... I know you like Daala. So do I. But why not at least let webp be the stop-gap solution in the mean time?

Or hurry up your developments with Daala / AOmedia... but DO SOMETHING. Please. We NEED this.

I have been watching the developments with great interest but honestly waiting for 4 years now and it seems like you guys are just not interested in moving the image encoder situation forward. 

Besides text, images are the most important elements on any web page but for some reason we are stuck in the 90s on them. Why? Why does this not have the #1 priority?? I just can't understand that. Can someone please explain why we don't simply move this into GA and let websites decide whether webp is important / useful enough?

Comment 118

[Jeff Muizelaar [:jrmuizel]]

(In reply to Stijn de Witt from comment #117)
> > Indeed, there is a reason for "experimental"
> 
> I guess that reason is that we don't care that JPG is now 25 (!!) years old
> and gives us really bad compression and large file sizes

This just isn't true. mozjpeg and guetzli have comparable or better compression than webp.

Comment 119

[Pascal Massimino]

That's debatable. But anyhow, WebP degrades better a low or ultra-low quality setting (q < 80 and downward).

Comment 120

[Olivier Borowski]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #118)
> This just isn't true. mozjpeg and guetzli have comparable or better
> compression than webp.

I don't agree, guetzli is only for very high quality and encoding is slow as hell.

(In reply to Pascal Massimino from comment #119)
> That's debatable. But anyhow, WebP degrades better a low or ultra-low
> quality setting (q < 80 and downward).

As Pascal said, webp is perfect for low quality retina images for example.
I've tried mozjpeg but it shows more artifacts at same file size.

Comment 121

[kfuks2]

In my opinion, WebP should be a standard as for now, like WebM...
And if WebP is standard, we should focus on FLIFF as it seems very promissing

Comment 122

[Mike Hoye [:mhoye]]

Hello, everyone. 

For the time being I am restricting participation in this bug to Bugzilla users with editbugs privileges only. 

As always, Bugzilla is not a forum for web-standards debates or policy advocacy. If you feel this is in error, or have something to add to this bug that will materially advance it to a conclusion, please contact me directly.

Thank you.

Comment 123

[Jan Beich]

This bug blocks bug 802882 but existing patches don't implement WebP Encoder (i.e., @mozilla.org/image/encoder;2?type=image/webp) used by html5test.com to detect WebP support e.g.,

  Pale Moon, Waterfox:

    > document.createElement('canvas').toDataURL('image/webp').substring(5, 15) == 'image/webp';
    < false


  Chromium:

    > document.createElement('canvas').toDataURL('image/webp').substring(5, 15) == 'image/webp';
    < true

Comment 124

[Mark Straver]

I think detecting image support by *en*coder presence is not very smart. Is it important that browsers can also encode all image formats they can display? I don't think so.

Comment 125

[Mark Straver]

I've used the proposed implementation in this bug in our platform in development (ref application being Basilisk) and it works well, with one obvious snag: it leaks a lot of memory. If a lot of webp images are decoded, memory isn't returned to the system when the tab with images is closed (e.g. on giphy), with memory ending up in heap-unclassified. Disabling WebP and letting the site feed us other formats like PNG and JPG solves the issue, so there seems to be a problem in the decoder that it doesn't free up the memory when it's done.

Comment 126

[Mark Straver]

Thanks to one of our contributors, "roytam", the memory leak has been found and solved -- this might also interest you for implementation:

"I think WebPFreeDecBuffer(&mBuffer); is missing in nsWebPDecoder::EndFrame() before WebPIDelete(mDecoder); so it is leaking memory?"

Confirmed to stop the leaks.

Comment 127

[Chris Peterson [:cpeterson]]

Microsoft has added support for WebP in the Edge 18 Preview Release.

https://developer.microsoft.com/en-us/microsoft-edge/platform/status/webpimageformat/

Comment 128

[Mark Straver]

Don't you think it's about time this moves forward?

For the record, we have stable WebP support (libwebp 1.0) with conneg in UXP/Pale Moon/Basilisk, and it's been happily used by our million or so users in its implementation resulting from the patches in this bug, with the mem leak fixup from comment 126. You should just land this, and I suggest you enable it by default as well.

Comment 129

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add libwebp to source tree. v7 [carries r=jmuizelaar]

Upgrade to libwebp 1.0.0.

Comment 130

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add build files to support libwebp decoding. v7 [carries r=glandium]

Comment 131

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Implement WebP decoder. v8

Incorporates the leak fix (thanks!). However I think I need to rewrite this to make the animated WebP decoding performance acceptable. Right now it keeps copies the data from the lexer into its own buffer and grows it to fit everything. However most of the time the source data will be in a contiguous buffer, and I would like to avoid that copy. It will only be discontiguous if our size hint for the encoded data was wrong, or if it is really big (since we have a maximum chunk size), which should be relatively rare. We can accept the perf hit in those cases if the common case is fine.

Comment 132

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Implement telemetry for WebP decoder. v5 [carries r=jmuizelaar]

Comment 133

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add --with-system-webp switch to build. v8

Comment 134

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Implement WebP tests. v4

Comment 135

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #131)
> Created attachment 9014481 [details] [diff] [review]
> Part 3. Implement WebP decoder. v8
> 
> Incorporates the leak fix (thanks!). However I think I need to rewrite this
> to make the animated WebP decoding performance acceptable. Right now it
> keeps copies the data from the lexer into its own buffer and grows it to fit
> everything. However most of the time the source data will be in a contiguous
> buffer, and I would like to avoid that copy. It will only be discontiguous
> if our size hint for the encoded data was wrong, or if it is really big
> (since we have a maximum chunk size), which should be relatively rare. We
> can accept the perf hit in those cases if the common case is fine.

I have a patch to fix this ready, but I realized I am also missing QCMS integration.

Comment 136

[Jean-Yves Avenard [:jya]]

We should be using the already existing media stack and/or the existing webm demuxer and vp8/vp9 decoder.

not provide yet another way to decode vp9. This would be our 3rd vp8/vp9 decoders included in the tree otherwise.

Comment 137

[Jean-Yves Avenard [:jya]]

actual, the 4th...

Comment 138

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jean-Yves Avenard [:jya] from comment #136)
> We should be using the already existing media stack and/or the existing webm
> demuxer and vp8/vp9 decoder.
> 
> not provide yet another way to decode vp9. This would be our 3rd vp8/vp9
> decoders included in the tree otherwise.

libwebp includes for support for webp lossless which is completely different from vp8. So if we wanted to use an existing vp8 decoder it would require hooking up that decoder and libwebp. Further, libwebp supports incremental decoding (partially decoding a frame when not all of the data for that frame has arrived yet). I don't think any other vp8 decoders support this functionality.

Comment 139

[Jean-Yves Avenard [:jya]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #138)
> (In reply to Jean-Yves Avenard [:jya] from comment #136)
> > We should be using the already existing media stack and/or the existing webm
> > demuxer and vp8/vp9 decoder.
> > 
> > not provide yet another way to decode vp9. This would be our 3rd vp8/vp9
> > decoders included in the tree otherwise.
> 
> libwebp includes for support for webp lossless which is completely different
> from vp8. So if we wanted to use an existing vp8 decoder it would require
> hooking up that decoder and libwebp. Further, libwebp supports incremental
> decoding (partially decoding a frame when not all of the data for that frame
> has arrived yet). I don't think any other vp8 decoders support this
> functionality.

OK.

This doesn't change the need to implement this through the current media framework and interface it through the image component.

We shouldn't need the demuxer part, and we can add a new decoder as needed (MediaDataDecoder) object.

This would be similar to the preliminary work done for the gif object a couple of years ago. Unfortunately it never went through: bug 1187118

The main advantage of this approach is that once the plumbing is done, going forward will get automatic support for webp2 or the new image format Netflix is pushing forward, that use a mp4 container. Or av1 based image.

This approach may present some challenges, in particular incremental decoding (the MediaDataDecoder API requires a single compressed frame).

I'd be more than happy to present in more details what I have in mind.

Comment 141

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 2. Add build files to support libwebp decoding.

Comment 142

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 3. Implement WebP decoder.

Depends on D8115

Comment 143

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 4. Implement telemetry for WebP decoder.

Depends on D8116

Comment 144

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 5. Add --with-system-webp switch to build.

Depends on D8117

Comment 145

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 6. Implement WebP tests.

Depends on D8118

Comment 146

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1294490 - Part 7. Enable WebP by default.

Bug 1249474 suggested that we add image/webp to the front of the Accept
header for images, to indicate to servers that we actually support WebP.

Depends on D8119

Comment 147

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #142)
> Created attachment 9015618 [details]
> Bug 1294490 - Part 3. Implement WebP decoder.
> 
> Depends on D8115

This added support for QCMS, fixed the performance/extra copying issues I had comments in the code about, and support for skipping pixel premultiplication (if a canvas ends up accessing images for example).

Comment 148

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #145)
> Created attachment 9015621 [details]
> Bug 1294490 - Part 6. Implement WebP tests.
> 
> Depends on D8118

I added a new test as well that tests a tagged WebP image. The default config causes us to apply the transform (which produces the same result as without, it is good to see it call into the QCMS code).

Comment 149

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0243589d63400c1707a46bb35aa6529097984e
try (linux + minor build typo fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=028813bcdcb3a0a127a7cce7bcbdc3022a4eb72f

Comment 150

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Request for data collection.txt, v1

Comment 151

[Chris H-C :chutten]

Comment on attachment 9019706 [details]
Request for data collection.txt, v1

Preliminary notes:

For never-expiring histograms it is good practice to have a test for the collection. This helps stop in CI any code that may break your collection.

For never-expiring histograms it is good practice to have the individual permanently monitoring the data to have their individual email address in the alert_emails/notification_emails field to aid in discovering someone with knowledge about the probe in the event something goes awry.

Neither of these are required for Data Collection Review, but you may wish to consider them.

DATA COLLECTION REVIEW RESPONSE

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. Standard Telemetry mechanisms apply.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. Standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

Andrew Osmond.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 1, Technical. (The request says Category 2, Interaction, but I don't think that the speed or number of webp images loaded is something under most users' control)

    Is the data collection request for default-on or default-off?

The request is for all channels, default on, but the code is for prerelease only. This review is approved for prerelease only.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

N/A permanent collection.

---
Result: datareview+

Comment 152

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74320599eba
Part 1. Add libwebp to source tree. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fc5a421819
Part 2. Add build files to support libwebp decoding. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/d35b5fadd6ca
Part 3. Implement WebP decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a06794651b2
Part 4. Implement telemetry for WebP decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9bc181d099
Part 5. Add --with-system-webp switch to build. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/fecbc475cf54
Part 6. Implement WebP tests. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1d5b5a6a1a
Part 7. Enable WebP by default. r=tnikkel

Comment 153

[Cristian Brindusan [:cbrindusan]]

Backed out 7 changesets (Bug 1294490) for android build bustages.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/649c36f9f0a69ddc8116cbd4b10a565d4a667332

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1a1d5b5a6a1ae680f2ccbc9b63f4ed742a8837ef&selectedJob=208022652

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208022652&repo=mozilla-inbound&lineNumber=33270

Comment 154

[Cristian Brindusan [:cbrindusan]]

There are also failures on mochitest:
https://treeherder.mozilla.org/logviewer.html#?job_id=208026117&repo=mozilla-inbound&lineNumber=1791

Comment 155

[Andrew Osmond [:aosmond] (he/him)]

Let's try this again...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=727fc40d962f0d16d6589037ebd8f47a456594f0

I confirmed WebP works on Fennec, in both the emulator (via mochitest passing) and on a real device (Nexus 5). It seems like I've somehow hurt the timing of that test in CI, so I decided to disable it on android.

Comment 156

[Andrew Osmond [:aosmond] (he/him)]

Also, I'll be landing the enabling of WebP by default separately to give the fuzzing team a chance to setup their own harness. Let's make sure the tests still pass without part 7:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da41c379a053fd9e97622454a8702460866c0a2

Comment 157

[Andrew Osmond [:aosmond] (he/him)]

I split off the mochitests to go with WebP enabled (there is a delay in the pref getting set, and just isn't worth my time to investigate this right now...) and hopefully fixed the ARM64 build issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57e0de42c160a2423410db5de05ae2a68987239

Comment 158

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c519480f7d62
Part 1. Add libwebp to source tree. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb0fe982247
Part 2. Add build files to support libwebp decoding. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d76871100be
Part 3. Implement WebP decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/77efeae42385
Part 4. Implement telemetry for WebP decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/99d6a3b6e120
Part 5. Add --with-system-webp switch to build. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/25aa97e7d106
Part 6. Implement WebP gtests. r=tnikkel

Comment 159

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c519480f7d62
https://hg.mozilla.org/mozilla-central/rev/ceb0fe982247
https://hg.mozilla.org/mozilla-central/rev/8d76871100be
https://hg.mozilla.org/mozilla-central/rev/77efeae42385
https://hg.mozilla.org/mozilla-central/rev/99d6a3b6e120
https://hg.mozilla.org/mozilla-central/rev/25aa97e7d106

Comment 160

[Matthew N. [:MattN]]

(In reply to Andrew Osmond [:aosmond] from comment #157)
> I split off the mochitests to go with WebP enabled (there is a delay in the
> pref getting set, and just isn't worth my time to investigate this right
> now...)

In case you didn't know, you can set prefs from the mochitest.ini file and they will be set at startup time: https://dxr.mozilla.org/mozilla-central/search?q=ext%3Aini%20prefs

Comment 161

[Chris Mills [:cmills]]

Note to MDN writers:

I have added a note to the Fx65 rel notes to cover this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#Other

We still need to document this in other appropriate places on MDN. In fact, I think it'd make sense to add a proper reference guide to images formats supported on the web. This work is outlined on this card: https://trello.com/c/KbbXqkmA/145-add-improve-documentation-on-image-audio-and-video-file-formats

Comment 162

[Chris Mills [:cmills]]

Documented: https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Image_types#WebP