Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1244196 - Review Request: qt5-qtwebengine - Qt5 - QtWebEngine components
Summary: Review Request: qt5-qtwebengine - Qt5 - QtWebEngine components
Keywords:
Status: CLOSED DUPLICATE of bug 1295549
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: qt-reviews
TreeView+ depends on / blocked
 
Reported: 2015-07-17 12:53 UTC by Helio Chissini de Castro
Modified: 2016-01-16 23:05 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-04 19:14:10 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch (55.49 KB, patch)
2015-12-21 06:35 UTC, Kevin Kofler
no flags Details | Diff
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v2 (55.46 KB, patch)
2015-12-21 07:04 UTC, Kevin Kofler
no flags Details | Diff
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v3 (55.47 KB, patch)
2015-12-21 07:11 UTC, Kevin Kofler
no flags Details | Diff
[incomplete upload] (7.35 KB, patch)
2015-12-21 16:25 UTC, Kevin Kofler
no flags Details | Diff
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v4 (55.65 KB, patch)
2015-12-21 16:32 UTC, Kevin Kofler
no flags Details | Diff
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v5 (84.55 KB, patch)
2015-12-21 17:09 UTC, Kevin Kofler
no flags Details | Diff
qtwebengine-spec-ffmpeg-clean.patch (13.38 KB, patch)
2015-12-24 01:31 UTC, Kevin Kofler
no flags Details | Diff

Description Helio Chissini de Castro 2015-07-17 12:53:24 UTC
Spec URL: https://heliocastro.fedorapeople.org/qt5/qt5-qtwebengine.spec
SRPM URL: https://heliocastro.fedorapeople.org/qt5/qt5-qtwebengine-5.5.0-1.fc22.src.rpm
Description: Qt5 - QtWebEngine components
Fedora Account System Username: heliocastro

Comment 1 Rex Dieter 2015-07-17 12:58:14 UTC
this one will be fun.

Comment 2 Kevin Kofler 2015-07-17 13:05:15 UTC
* The non-echoing of build commands is a clear review blocker.
* The bundled libraries are also a clear review blocker. (You haven't even unbundled those libraries that only require a configure flag to unbundle.)
* You also have bogus BuildRequires. In particular, GStreamer, which is NOT used anywhere as far as I can tell.

Comment 3 Helio Chissini de Castro 2015-07-20 21:42:39 UTC
Update:

https://heliocastro.fedorapeople.org/qt5/qt5-qtwebengine.spec
https://heliocastro.fedorapeople.org/qt5/qt5-qtwebengine-5.5.0-2.fc22.src.rpm

- Echoing command is NOT blocker, although is doing it right now
- Boguis requires are removed, but is not blocker at all

- Unbundled most of the libraries, with exceptions:

ffmpeg - Usage of internal one
sqlite3 - internal sqlite3 add some functions still not launched upstream
libvpx - Single function not present on Fedora 1.4.0
v8 - Fedora version is too old

Comment 4 Kevin Kofler 2015-07-21 13:54:22 UTC
> v8 - Fedora version is too old

Then the BuildRequires: v8-devel is bogus.

Comment 5 Daniel Vrátil 2015-07-25 21:33:20 UTC
News from Akademy: the upstream maintainer managed to unbundle many things directly in QtWebEngine for Qt 5.6, including ffmpeg, which is great news.

Comment 6 Kevin Kofler 2015-08-06 01:08:44 UTC
Unbundled FFmpeg is not of any help to us, because FFmpeg is not allowed in Fedora.

We need to try to patch this:
http://blogs.s-osg.org/announcing-a-new-gstreamer-backend-for-chromium/
into our QtWebEngine builds.

Comment 7 Kevin Kofler 2015-08-06 01:28:23 UTC
Looks like that code will only work on Fedora ≥ 23 though (needs GStreamer ≥ 1.5.2, F23 has it already, F22 has only 1.4.5).

Comment 8 Helio Chissini de Castro 2015-08-06 01:35:52 UTC
Thanks Kevin

I will check it tomorrow

Comment 9 Kevin Kofler 2015-08-06 01:47:28 UTC
There's another showstopper though: According to http://code.google.com/p/v8/ , V8 requires SSE2 on 32-bit x86, which is not allowed by Fedora policy. (Of course, secondary architectures are also screwed, but there is no policy against that.)

Comment 10 Kevin Kofler 2015-08-06 14:16:00 UTC
Wow, another miracle, this time going on at Chromium upstream:
https://chromium.googlesource.com/v8/v8/+log/master/src/interpreter
(started 2 weeks ago, probably not usable yet). When done, and when it reaches (or is backported by us into) QtWebEngine, this will cover the secondary architectures, and through a similar setup as for QtDeclarative, non-SSE2 x86.

Comment 11 Kevin Kofler 2015-08-06 21:50:51 UTC
Until that happens, I think we should treat the V8 portability issue as a non-blocker. Especially, because technically, one could get around the SSE2 issue by excluding i686 entirely, but it is of course better to ship a package that works on many-but-not-all i686 machines than none at all. We should just have a tracker bug for the SSE2 issue, as would be required for ExcludeArch. Maybe even put it on the x86 ExcludeArch tracker, even though we won't actually exclude the arch (because that'd be silly).

