Bug 104909 – [Qt] The Qt's configuration isn't honoured regarding the use of the system libpng and libjpeg

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

[Qt] The Qt's configuration isn't honoured regarding the use of the system libpng and libjpeg

[Jocelyn Turcotte]

Created attachment 179263 [details]
Patch

[Jocelyn Turcotte]

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.

[Simon Hausmann]

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 ;-)

[Simon Hausmann]

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 :)

[Early Warning System Bot]

Comment on attachment 179263 [details]
Patch

Attachment 179263 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15316281

[Jocelyn Turcotte]

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.

[Simon Hausmann]

(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 :)

[Simon Hausmann]

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 :)

[Jocelyn Turcotte]

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.

[Early Warning System Bot]

Comment on attachment 179466 [details]
Patch

Attachment 179466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15314641

[Early Warning System Bot]

Comment on attachment 179466 [details]
Patch

Attachment 179466 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15309690

[Simon Hausmann]

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

[Jocelyn Turcotte]

Created attachment 182542 [details]
Patch

Fixed the type and re-uploading to see if the EWS is still broken.

[Build Bot]

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

[Jocelyn Turcotte]

Comment on attachment 182542 [details]
Patch

It seems like it works now on the Qt bots, CQing it.

[WebKit Review Bot]

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

[Csaba Osztrogonác]

(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 ...

[WebKit Review Bot]

Comment on attachment 182542 [details]
Patch

Clearing flags on attachment: 182542

Committed r139609: <http://trac.webkit.org/changeset/139609>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jocelyn Turcotte]

(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?

[Csaba Osztrogonác]

(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)

[Csaba Osztrogonác]

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.