Bug 105915 - [GTK] Add WebP image support
: [GTK] Add WebP image support
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Sergio Villar Senin
http://www.html5rocks.com/en/tutorial...
: Gtk, Soup
Depends on: 47512
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-02 03:51 PST by Sergio Villar Senin
Modified: 2013-01-16 07:38 PST (History)
9 users (show)

See Also:


Attachments
Patch (6.66 KB, patch)
2013-01-02 11:38 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2013-01-03 02:12 PST, Sergio Villar Senin
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2013-01-02 03:51:24 PST
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).
Comment 1 Sergio Villar Senin 2013-01-02 08:36:48 PST
OK, so all of them are webp images, maybe it's just some issue with my setup...
Comment 2 Martin Robinson 2013-01-02 08:38:11 PST
We don't have support for WebP yet. Perhaps the site is sending them because of our Chromium-esque user agent.
Comment 3 Sergio Villar Senin 2013-01-02 08:58:30 PST
(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?
Comment 4 Martin Robinson 2013-01-02 09:05:48 PST
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.
Comment 5 Sergio Villar Senin 2013-01-02 09:25:40 PST
It should be a matter of correctly detecting and linking against the libwebp library
Comment 6 Sergio Villar Senin 2013-01-02 11:38:00 PST
Created attachment 181040 [details]
Patch
Comment 7 Martin Robinson 2013-01-02 12:28:45 PST
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.
Comment 8 Sergio Villar Senin 2013-01-03 02:12:58 PST
Created attachment 181160 [details]
Patch
Comment 9 Zan Dobersek 2013-01-03 06:22:50 PST
The bots require libwebp. Should it be included in the Jhbuild moduleset?
Comment 10 Sergio Villar Senin 2013-01-03 07:13:48 PST
(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...
Comment 11 Martin Robinson 2013-01-03 09:15:14 PST
(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.
Comment 12 Martin Robinson 2013-01-03 09:19:39 PST
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])
Comment 13 Sergio Villar Senin 2013-01-03 09:26:27 PST
(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.
Comment 14 Sergio Villar Senin 2013-01-03 10:41:58 PST
I had updated Igalia bots, kov, philn, could you please install libwebp-dev in your machines?
Comment 15 Sergio Villar Senin 2013-01-04 03:59:25 PST
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.
Comment 16 Sergio Villar Senin 2013-01-08 01:33:30 PST
Committed r139046: <http://trac.webkit.org/changeset/139046>
Comment 17 Priit Laes (IRC: plaes) 2013-01-09 03:11:39 PST
Is there a reason of not using pkg-config for lookup?
Comment 18 Sergio Villar Senin 2013-01-09 04:05:59 PST
(In reply to comment #17)
> Is there a reason of not using pkg-config for lookup?

Yes, the library does not provide a .pc
Comment 19 Priit Laes (IRC: plaes) 2013-01-09 04:22:33 PST
(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 :(
Comment 20 Sergio Villar Senin 2013-01-09 04:43:38 PST
(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
Comment 21 Sergio Villar Senin 2013-01-16 07:38:48 PST
*** Bug 98939 has been marked as a duplicate of this bug. ***