Bug 105915 – [GTK] Add WebP image support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sergio Villar Senin]

I didn't have time to take a look at it, but at first sight looks like the network connections are retrieving the resources properly (just checked the inspector).

[Sergio Villar Senin]

OK, so all of them are webp images, maybe it's just some issue with my setup...

[Martin Robinson]

We don't have support for WebP yet. Perhaps the site is sending them because of our Chromium-esque user agent.

[Sergio Villar Senin]

(In reply to comment #2)
> We don't have support for WebP yet. Perhaps the site is sending them because of our Chromium-esque user agent.

Isn't there a generic WebP decoder in platform/image-decoders? What else do we need?

[Martin Robinson]

There's a platform-independent decoder, but it has a dependency on some external library. Take a look in Source/WebCore/platform/image-decoders/webp for more details. It'd be great to add WebP support. It might be as simple as compiling this library into ours.

[Sergio Villar Senin]

It should be a matter of correctly detecting and linking against the libwebp library

[Sergio Villar Senin]

Created attachment 181040 [details]
Patch

[Martin Robinson]

Comment on attachment 181040 [details]
Patch

Instead of a configuration option, we should always turn this on. I don't think it should be configurable.

[Sergio Villar Senin]

Created attachment 181160 [details]
Patch

[Zan Dobersek]

The bots require libwebp. Should it be included in the Jhbuild moduleset?

[Sergio Villar Senin]

(In reply to comment #9)
> The bots require libwebp. Should it be included in the Jhbuild moduleset?

As we don't need any special version, I'd say that we could simply install the distro version in the bots...

[Martin Robinson]

(In reply to comment #9)
> The bots require libwebp. Should it be included in the Jhbuild moduleset?

Right now we only use the JHBuild for dependencies where you need the correct version to have correct test results. I think it would be nice to have an alternate JHBuild moduleset with many hard-to-get dependencies, but probably not in this patch.

[Martin Robinson]

Comment on attachment 181160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181160&action=review

Thanks! Perhaps you could investigate a small change before landing:

> configure.ac:278
> +# Check for WEBP Image support
> +have_webp=no
> +AC_CHECK_HEADERS([webp/decode.h], [have_webp="yes"], [have_webp="no"])
> +if test "$have_webp" = "yes"; then
> +  WEBP_LIBS='-lwebp'
> +else
> +  AC_MSG_ERROR([WebP library (libwebp) not found])
> +fi
> +AC_SUBST([WEBP_LIBS])

I don't mean to be nitpicky, but I think you can probably simplify this a bit by getting rid of the have_webp variable. That would be more useful if you were actually using it later.

Instead I think you could just keep what you need here inline in the macro. For instance, something like this should work:

AC_CHECK_HEADERS([webp/decode.h], [WEBP_LIBS="-lwebp'], AC_MSG_ERROR([WebP library (libwebp) not found])
AC_SUBST([WEBP_LIBS])

[Sergio Villar Senin]

(In reply to comment #12)
> (From update of attachment 181160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181160&action=review
> 
> Thanks! Perhaps you could investigate a small change before landing:
> 
> > configure.ac:278
> > +# Check for WEBP Image support
> > +have_webp=no
> > +AC_CHECK_HEADERS([webp/decode.h], [have_webp="yes"], [have_webp="no"])
> > +if test "$have_webp" = "yes"; then
> > +  WEBP_LIBS='-lwebp'
> > +else
> > +  AC_MSG_ERROR([WebP library (libwebp) not found])
> > +fi
> > +AC_SUBST([WEBP_LIBS])
> 
> I don't mean to be nitpicky, but I think you can probably simplify this a bit by getting rid of the have_webp variable. That would be more useful if you were actually using it later.
> 
> Instead I think you could just keep what you need here inline in the macro. For instance, something like this should work:
> 
> AC_CHECK_HEADERS([webp/decode.h], [WEBP_LIBS="-lwebp'], AC_MSG_ERROR([WebP library (libwebp) not found])
> AC_SUBST([WEBP_LIBS])

Absolutely true, I should have removed it because I only defined it for using it later in the AM_CONDITIONAL.

[Sergio Villar Senin]

I had updated Igalia bots, kov, philn, could you please install libwebp-dev in your machines?

[Sergio Villar Senin]

The patch to land will unskip also these two tests:

http/tests/images/webp-partial-load.html
http/tests/images/webp-progressive-load.html

We cannot unskip the WebP decoding test (fast/images/webp-image-decoding.html) just because it requires version 0.2 of the library which is still not available for bots distros.

[Sergio Villar Senin]

Committed r139046: <http://trac.webkit.org/changeset/139046>

[Priit Laes (IRC: plaes)]

Is there a reason of not using pkg-config for lookup?

[Sergio Villar Senin]

(In reply to comment #17)
> Is there a reason of not using pkg-config for lookup?

Yes, the library does not provide a .pc

[Priit Laes (IRC: plaes)]

(In reply to comment #18)
> (In reply to comment #17)
> > Is there a reason of not using pkg-config for lookup?
> 
> Yes, the library does not provide a .pc

Seems to be 0.2.x+ feature :(

[Sergio Villar Senin]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Is there a reason of not using pkg-config for lookup?
> > 
> > Yes, the library does not provide a .pc
> 
> Seems to be 0.2.x+ feature :(

Yeah, apart from that, 0.2 will allow us to unskip the webp-decode test which requires some features not present in 0.1.x

[Sergio Villar Senin]

*** Bug 98939 has been marked as a duplicate of this bug. ***