QtMir

Merge lp://qastaging/~gerboland/qtmir/fix-font-rendering into lp://qastaging/qtmir

  1. fix-font-rendering
  2. Merge into trunk
Proposed by Gerry Boland
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 598
Merged at revision: 613
Proposed branch: lp://qastaging/~gerboland/qtmir/fix-font-rendering
Merge into: lp://qastaging/qtmir
Diff against target: 12 lines (+4/-0)
1 file modified
src/modules/Unity/Application/mirbuffersgtexture.cpp (+4/-0)
To merge this branch: bzr merge lp://qastaging/~gerboland/qtmir/fix-font-rendering
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Daniel d'Andrada (community) test Approve
Unity8 CI Bot (community) continuous-integration Approve
Review via email: mp+317239@code.qastaging.launchpad.net

Commit message

Workaround for Mir changing GL state unexpectedly on Qt, breaking font rendering.

For non-GL clients, Mir uploads the client pixel buffer to a GL texture which makes compositor code nicer. But as it does to, it changes some GL state (mainly the stride = GL_UNPACK_ALIGNMENT) and neglects to restore it, which breaks Qt's font rendering (which expects a certain stride).

Description of the change

To test, have unity8 running and launch

mir_demo_client_fingerpaint -- --desktop_file_hint=gedit

Then start opening the indicators, without this patch the text can be corrupted.

To post a comment you must log in.
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

"But as it does to"

typo?

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:596
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/508/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4108
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4136
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3976/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3976/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3976/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3976/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3976/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3976
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3976/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/508/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

Fixed

lp://qastaging/~gerboland/qtmir/fix-font-rendering updated
597. By Gerry Boland

Fix typo

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:597
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/509/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4110
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4138
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3978/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3978/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3978/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3978/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3978/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3978
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3978/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/509/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

We need to avoid calling glGetIntegerv. It is slow, and horrendously slow on some drivers like nvidia.

https://www.khronos.org/opengl/wiki/Common_Mistakes#glGetFloatv_glGetBooleanv_glGetDoublev_glGetIntegerv

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

So the answer is to either glPixelStorei(GL_UNPACK_ALIGNMENT, 4) everywhere Qt needs it set to four, or to perhaps (somehow) reduce intermingling Mir/Qt code that modifies the GL state.

lp://qastaging/~gerboland/qtmir/fix-font-rendering updated
598. By Gerry Boland

More efficient to just reset to known default, instead of read/save/write per texture, to avoid pipeline flush

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:598
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/510/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4111
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4139
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/3979/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/3979/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/3979/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/3979/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/3979/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3979
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/3979/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/510/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Fixes the bug.

review: Approve (test)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Cool.

I think a proper fix should eventually go in qtdeclarative though. Assuming that's where the offending glyph cache logic is. Somewhere in there they are moving pixels between main memory and the GPU with the wrong unpack alignment assumed (not explicitly set).

review: Approve
Revision history for this message
Gerry Boland (gerboland) wrote :

> Cool.
>
> I think a proper fix should eventually go in qtdeclarative though. Assuming
> that's where the offending glyph cache logic is. Somewhere in there they are
> moving pixels between main memory and the GPU with the wrong unpack alignment
> assumed (not explicitly set).

Here is what Qt is doing, on upload:
https://code.woboq.org/qt5/qtdeclarative/src/quick/scenegraph/qsgdefaultdistancefieldglyphcache.cpp.html#158
It saves & restores the alignment. Mir isn't

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Whoa, qtdeclarative needs fixing. It may well avoid the bug by calling glGetIntegerv, but that's an unacceptable performance hit in GL as mentioned above.

Some time later when we profile Unity8 again we will eventually encounter the glGetIntegerv calls as a significant bottleneck, and they will need to be eliminated.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches