Switch to immutable texture storage and invalidate unused framebuffers #3195

bholley · 2018-10-12T20:16:54Z

Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1496168

This change is


        Bump gleam to 0.6.3.


        Switch FormatDesc::internal to GLenum.

That's the proper type. The fact that the glTexImage* calls take a glint is a specific wart of that API that's not relevant to us here. Differential Revision: https://phabricator.services.mozilla.com/D8564

bholley · 2018-10-12T20:17:02Z

@bors-servo r=kvark

bors-servo · 2018-10-12T20:17:03Z

📌 Commit 7f625ca has been approved by kvark

bors-servo · 2018-10-12T20:17:06Z

⌛ Testing commit 7f625ca with merge 0c91d47...


        Auto merge of #3195 - bholley:immutable_storage, r=kvark

Switch to immutable texture storage and invalidate unused framebuffers Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1496168  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3195)

bors-servo · 2018-10-12T20:57:29Z

💔 Test failed - status-taskcluster

jrmuizel · 2018-10-12T21:08:23Z

thread 'main' panicked at 'assertion failed: texture.target == gl::TEXTURE_2D || texture.target == gl::TEXTURE_RECTANGLE ||
    texture.target == gl::TEXTURE_EXTERNAL_OES', webrender/src/device/gl.rs:1671:9


        Make creation-time immediate texture upload explicit.

The current setup doesn't work with glTexStorage. Given that it's only used for a few callers, it's cleanest to move it to an explicit call at those sites. Differential Revision: https://phabricator.services.mozilla.com/D8565


        Always use a sized internal format.

This is required by glTexStorage*. We also take the opportunity to simplify and clarify the situation with BGRA. Differential Revision: https://phabricator.services.mozilla.com/D8566


        Use glTexStorage* where available.

Differential Revision: https://phabricator.services.mozilla.com/D8567


        Invalidate render targets when returning them to the pool.

This allows the driver to avoid preserving the old contents we don't care about. Differential Revision: https://phabricator.services.mozilla.com/D8568

bholley · 2018-10-13T00:21:43Z

I had to add support for the texture array case to upload_texture_immediate to support loading captures of the texture cache.

bholley · 2018-10-13T00:21:58Z

@bors-servo r=kvark

bors-servo · 2018-10-13T00:21:59Z

📌 Commit 36cb19d has been approved by kvark

bors-servo · 2018-10-13T00:22:02Z

⌛ Testing commit 36cb19d with merge 5adc86c...


        Auto merge of #3195 - bholley:immutable_storage, r=kvark

Switch to immutable texture storage and invalidate unused framebuffers Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1496168  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3195)

bors-servo · 2018-10-13T01:24:14Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 5adc86c to master...

staktrace · 2018-10-17T18:52:07Z

@bholley @kvark any ideas off the top of your head why this PR would result in all the glyphs and images turning into black rectangles on Android? https://bugzilla.mozilla.org/show_bug.cgi?id=1499785

bholley · 2018-10-17T19:00:31Z

I'll comment in the bug.

kvark · 2018-10-17T21:29:39Z

@staktrace re-reading the code, maybe we are using the wrong external format now?
Old code had gl::BGRA_EXT or gl::RGBA. The new code has gl::BGRA always as the external format. Although, this wouldn't turn them black, I suppose.

bholley · 2018-10-17T22:46:24Z

Old code had gl::BGRA_EXT or gl::RGBA. The new code has gl::BGRA always as the external format.

I think the old code was nonsensical. IIUC, the external format describes the arrangement of pixels delivered by the application, which wouldn't change based on whether GL_EXT_texture_format_BGRA8888 is supported by the driver. ImageFormat::BGRA is going to have pixels in that order.

Although, this wouldn't turn them black, I suppose.

Yeah, it would just swap around the colors. And that's exactly what jdm reported in #2731. :-)

I'm pretty sure the black means that the textures aren't getting created at all. Driver logs are probably the next step.

bholley added 2 commits Oct 12, 2018

Bump gleam to 0.6.3.

5a914be

Switch FormatDesc::internal to GLenum.

6e80ae2

That's the proper type. The fact that the glTexImage* calls take a glint is a specific wart of that API that's not relevant to us here. Differential Revision: https://phabricator.services.mozilla.com/D8564

bholley added 4 commits Oct 11, 2018

Make creation-time immediate texture upload explicit.

b79e230

The current setup doesn't work with glTexStorage. Given that it's only used for a few callers, it's cleanest to move it to an explicit call at those sites. Differential Revision: https://phabricator.services.mozilla.com/D8565

Always use a sized internal format.

ffa6bb3

This is required by glTexStorage*. We also take the opportunity to simplify and clarify the situation with BGRA. Differential Revision: https://phabricator.services.mozilla.com/D8566

Use glTexStorage* where available.

ccdede2

Differential Revision: https://phabricator.services.mozilla.com/D8567

Invalidate render targets when returning them to the pool.

Loading status checks…

36cb19d

This allows the driver to avoid preserving the old contents we don't care about. Differential Revision: https://phabricator.services.mozilla.com/D8568

bholley force-pushed the bholley:immutable_storage branch from 7f625ca to 36cb19d Oct 13, 2018

bors-servo merged commit 36cb19d into servo:master Oct 13, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

Darkspirit mentioned this pull request Nov 6, 2018

Android nightly fails to render any web servo/servo#22114

Closed

jamienicol mentioned this pull request Nov 15, 2018

GL setup is busted on android emulator #3312

Closed

servo / webrender

Switch to immutable texture storage and invalidate unused framebuffers #3195

Switch to immutable texture storage and invalidate unused framebuffers #3195

bholley commented Oct 12, 2018 •

edited by larsbergstrom

bholley commented Oct 12, 2018

bors-servo commented Oct 12, 2018

bors-servo commented Oct 12, 2018

bors-servo commented Oct 12, 2018

jrmuizel commented Oct 12, 2018

bholley commented Oct 13, 2018

bholley commented Oct 13, 2018

bors-servo commented Oct 13, 2018

bors-servo commented Oct 13, 2018

bors-servo commented Oct 13, 2018

staktrace commented Oct 17, 2018 •

edited

bholley commented Oct 17, 2018

kvark commented Oct 17, 2018

bholley commented Oct 17, 2018

servo / webrender

Join GitHub today

Switch to immutable texture storage and invalidate unused framebuffers #3195

Switch to immutable texture storage and invalidate unused framebuffers #3195

Conversation

bholley commented Oct 12, 2018 • edited by larsbergstrom

bholley commented Oct 12, 2018

bors-servo commented Oct 12, 2018

bors-servo commented Oct 12, 2018

bors-servo commented Oct 12, 2018

jrmuizel commented Oct 12, 2018

bholley commented Oct 13, 2018

bholley commented Oct 13, 2018

bors-servo commented Oct 13, 2018

bors-servo commented Oct 13, 2018

bors-servo commented Oct 13, 2018

staktrace commented Oct 17, 2018 • edited

bholley commented Oct 17, 2018

kvark commented Oct 17, 2018

bholley commented Oct 17, 2018

bholley commented Oct 12, 2018 •

edited by larsbergstrom

staktrace commented Oct 17, 2018 •

edited