Closed
Bug 1294490
(WebP)
Opened 8 years ago
Closed 6 years ago
Implement WebP image support
Categories
(Core :: Graphics: ImageLib, enhancement, P3)
Tracking
()
VERIFIED
FIXED
mozilla65
People
(Reporter: aosmond, Assigned: aosmond)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p1][gfx-noted])
Attachments
(8 files, 42 obsolete files)
(deleted),
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/plain
|
chutten
:
review+
|
Details |
Implement WebP decoding, pref'd off by default.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd6a53932b35
Assignee | ||
Comment 6•8 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•8 years ago
|
||
Add script to facilitate updating libwebp.
Attachment #8780227 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Should fix the Android build. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c5442e251c
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 15•8 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•8 years ago
|
||
This would conflict with bug 572650. Web Devs should use features like <source>/srcset.
Comment 17•8 years ago
|
||
(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•8 years ago
|
||
Remove src/dsp/cpu.c as we implement the necessary symbols ourselves.
Attachment #8782488 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 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•8 years ago
|
||
Fix handling of non-0xFF alpha if using SurfaceFormat::B8G8R8X8.
Attachment #8782490 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 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•8 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 23•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97f5e4cc98a4
Assignee | ||
Comment 24•8 years ago
|
||
Refresh against tree.
Attachment #8782491 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
Add library version checks.
Attachment #8792068 -
Attachment is obsolete: true
Comment 26•8 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•8 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•8 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•8 years ago
|
Attachment #8792066 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8792067 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8792079 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8792509 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8792080 -
Flags: review?(mh+mozilla)
Comment 29•8 years ago
|
||
(In reply to Mark Straver from comment #26) > Of course! I'm just making a point that servers who serve WebP in general > will NOT do this unless you have the support explicitly listed in the > http-accept header (to prevent web compat issues). Can you name some popular WebP-serving servers which make the decision based on the Accept: header? If there are high-traffic sites out there which do this, it may be worth it. But this form of content negotiation is basically dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good evidence to add it to the Accept: header. Gerv
Comment 30•8 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #29) \ > this, it may be worth it. But this form of content negotiation is basically > dead (https://wiki.whatwg.org/wiki/Why_not_conneg) so we'd need good > evidence to add it to the Accept: header. factually that is not true that conneg is dead - the reference is an opinion piece and I've disagreed strongly in the past and still do. content negotiation is used for webp (https://www.igvita.com/2013/05/01/deploying-webp-via-accept-content-negotiation/), and (relatedly) brotli and makes a lot of sense for incremental deployment of new encodings. with webp in particular, this is the expected way to do it (and is chrome compat - which should always be a goal right now) and I would rather strongly want us to add it.
Comment 31•8 years ago
|
||
I agree conneg has a shot at working if everyone so far who's implemented the standard has simultaneously implemented the header, and if sites are therefore using it for detection. If that's true, I have no objection to adding it. Are we planning to register ourselves as the default WebP viewer if nothing else is registered? This is to support the "Save As..." use case. Gerv
Comment 32•8 years ago
|
||
(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•8 years 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•8 years 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
Comment 35•8 years ago
|
||
rstrong fixed bug 452254, so I suspect he knows how to register for filetypes in Windows :-) Gerv
Flags: needinfo?(robert.strong.bugs)
Comment 36•8 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.
![]() |
||
Comment 37•8 years ago
|
||
(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•8 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.
![]() |
||
Comment 39•8 years 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 40•8 years ago
|
||
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 41•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8792079 -
Flags: review?(mh+mozilla) → review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8792066 -
Flags: review?(jmuizelaar) → review?(mh+mozilla)
Comment 42•8 years ago
|
||
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•8 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 44•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8792064 -
Flags: review?(jmuizelaar) → review+
Comment 45•8 years ago
|
||
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•8 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•8 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•8 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.
Comment 49•8 years ago
|
||
(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•8 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 51•8 years ago
|
||
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 52•8 years ago
|
||
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•8 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•8 years ago
|
||
Incorporate feedback from comment 45 and comment 51. Adds building animated image support as well.
Attachment #8792066 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8792079 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 56•8 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•8 years ago
|
||
Refresh against other patches.
Attachment #8792079 -
Attachment is obsolete: true
Assignee | ||
Comment 58•8 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•8 years ago
|
||
Incorporated feedback from comment 41.
Attachment #8792509 -
Attachment is obsolete: true
Attachment #8795743 -
Flags: review+
Assignee | ||
Comment 60•8 years ago
|
||
Broken out of Part 3. Added gtests and mochitests for animated images.
Assignee | ||
Comment 61•8 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•8 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•8 years ago
|
Attachment #8795737 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8795741 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 63•8 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•8 years ago
|
Attachment #8795744 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8795737 -
Flags: review?(mh+mozilla) → review+
Comment 64•8 years ago
|
||
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)
Assignee | ||
Comment 65•8 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 66•8 years ago
|
||
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 years ago
|
||
Incorporate latest feedback and fix to work with latest build changes.
Attachment #8800624 -
Attachment is obsolete: true
Assignee | ||
Comment 68•8 years ago
|
||
Rebase against tree.
Attachment #8795805 -
Attachment is obsolete: true
Attachment #8795805 -
Flags: review?(jmuizelaar)
Attachment #8818550 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 69•8 years ago
|
||
Rebase against tree.
Attachment #8795744 -
Attachment is obsolete: true
Attachment #8795744 -
Flags: review?(jmuizelaar)
Attachment #8818551 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 70•8 years ago
|
||
Upgrade to libwebp-0.5.2.
Attachment #8795733 -
Attachment is obsolete: true
Attachment #8823634 -
Flags: review+
Assignee | ||
Comment 71•8 years ago
|
||
Update MOZCHANGES to reflect lib upgrade.
Attachment #8795737 -
Attachment is obsolete: true
Attachment #8823635 -
Flags: review+
Assignee | ||
Comment 72•8 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•8 years ago
|
||
Update version check.
Attachment #8808680 -
Attachment is obsolete: true
Attachment #8823667 -
Flags: review?(mh+mozilla)
Comment 74•8 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•8 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•8 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•8 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 80•8 years ago
|
||
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•8 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•8 years ago
|
||
Upgrade to libwebp-0.6.0.
Attachment #8823634 -
Attachment is obsolete: true
Assignee | ||
Comment 83•8 years ago
|
||
Update build files and MOZCHANGES to reflect new library version.
Attachment #8823635 -
Attachment is obsolete: true
Attachment #8831266 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8831265 -
Flags: review+
Assignee | ||
Comment 84•8 years ago
|
||
Rebase and update library version requirement.
Attachment #8823667 -
Attachment is obsolete: true
Comment hidden (advocacy) |
Assignee | ||
Comment 86•8 years ago
|
||
Rebase.
Attachment #8818550 -
Attachment is obsolete: true
Attachment #8818550 -
Flags: review?(jmuizelaar)
Attachment #8839972 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
platform-rel: --- → ?
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic]
Updated•8 years ago
|
platform-rel: ? → +
![]() |
||
Updated•8 years ago
|
Flags: webcompat?
See Also: → https://webcompat.com/issues/3550
![]() |
||
Updated•8 years ago
|
![]() |
||
Updated•8 years ago
|
Comment 87•7 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)
Comment 88•7 years ago
|
||
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•7 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)
Updated•7 years ago
|
Flags: needinfo?(aosmond)
Updated•7 years ago
|
Comment 100•7 years ago
|
||
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
Updated•7 years ago
|
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat]
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat][gfx-noted]
Updated•7 years ago
|
Flags: needinfo?(telin)
Comment 101•7 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
Comment 102•7 years ago
|
||
(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•7 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.
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/9637
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/10463
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/10491
Comment 104•7 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.
Comment 106•7 years ago
|
||
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) |
Comment 118•7 years ago
|
||
(In reply to Stijn de Witt from comment #117) > > Indeed, there is a reason for "experimental" > > I guess that reason is that we don't care that JPG is now 25 (!!) years old > and gives us really bad compression and large file sizes This just isn't true. mozjpeg and guetzli have comparable or better compression than webp.
Comment 119•7 years ago
|
||
That's debatable. But anyhow, WebP degrades better a low or ultra-low quality setting (q < 80 and downward).
Comment 120•7 years ago
|
||
(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) |
Comment 122•7 years ago
|
||
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
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/13075
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/13886
Updated•7 years ago
|
Updated•7 years ago
|
Comment 123•7 years 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•7 years 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.
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/16221
Updated•6 years ago
|
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•6 years 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•6 years 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•6 years ago
|
![]() |
||
Updated•6 years ago
|
See Also: → https://webcompat.com/issues/18237
Updated•6 years ago
|
Updated•6 years ago
|
Comment 127•6 years ago
|
||
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
Updated•6 years ago
|
Whiteboard: [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p2][gfx-noted] → [platform-rel-Amazon][platform-rel-AmazonMusic][webcompat:p1][gfx-noted]
Comment 128•6 years 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 years ago
|
||
Upgrade to libwebp 1.0.0.
Attachment #8831265 -
Attachment is obsolete: true
Attachment #9014472 -
Flags: review+
Assignee | ||
Comment 130•6 years ago
|
||
Attachment #8831266 -
Attachment is obsolete: true
Attachment #9014473 -
Flags: review+
Assignee | ||
Comment 131•6 years 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 years ago
|
Attachment #9014481 -
Flags: review+
Assignee | ||
Comment 132•6 years ago
|
||
Attachment #8795740 -
Attachment is obsolete: true
Attachment #9014484 -
Flags: review+
Assignee | ||
Comment 134•6 years ago
|
||
Attachment #8795743 -
Attachment is obsolete: true
Attachment #8823636 -
Attachment is obsolete: true
Attachment #8823636 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 135•6 years 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.
Comment 136•6 years ago
|
||
We should be using the already existing media stack and/or the existing webm demuxer and vp8/vp9 decoder. not provide yet another way to decode vp9. This would be our 3rd vp8/vp9 decoders included in the tree otherwise.
Comment 138•6 years ago
|
||
(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.
Updated•6 years ago
|
QA Contact: aosmond
Comment 139•6 years ago
|
||
(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.
Assignee | ||
Comment 141•6 years ago
|
||
Assignee | ||
Comment 146•6 years 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•6 years ago
|
Attachment #9014473 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9014481 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9014485 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9014486 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9014484 -
Attachment is obsolete: true
Assignee | ||
Comment 147•6 years 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•6 years 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).
![]() |
||
Updated•6 years ago
|
See Also: → https://webcompat.com/issues/19585
Assignee | ||
Updated•6 years 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 149•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0243589d63400c1707a46bb35aa6529097984e try (linux + minor build typo fix): https://treeherder.mozilla.org/#/jobs?repo=try&revision=028813bcdcb3a0a127a7cce7bcbdc3022a4eb72f
Comment 151•6 years ago
|
||
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•6 years 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
Comment 153•6 years ago
|
||
Backed out 7 changesets (Bug 1294490) for android build bustages. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/649c36f9f0a69ddc8116cbd4b10a565d4a667332 Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1a1d5b5a6a1ae680f2ccbc9b63f4ed742a8837ef&selectedJob=208022652 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208022652&repo=mozilla-inbound&lineNumber=33270
Flags: needinfo?(aosmond)
Comment 154•6 years ago
|
||
There are also failures on mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=208026117&repo=mozilla-inbound&lineNumber=1791
Assignee | ||
Comment 155•6 years 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•6 years 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•6 years 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•6 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Target Milestone: --- → mozilla65
Version: unspecified → 65 Branch
Comment 158•6 years 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
Comment 159•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c519480f7d62 https://hg.mozilla.org/mozilla-central/rev/ceb0fe982247 https://hg.mozilla.org/mozilla-central/rev/8d76871100be https://hg.mozilla.org/mozilla-central/rev/77efeae42385 https://hg.mozilla.org/mozilla-central/rev/99d6a3b6e120 https://hg.mozilla.org/mozilla-central/rev/25aa97e7d106
Comment 160•6 years ago
|
||
(In reply to Andrew Osmond [:aosmond] from comment #157) > I split off the mochitests to go with WebP enabled (there is a delay in the > pref getting set, and just isn't worth my time to investigate this right > now...) In case you didn't know, you can set prefs from the mochitest.ini file and they will be set at startup time: https://dxr.mozilla.org/mozilla-central/search?q=ext%3Aini%20prefs
Comment 161•6 years ago
|
||
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•6 years ago
|
See Also: → https://webcompat.com/issues/14124
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Type: defect → enhancement
Updated•4 years ago
|
Updated•3 years ago
|
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•