Bug 104909

Summary: [Qt] The Qt's configuration isn't honoured regarding the use of the system libpng and libjpeg
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, buildbot, hausmann, jturcotte, ossy, rniwa, vestbo, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 103747    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jocelyn Turcotte 2012-12-13 06:29:15 PST
[Qt] The Qt's configuration isn't honoured regarding the use of the system libpng and libjpeg
Comment 1 Jocelyn Turcotte 2012-12-13 06:36:13 PST
Created attachment 179263 [details]
Patch
Comment 2 Jocelyn Turcotte 2012-12-13 06:37:19 PST
Tell me if I'm overseeing cases where we would need to override this in the WebKit's configure step. This patch is only valid if we can agree on this.
Comment 3 Simon Hausmann 2012-12-13 07:26:55 PST
If we do this, then we must also change the way we link against libpng. IOW: 

Currently we use pkg-config to _discover_ the presence of libpng/libjpeg and we also use to determine the include search paths, compiler flags, library search path and library name (through PKGCONFIG += libpng)

If we use the system-png to determine the presence, then we should also link against it like Qt, i.e. rely other ways of determining the include search path for example. From qpnghandler.pri:

contains(QT_CONFIG, system-png) {
    if(unix|win32-g++*): LIBS_PRIVATE  += -lpng
    else:win32:          LIBS += libpng.lib

}


Also, if we do this, then we should do it consistently also for the other third party libraries like sqlite (which I recently just changed over from system-sqlite to use pkg-config ;-)
Comment 4 Simon Hausmann 2012-12-13 07:28:47 PST
It would also be good if this bug report would explain the reasoning for this change, so that if we decide to go for it and later come back we have the arguments in this bug report :)
Comment 5 Early Warning System Bot 2012-12-13 07:42:13 PST
Comment on attachment 179263 [details]
Patch

Attachment 179263 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15316281
Comment 6 Jocelyn Turcotte 2012-12-13 07:42:47 PST
Comment on attachment 179263 [details]
Patch

Yep so I agree that this is not a good solution, but the current situation might soon be problematic too.
I talked with Tobias a bit and we'll run into problems if Qt is configured to use any other version than the system default path's libraries and that we try to load two versions of the same library as a result of WebKit having its own system dependencies.
This hasn't been a problem before since we always religiously tried not to use anything outside qt/src/3rdparty.

A solution would be to delegate all our configuration work to Qt and get the result down to every modules.
"Oh you need sqlite? Here I did the work for you, just add this and this to your build flags."

If necessary, the need behind this patch is mostly explained in the ChangeLog.
Comment 7 Simon Hausmann 2012-12-13 07:51:17 PST
(In reply to comment #6)
> (From update of attachment 179263 [details])
> Yep so I agree that this is not a good solution, but the current situation might soon be problematic too.

I think it depends. The current solution for example is for example rather suitable for cross-compilation with sysroots and pkg-config.

> I talked with Tobias a bit and we'll run into problems if Qt is configured to use any other version than the system default path's libraries and that we try to load two versions of the same library as a result of WebKit having its own system dependencies.
> This hasn't been a problem before since we always religiously tried not to use anything outside qt/src/3rdparty.
> 
> A solution would be to delegate all our configuration work to Qt and get the result down to every modules.
> "Oh you need sqlite? Here I did the work for you, just add this and this to your build flags."

Yes. Either we let Qt do all the work (discovery, build and link settings) or _both_ use an external mechanism like pkg-config. Right now we have a hybrid in the sense that Qt uses pkg-config for many libs but not all and WebKit always uses pkg-config.

Wouldn't it be another option btw to configure Qt with -no-pkg-config? WebKit should obey that.

Oh wait, the current situation is not how I described it in WebKit, it's in fact worse. We don't even use pkg-config to _discover_ libpng. Argh. 

> If necessary, the need behind this patch is mostly explained in the ChangeLog.

Ooops, sorry, I was too quick on that one :)
Comment 8 Simon Hausmann 2012-12-13 07:54:28 PST
Comment on attachment 179263 [details]
Patch

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

> Tools/qmake/mkspecs/features/features.prf:48
> -    config_libjpeg: WEBKIT_CONFIG += use_libjpeg
> -    else: CONFIGURE_WARNINGS += "JPEG library not found, QImageDecoder will decode JPEG images"
> -
> -    config_libpng: WEBKIT_CONFIG += use_libpng
> -    else: CONFIGURE_WARNINGS += "PNG library not found, QImageDecoder will decode PNG images"
> +    # We can't use Qt's 3rdparty sources for libjpeb and libpng outside of qtbase, but if Qt
> +    # is using the system libraries, use them to take advantage of the WebCore image decoders as well.
> +    contains(QT_CONFIG, system-jpeg): WEBKIT_CONFIG += use_libjpeg
> +    contains(QT_CONFIG, system-png): WEBKIT_CONFIG += use_libpng

