Bug 1294490 (WebP)

Implement experimental WebP image support

ASSIGNED
Assigned to

Status

()

Core
ImageLib
ASSIGNED
11 months ago
3 days ago

People

(Reporter: aosmond, Assigned: aosmond, NeedInfo)

Tracking

(Blocks: 1 bug, {dev-doc-needed, feature})

unspecified
dev-doc-needed, feature
Points:
---
Dependency tree / graph
Bug Flags:
webcompat ?

Firefox Tracking Flags

(platform-rel +)

Details

(Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic])

Attachments

(7 attachments, 30 obsolete attachments)

3.38 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
3.98 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
33.41 KB, patch
aosmond
: review?
jrmuizel
Details | Diff | Splinter Review
1.14 MB, patch
aosmond
: review+
Details | Diff | Splinter Review
10.77 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
28.27 KB, patch
aosmond
: review?
jrmuizel
Details | Diff | Splinter Review
(Assignee)

Description

11 months ago
Implement WebP decoding, pref'd off by default.
(Assignee)

Updated

11 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

11 months ago
Created attachment 8780226 [details] [diff] [review]
Part 1. Add libwebp to source tree.
(Assignee)

Comment 2

11 months ago
Created attachment 8780227 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding.
(Assignee)

Comment 3

11 months ago
Created attachment 8780228 [details] [diff] [review]
Part 3. Implement WebP decoder and tests.
(Assignee)

Comment 4

11 months ago
Created attachment 8780229 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder.
(Assignee)

Comment 5

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd6a53932b35
(Assignee)

Comment 6

10 months ago
Created attachment 8782488 [details] [diff] [review]
Part 1. Add libwebp to source tree. v2

Shrink the copied libwebp tree significantly so that we only take what we need.
Attachment #8780226 - Attachment is obsolete: true
(Assignee)

Comment 7

10 months ago
Created attachment 8782489 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding. v2

Add script to facilitate updating libwebp.
Attachment #8780227 - Attachment is obsolete: true
(Assignee)

Comment 8

10 months ago
Created attachment 8782490 [details] [diff] [review]
Part 3. Implement WebP decoder and tests. v2

Refresh.
Attachment #8780228 - Attachment is obsolete: true
(Assignee)

Comment 9

10 months ago
Created attachment 8782491 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder. v2

Refresh.
Attachment #8780229 - Attachment is obsolete: true
(Assignee)

Comment 10

10 months ago
Created attachment 8782492 [details] [diff] [review]
Part 5. Fix Android builds.

Should fix the Android build.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c5442e251c
Duplicate of this bug: 856375
So this bug inherits the "WebP" alias from bug 856375 :)
Alias: WebP
Comment hidden (off-topic)
Comment hidden (off-topic)
Depends on: 1297902
Keywords: dev-doc-needed

Comment 15

10 months ago
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