Comment 12 Kevin Kofler 2015-08-07 12:01:36 UTC
For GStreamer, the bad news is that disabling FFmpeg in favor of GStreamer is not supported yet:
https://github.com/Samsung/ChromiumGStreamerBackend/issues/3

Comment 13 Kevin Kofler 2015-08-07 12:47:07 UTC
If we can get it to build with media_use_ffmpeg=0, then that should work (but it will still compile a lot of the default media backend, just without the codecs).

Comment 14 Kevin Kofler 2015-08-08 01:34:02 UTC
The minimum to do to get a legally redistributable tarball is to run the clean_ffmpeg.sh script (uses process_ffmpeg_gyp.py) from spot's Chromium SRPMs. (That doesn't unbundle or remove FFmpeg, mind you, it just removes the encumbered files from it.) Simpler would be to rm -rf third_party/ffmpeg entirely, if we can get stuff to build without it (but then we need the GStreamer stuff or codec support will be almost nonexistent).

This might also be needed, if QtWebEngine is including those reference_build directories too:
find . -depth -name reference_build -type d -exec rm -rf {} \;

Comment 15 Kevin Kofler 2015-12-21 01:59:06 UTC
For the SSE2 stuff, I found out Chromium had actually supported non-SSE2 until mid-2014:
http://code.google.com/p/chromium/issues/detail?id=348761

This contains a bunch of commits that need to be reverted in Chromium itself to make non-SSE2 work. For V8, there was x87 support in there until 2014, it will need to be resurrected.

Comment 16 Kevin Kofler 2015-12-21 02:00:40 UTC
This is an attempt by somebody to restore non-SSE2 support, but it is neither up to date nor complete:
https://github.com/bircoph/chromium-no-sse2-patch

Comment 17 Kevin Kofler 2015-12-21 02:07:08 UTC
See also:
https://groups.google.com/forum/#!topic/v8-users/cIVgTxcDrNI
for the V8 part of the story, pointing to 2 revisions that would need to be reverted for a starter.

Comment 18 Kevin Kofler 2015-12-21 02:12:15 UTC
This is the referenced patchset to revert for V8:
https://codereview.chromium.org/275433004

The other one:
https://codereview.chromium.org/275253004
is about CMOV, but since we already require CMOV in Fedora, that one need not be reverted.

But since your packaging apparently uses system V8, I guess I need to file this against the system V8 package.

Comment 19 Kevin Kofler 2015-12-21 02:32:46 UTC
Actually, we don't have to patch V8: Shortly after that announcement, a separate x87 backend (x86 with x87 FP, no SSE2) was added to V8:
https://github.com/v8/v8/tree/master/src/x87

