Bug 1294490 (WebP)

Implement WebP image support

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
3 years ago
22 days ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

(Depends on: 1 bug, Blocks: 5 bugs, 4 keywords)

65 Branch
mozilla65
dev-doc-needed, feature, parity-chrome, parity-edge
Points:
---
Dependency tree / graph
Bug Flags:
webcompat +

Firefox Tracking Flags

(platform-rel +, firefox-esr60 wontfix, firefox57 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

(Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p1][gfx-noted])

Attachments

(8 attachments, 42 obsolete attachments)

1.54 MB, patch
aosmond
: review+
Details | Diff | Splinter Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
2.96 KB, text/plain
chutten
: review+
Details
(Assignee)

Description

3 years ago
Implement WebP decoding, pref'd off by default.
(Assignee)

Updated

3 years ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

3 years ago
Refresh.
Attachment #8780229 - Attachment is obsolete: true
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)
Keywords: dev-doc-needed

Comment 15

3 years 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

3 years 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

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

Comment 19

3 years ago
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

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

Comment 21

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

Comment 22

3 years 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 24

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

Comment 25

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

Comment 26

3 years 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

3 years ago
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

3 years 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

3 years ago
Attachment #8792066 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8792067 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8792079 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8792509 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years 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.
(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/
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

3 years 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

3 years 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.
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

3 years 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

3 years 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

3 years 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

3 years 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

3 years 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

3 years ago
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

3 years ago
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

3 years ago
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

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

Comment 58

3 years ago
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

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

Comment 60

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

Comment 61

3 years 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

3 years ago
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

3 years ago
Attachment #8795737 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Attachment #8795741 - Flags: review?(mh+mozilla)
(Assignee)

Comment 63

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

Updated

3 years ago
Attachment #8795744 - Flags: review?(jmuizelaar)
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)
Depends on: 1307355
(Assignee)

Comment 65

3 years ago
(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

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

Comment 68

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

Comment 69

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

Comment 70

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

Comment 71

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

Comment 72

2 years ago
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

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

Comment 74

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

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

Comment 83

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

Updated

2 years ago
Attachment #8831265 - Flags: review+
(Assignee)

Comment 84

2 years ago
Rebase and update library version requirement.
Attachment #8823667 - Attachment is obsolete: true
Comment hidden (advocacy)
(Assignee)

Comment 86

2 years ago
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]
platform-rel: ? → +
Flags: webcompat?

Comment 87

2 years 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

2 years 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
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
status-firefox57: --- → affected
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat]
status-firefox57: affected → wontfix
Priority: -- → P3
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat][gfx-noted]
Flags: needinfo?(telin)

Comment 101

2 years ago
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
(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

2 years ago
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

2 years ago
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.
Where did Mozilla announce they would support webp?
Googling says this bug being open was taken an an announcement and made the news a year ago.
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (advocacy)
(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 hidden (advocacy)
Comment hidden (offtopic)
Comment hidden (advocacy)
Comment hidden (offtopic)
Comment hidden (advocacy)
(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.
That's debatable. But anyhow, WebP degrades better a low or ultra-low quality setting (q < 80 and downward).
(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 hidden (advocacy)
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.
Restrict Comments: true

Comment 123

a year ago
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

a year ago
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.
Flags: webcompat? → webcompat+
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat][gfx-noted] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p2][gfx-noted]

Comment 125

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

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

Updated

9 months ago
See Also: → bug 1472145
Microsoft has added support for WebP in the Edge 18 Preview Release.

https://developer.microsoft.com/en-us/microsoft-edge/platform/status/webpimageformat/
Keywords: parity-chrome, parity-edge
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p2][gfx-noted] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p1][gfx-noted]

Comment 128

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

Comment 129

6 months ago
Upgrade to libwebp 1.0.0.
Attachment #8831265 - Attachment is obsolete: true
Attachment #9014472 - Flags: review+
(Assignee)

Comment 131

6 months ago
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.
Attachment #8839972 - Attachment is obsolete: true
Attachment #8839972 - Flags: review?(jmuizelaar)
Attachment #9014481 - Flags: review+
(Assignee)

Updated

6 months ago
Attachment #9014481 - Flags: review+
(Assignee)

Comment 132

6 months ago
Attachment #8795740 - Attachment is obsolete: true
Attachment #9014484 - Flags: review+
(Assignee)

Comment 133

6 months ago
Attachment #8831267 - Attachment is obsolete: true
(Assignee)

Comment 134

6 months ago
Attachment #8795743 - Attachment is obsolete: true
Attachment #8823636 - Attachment is obsolete: true
Attachment #8823636 - Flags: review?(jmuizelaar)
(Assignee)

Comment 135

6 months ago
(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.
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.
actual, the 4th...
QA Contact: aosmond
(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.
(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.
Duplicate of this bug: 1496856
(Assignee)

Comment 142

5 months ago
Depends on D8115
(Assignee)

Comment 145

5 months ago
Depends on D8118
(Assignee)

Comment 146

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

Updated

5 months ago
Attachment #9014473 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9014481 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9014485 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9014486 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9014484 - Attachment is obsolete: true
(Assignee)

Comment 147

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

Comment 148

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

Updated

5 months ago
Blocks: 1497772
(Assignee)

Updated

5 months ago
status-firefox62: --- → wontfix
status-firefox63: --- → wontfix
status-firefox64: --- → wontfix
status-firefox-esr60: --- → wontfix
Summary: Implement experimental WebP image support → Implement WebP image support
(Assignee)

Comment 150

5 months ago
Attachment #9019706 - Flags: review?(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+
Attachment #9019706 - Flags: review?(chutten) → review+

Comment 152

5 months ago
Pushed by aosmond@gmail.com:
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
Blocks: 1502423
(Assignee)

Comment 155

5 months ago
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.
Flags: needinfo?(aosmond)
(Assignee)

Comment 156

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

Comment 157

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

Updated

5 months ago
OS: Unspecified → All
Hardware: Unspecified → All
Target Milestone: --- → mozilla65
Version: unspecified → 65 Branch
(Assignee)

Updated

5 months ago
Blocks: 1503653

Comment 158

5 months ago
Pushed by aosmond@gmail.com:
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
(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

Updated

5 months ago
Depends on: 1503935
Depends on: 1504016

Updated

5 months ago
Depends on: 1504120
(Assignee)

Updated

5 months ago
Depends on: 1504237
(Assignee)

Updated

5 months ago
Blocks: 1504434

Updated

4 months ago
Blocks: 1511670
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

Updated

a month ago
Depends on: 1526731

Updated

22 days ago
Depends on: 1530887

Updated

22 days ago
No longer depends on: 1530887
You need to log in before you can comment on or make changes to this bug.