10 months ago
This would conflict with bug 572650.
Web Devs should use features like <source>/srcset.
(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?
(Assignee)

Comment 18

10 months ago
Created attachment 8792064 [details] [diff] [review]
Part 1. Add libwebp to source tree. v3

Remove src/dsp/cpu.c as we implement the necessary symbols ourselves.
Attachment #8782488 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
Created attachment 8792066 [details] [diff] [review]
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.
Attachment #8782489 - Attachment is obsolete: true
(Assignee)

Comment 20

10 months ago
Created attachment 8792067 [details] [diff] [review]
Part 3. Implement WebP decoder and tests. v3

Fix handling of non-0xFF alpha if using SurfaceFormat::B8G8R8X8.
Attachment #8782490 - Attachment is obsolete: true
(Assignee)

Comment 21

10 months ago
Created attachment 8792068 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v1

Add support for build switch --with-system-webp to mirror other image decoder libraries.
Attachment #8782492 - Attachment is obsolete: true
(Assignee)

Comment 22

10 months ago
(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 :).
(Assignee)

Comment 23

10 months ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97f5e4cc98a4
(Assignee)

Comment 24

10 months ago
Created attachment 8792079 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder. v3

Refresh against tree.
Attachment #8782491 - Attachment is obsolete: true
(Assignee)

Comment 25

10 months ago
Created attachment 8792080 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v2

Add library version checks.
Attachment #8792068 - Attachment is obsolete: true

Comment 26

10 months ago
(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.
(Assignee)

Comment 27

9 months ago
Created attachment 8792509 [details] [diff] [review]
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.
(Assignee)

Comment 28

9 months ago
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 :).
Attachment #8792064 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8792066 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8792067 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8792079 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8792509 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8792080 - Flags: review?(mh+mozilla)
(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
(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.
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
(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

9 months ago
(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

9 months ago
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
rstrong fixed bug 452254, so I suspect he knows how to register for filetypes in Windows :-)

Gerv
Flags: needinfo?(robert.strong.bugs)

Comment 36

9 months ago
(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.
(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.
Flags: needinfo?(robert.strong.bugs)

Comment 38

9 months ago
(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

9 months ago
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 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
Attachment #8792079 - Flags: review?(jmuizelaar) → review?(mh+mozilla)
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
Attachment #8792509 - Flags: review?(jmuizelaar) → review+
Attachment #8792079 - Flags: review?(mh+mozilla) → review?(jmuizelaar)
Attachment #8792066 - Flags: review?(jmuizelaar) → review?(mh+mozilla)
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?
(Assignee)

Comment 43

9 months ago
(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 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.
Attachment #8792067 - Flags: review?(jmuizelaar) → review-
Attachment #8792064 - Flags: review?(jmuizelaar) → review+
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

9 months ago
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?
(Assignee)

Comment 47

9 months ago
(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.
(Assignee)

Comment 48

9 months ago
(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.
(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

9 months ago
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 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']
Attachment #8792066 - Flags: review?(mh+mozilla)
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.
Attachment #8792080 - Flags: review?(mh+mozilla)
Comment hidden (offtopic)
(Assignee)

Comment 54

9 months ago
Created attachment 8795733 [details] [diff] [review]
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.
Attachment #8792064 - Attachment is obsolete: true
Attachment #8795733 - Flags: review+
(Assignee)

Comment 55

9 months ago
Created attachment 8795737 [details] [diff] [review]
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.
Attachment #8792066 - Attachment is obsolete: true
Attachment #8792079 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 56

9 months ago
Created attachment 8795739 [details] [diff] [review]
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.
Attachment #8792067 - Attachment is obsolete: true
(Assignee)

Comment 57

9 months ago
Created attachment 8795740 [details] [diff] [review]
Part 4. Implement telemetry for WebP decoder. v4 [carries r=jmuizelaar]

Refresh against other patches.
Attachment #8792079 - Attachment is obsolete: true
(Assignee)

Comment 58

9 months ago
Created attachment 8795741 [details] [diff] [review]
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.
Attachment #8792080 - Attachment is obsolete: true
(Assignee)

Comment 59

9 months ago
Created attachment 8795743 [details] [diff] [review]
Part 6. Detect WebP MIME type from content sniffing. v2 [carries r=jmuizelaar]

Incorporated feedback from comment 41.
Attachment #8792509 - Attachment is obsolete: true
Attachment #8795743 - Flags: review+
(Assignee)

Comment 60

9 months ago
Created attachment 8795744 [details] [diff] [review]
Part 7. Implement WebP tests. v1

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

Comment 61

9 months ago
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 :).
Attachment #8795740 - Attachment description: Part 4. Implement telemetry for WebP decoder. v4 → Part 4. Implement telemetry for WebP decoder. v4 [carries r=jmuizelaar]
Attachment #8795740 - Flags: review+
(Assignee)

Comment 62

9 months ago
Created attachment 8795787 [details] [diff] [review]
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.
Attachment #8795739 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8795737 - Flags: review?(mh+mozilla)
(Assignee)

Updated

9 months ago
Attachment #8795741 - Flags: review?(mh+mozilla)
(Assignee)

Comment 63

9 months ago
Created attachment 8795805 [details] [diff] [review]
Part 3. Implement WebP decoder. v6

Some minor refactoring to clean up return values of ReadMultiple/ReadSingle.
Attachment #8795787 - Attachment is obsolete: true
Attachment #8795805 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 months ago
Attachment #8795744 - Flags: review?(jmuizelaar)

Updated

9 months ago
Attachment #8795737 - Flags: review?(mh+mozilla) → review+
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)
Attachment #8795741 - Flags: review?(mh+mozilla)

Updated

9 months ago
Depends on: 1307355
(Assignee)

Comment 65

9 months ago
Created attachment 8800624 [details] [diff] [review]
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.
Attachment #8795741 - Attachment is obsolete: true
Attachment #8800624 - Flags: review?(mh+mozilla)
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.
Attachment #8800624 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 67

8 months ago
Created attachment 8808680 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v5

Incorporate latest feedback and fix to work with latest build changes.
Attachment #8800624 - Attachment is obsolete: true
Keywords: feature
(Assignee)

Comment 68

7 months ago
Created attachment 8818550 [details] [diff] [review]
Part 3. Implement WebP decoder. v7

Rebase against tree.
Attachment #8795805 - Attachment is obsolete: true
Attachment #8795805 - Flags: review?(jmuizelaar)
Attachment #8818550 - Flags: review?(jmuizelaar)
(Assignee)

Comment 69

7 months ago
Created attachment 8818551 [details] [diff] [review]
Part 7. Implement WebP tests. v2

Rebase against tree.
Attachment #8795744 - Attachment is obsolete: true
Attachment #8795744 - Flags: review?(jmuizelaar)
Attachment #8818551 - Flags: review?(jmuizelaar)
(Assignee)

Comment 70

6 months ago
Created attachment 8823634 [details] [diff] [review]
Part 1. Add libwebp to source tree. v5 [carries r=jmuizelaar]

Upgrade to libwebp-0.5.2.
Attachment #8795733 - Attachment is obsolete: true
Attachment #8823634 - Flags: review+
(Assignee)

Comment 71

6 months ago
Created attachment 8823635 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding. v5 [carries r=glandium]

Update MOZCHANGES to reflect lib upgrade.
Attachment #8795737 - Attachment is obsolete: true
Attachment #8823635 - Flags: review+
(Assignee)

Comment 72

6 months ago
Created attachment 8823636 [details] [diff] [review]
Part 7. Implement WebP tests. v3

Refresh against tree.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10b72bdbd8b8bc148d8f1da25930fe387cbe8a75
Attachment #8818551 - Attachment is obsolete: true
Attachment #8818551 - Flags: review?(jmuizelaar)
Attachment #8823636 - Flags: review?(jmuizelaar)
(Assignee)

Comment 73

6 months ago
Created attachment 8823667 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v6

Update version check.
Attachment #8808680 - Attachment is obsolete: true
Attachment #8823667 - Flags: review?(mh+mozilla)

Comment 74

6 months ago
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

6 months ago
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

6 months ago
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.
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 hidden (off-topic)

Comment 79

6 months ago
> 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 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.
Attachment #8823667 - Flags: review?(mh+mozilla)
(Assignee)

Comment 81

5 months ago
(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
(Assignee)

Comment 82

5 months ago
Created attachment 8831265 [details] [diff] [review]
Part 1. Add libwebp to source tree. v6 [carries r=jmuizelaar]

Upgrade to libwebp-0.6.0.
Attachment #8823634 - Attachment is obsolete: true
(Assignee)

Comment 83

5 months ago
Created attachment 8831266 [details] [diff] [review]
Part 2. Add build files to support libwebp decoding. v6 [carries r=glandium]

Update build files and MOZCHANGES to reflect new library version.
Attachment #8823635 - Attachment is obsolete: true
Attachment #8831266 - Flags: review+
(Assignee)

Updated

5 months ago
Attachment #8831265 - Flags: review+
(Assignee)

Comment 84

5 months ago
Created attachment 8831267 [details] [diff] [review]
Part 5. Add --with-system-webp switch to build. v7

Rebase and update library version requirement.
Attachment #8823667 - Attachment is obsolete: true

Comment 85

5 months ago
(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!
(Assignee)

Comment 86

4 months ago
Created attachment 8839972 [details] [diff] [review]
Part 3. Implement WebP decoder. v7.1

Rebase.
Attachment #8818550 - Attachment is obsolete: true
Attachment #8818550 - Flags: review?(jmuizelaar)
Attachment #8839972 - Flags: review?(jmuizelaar)
platform-rel: --- → ?
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic]

Updated

4 months ago
platform-rel: ? → +

Updated

2 months ago
Flags: webcompat?

Updated

2 months ago

Updated

2 months ago

Comment 87

a month ago
(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.
Flags: needinfo?(telin)
Flags: needinfo?(dchinniah)
Flags: needinfo?(aosmond)
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.
(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?
Flags: needinfo?(dchinniah) → needinfo?(shellye5)

Comment 90

22 days ago
(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
Flags: needinfo?(shellye5)
Flags: needinfo?(aosmond)
Blocks: 802882
You need to log in before you can comment on or make changes to this bug.