So V8 support is actually already there in the source code, we just need the system v8 package to use it (as far as I can see, it currently doesn't) if we are building against system V8. (The last Chromium no-sse2 patch from https://github.com/bircoph/chromium-no-sse2-patch in fact already did set the V8 backend to x87.)

There's only the Chromium part to fix, and that's probably not that hard. At least it shouldn't require messing with a JIT compiler.

Comment 20 Kevin Kofler 2015-12-21 02:47:51 UTC
I have just filed bug #1293190: v8 on i686 should be built with the x87 arch, possibly with ia32 builds in lib/sse2.

Comment 21 Kevin Kofler 2015-12-21 03:08:11 UTC
No wonder the patch against Chromium v38 from https://github.com/bircoph/chromium-no-sse2-patch does not work: It removes -msse2, but not -mfpmath=sse, which also requires SSE2! It is also otherwise hackish, in particular, it completely removes SSE2 support for x86 instead of making it conditional on ENABLE_SSE2. We can definitely do better.

The "no-sse4" patch that patches the bundled libvpx is also bogus, because that code (and also the SSE2 code that is also there) is runtime-detected and as such should not be an issue.

Comment 22 Kevin Kofler 2015-12-21 06:35:09 UTC
Created attachment 1108190 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch

The attached (not yet tested in any way) patch should make QtWebEngine run on non-SSE2 x86 CPUs.

The patch is a cumulative (forward-ported) revert of upstream reviews 187423002, 511773002 (parts relevant to QtWebEngine only), 516543004, 1152053004 and 1161853008, along with some custom fixes (for example, the third-party qcms supports runtime detection of SSE2, my patch should make this work instead of just disabling SSE2 support entirely as Chromium was doing before they started requiring SSE2).

If you think this patch is too large, I can do a version that does not readd the removed (no longer supported by upstream) MMX- and SSE(1)-optimized routines in "media" (those where an SSE2 version exists), always falling back to the plain C ones for non-SSE2 instead. (The patch as it is now restores the full runtime detection, which is IMHO nicer.)

The current QtWebEngine only uses gyp, but I tried to also fix the gn setup because I guess it will be used in the future. I did not find the place where gn sets the -msse2 -mfpmath=sse -mmmx flags though, so that will need to be addressed separately when needed.

For those who care about the original Chromium, you will also want to revert the remaining parts of review 511773002 and the review 296663002. I cannot do this in this patch because QtWebEngine does not ship those files at all.

Comment 23 Kevin Kofler 2015-12-21 07:04:40 UTC
Created attachment 1108193 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v2

This version fixes the MMX stuff in src/3rdparty/chromium/media/base/simd/convert_yuv_to_rgb.h: The assembly code was actually correct because it was still updated by upstream as it is shared with the SSE version, but the C prototypes were not (they were outdated). (I realized this while proofreading the patch.)

Comment 24 Kevin Kofler 2015-12-21 07:11:16 UTC
Created attachment 1108194 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v3

This version fixes a missing closing brace in src/3rdparty/chromium/media/base/yuv_convert.cc (again found through proofreading).

Comment 25 Kevin Kofler 2015-12-21 16:25:56 UTC
Created attachment 1108332 [details]
[incomplete upload]

This version fixes src/3rdparty/chromium/third_party/qcms/BUILD.gn to define SSE2_ENABLE (needed to enable SSE2 runtime detection) for x86. qcms.gyp already got this right, I just added a comment there clarifying that SSE2_ENABLE enables runtime detection here.

Comment 26 Kevin Kofler 2015-12-21 16:32:58 UTC
Created attachment 1108333 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v4

(This time hopefully complete. I had accidentally uploaded the patch while diff was still running.)

This version fixes src/3rdparty/chromium/third_party/qcms/BUILD.gn to define SSE2_ENABLE (needed to enable SSE2 runtime detection) for x86. qcms.gyp already got this right, I just added a comment there clarifying that SSE2_ENABLE enables runtime detection here.

Comment 27 Kevin Kofler 2015-12-21 17:09:12 UTC
Created attachment 1108340 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v5

This version restores runtime detection for the last 2 places that were missing it: the SSE1 code in media/base. Instead of my previous quick hack that disabled the SSE routines entirely, I reverted review 308003004 properly. (Rebase hint: Most of the code there is just cut from one file and pasted into the other, so if you get a reject on the removal part, just redo the cut&paste and the _sse.cc file will be up to date as well. This is in fact what I did for sinc_resampler.)

Comment 28 Kevin Kofler 2015-12-21 18:38:46 UTC
As far as I can tell, what blocks the review is that the source tarball needs to be cleaned using this shell script:
http://copr-dist-git.fedorainfracloud.org/cgit/spot/chromium/chromium.git/tree/clean_ffmpeg.sh?id=474ebe047c82342cf37b722681981f6ab2a793da
and this Python script that is called by the above:
http://copr-dist-git.fedorainfracloud.org/cgit/spot/chromium/chromium.git/tree/process_ffmpeg_gyp.py?id=474ebe047c82342cf37b722681981f6ab2a793da

This just needs to be adapted to be run inside the 3rdparty/chromium directory.

Comment 29 Kevin Kofler 2015-12-22 00:45:29 UTC
I did a quick test: No matter what method is used to find libraries (systemwide ld.so search path, LD_LIBRARY_PATH or rpath), ld.so will ALWAYS look for sse2/*.so in that path before taking *.so. What this means for us is that even if we build a shared libv8.so in a private directory, we can provide an sse2/libv8.so override in the same private directory. So this can be used to make it pick the right V8 build at runtime. (This is not currently implemented in the no-sse2 patch, it just builds V8 once with the x87 target.)

Comment 30 Kevin Kofler 2015-12-24 01:31:15 UTC
Created attachment 1109065 [details]
qtwebengine-spec-ffmpeg-clean.patch

Here is a patch to the specfile that implements the required FFmpeg cleanup. In addition to patching the specfile, it also creates 3 scripts: clean_ffmpeg.sh and process_ffmpeg_gyp.py are verbatim from spot's Chromium packaging, clean_qtwebengine.sh is a small wrapper around clean_ffmpeg.sh that I wrote for our purposes.

The usage is documented in the specfile:
wget http://download.qt.io/official_releases/qt/5.5/5.5.1/submodules/qtwebengine-opensource-src-5.5.1.tar.xz
./clean_qtwebengine.sh 5.5.1
→ Source0: qtwebengine-opensource-src-5.5.1-clean.tar.xz

./clean_qtwebengine.sh 5.6.0-beta also works (and it accepts both the 7z and the tar.gz: the script looks for a tar.xz, tar.bz2, tar.gz and 7z in that order, others such as zip can easily be added when/if needed; the output is ALWAYS a tar.xz).

To generate the SRPM, just run the above 2 commands and rpmbuild -bs qt5-qtwebengine.spec. I haven't uploaded it because it is huge.

Comment 31 Kevin Kofler 2015-12-28 18:57:07 UTC
https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews#Submitter_not_responding
> When the submitter of a review ticket has not responded to comments for one month, a comment is added to the ticket indicating that the review is stalled and that a response is needed soon. 
> If there is no response within one week, the ticket is closed with resolution NOTABUG, and the fedora-review flag is set to the empty value.

Comment 32 Kevin Kofler 2015-12-28 18:57:43 UTC
(The last comment from the submitter was comment #8 at 2015-08-05 21:35:52 EDT.)

Comment 33 Kevin Kofler 2016-01-04 19:14:10 UTC
The one week is over now, still no response from the submitter, closing as per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews#Submitter_not_responding .

I can submit a new review request in a few minutes, I'm just waiting for my Copr build to complete.

Comment 34 Kevin Kofler 2016-01-04 19:48:05 UTC

*** This bug has been marked as a duplicate of bug 1295549 ***

Comment 35 Kevin Kofler 2016-01-16 23:05:29 UTC
Comment on attachment 1108340 [details]
qtwebengine-opensource-src-5.6.0-beta-no-sse2.patch v5

The no-sse2 patch attached here is incomplete (missing at least fixes for WebGL image conversion and for webrtc's use of OpenMAX) and thus obsolete. I am now working on this in dist-git.


Note You need to log in before you can comment on or make changes to this bug.