Shouldn't we still issue a warning? I mean, the choice of _not_ having system-jpeg comes at a performance cost that the CONFIGURE_WARNINGS += should sort of convey, no?

Otherwise this is clearly an improvement over the current situation. Although I'm starting to think that we should use pkg-config instead :)
Comment 9 Jocelyn Turcotte 2012-12-14 04:58:54 PST
Created attachment 179466 [details]
Patch

Ok, I'm still concerned that this might break things for common mortals in the future in favor of the few that needs to package redistributables, but until then this can be considered a step forward.
This patch version only re-add the warnings.
Comment 10 Early Warning System Bot 2012-12-14 05:13:06 PST
Comment on attachment 179466 [details]
Patch

Attachment 179466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15314641
Comment 11 Early Warning System Bot 2012-12-14 05:15:22 PST
Comment on attachment 179466 [details]
Patch

Attachment 179466 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15309690
Comment 12 Simon Hausmann 2012-12-14 05:26:13 PST
Comment on attachment 179466 [details]
Patch

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

r=me. It's strange that the EWS is complaining. Land with care manually I'd say :)

> Tools/qmake/mkspecs/features/features.prf:45
> +    # We can't use Qt's 3rdparty sources for libjpeb and libpng outside of qtbase, but if Qt

libjpeb -> libjpeg
Comment 13 Jocelyn Turcotte 2013-01-14 03:49:30 PST
Created attachment 182542 [details]
Patch

Fixed the type and re-uploading to see if the EWS is still broken.
Comment 14 Build Bot 2013-01-14 04:14:21 PST
Comment on attachment 182542 [details]
Patch

Attachment 182542 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15853497

New failing tests:
svg/as-image/img-relative-height.html
Comment 15 Jocelyn Turcotte 2013-01-14 04:43:49 PST
Comment on attachment 182542 [details]
Patch

It seems like it works now on the Qt bots, CQing it.
Comment 16 WebKit Review Bot 2013-01-14 04:45:51 PST
Comment on attachment 182542 [details]
Patch

Rejecting attachment 182542 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15841485
Comment 17 Csaba Osztrogonác 2013-01-14 04:50:22 PST
(In reply to comment #15)
> (From update of attachment 182542 [details])
> It seems like it works now on the Qt bots, CQing it.

We always build Qt with "-qt-libpng -qt-libjpeg". I don't know why, 
but somebody told us to use it a long time ago, and we've never
changed/reconsidered config options.

Now we use WebCore's imagedecoders and this change will force us
to use qt's imagedecoders. I'm not sure if the qmake build system
handles this situation correctly ...
Comment 18 WebKit Review Bot 2013-01-14 05:02:46 PST
Comment on attachment 182542 [details]
Patch

Clearing flags on attachment: 182542

Committed r139609: <http://trac.webkit.org/changeset/139609>
Comment 19 WebKit Review Bot 2013-01-14 05:02:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Jocelyn Turcotte 2013-01-14 05:13:12 PST
(In reply to comment #17)
> We always build Qt with "-qt-libpng -qt-libjpeg". I don't know why, 
> but somebody told us to use it a long time ago, and we've never
> changed/reconsidered config options.
> 
> Now we use WebCore's imagedecoders and this change will force us
> to use qt's imagedecoders. I'm not sure if the qmake build system
> handles this situation correctly ...

In that case it will switch to the qt's image decoders yep. It would be better if WebKit continued using the system libraries.

If we would now use the system libpng and libjpeg, it feels to me that any advantage that those configure switches would give was lost when we decided that WebKit would use the system libraries.
This is also an example why I think it's nice to have WebKit follow Qt's behavior.

Could you try removing those configure switches or maybe try to remember what would be the benefit of keeping them?
Comment 21 Csaba Osztrogonác 2013-01-14 23:58:16 PST
(In reply to comment #20)
> Could you try removing those configure switches or maybe try to remember what would be the benefit of keeping them?

Sure, will check near the next Qt update. But as far as I remember the
benefit was that we didn't want to add libjpeg/libpng as build dependency
long long time ago near Qt 4.5 / 4.6. (2-3 years before)
Comment 22 Csaba Osztrogonác 2013-01-15 02:32:35 PST
I tried the released Qt 5.0.0 without "-qt-libpng -qt-libjpeg" configure
options, and tests work fine with system libpng/libjpeg.

We had to skip 3 tests because of http://trac.webkit.org/changeset/139609:
 - fast/events/mouse-cursor-multiframecur.html 
 - fast/events/mouse-cursor.html 
 - fast/images/icon-decoding.html

Will unskip after Qt update.