1311642 - Partial GL Texture Upload is a bit of a mess

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Jamie Nicol [:jnicol]]

When debugging a different issue I noticed that, on desktop, if CanUploadSubTextures() in GLUploadHelpers.cpp returns false, we will reinitialize the texture to be the size of the updated region. This is because we will avoid calling glTexSubImage2D and call glTexImage2D instead. TextureImage should not pass a subregion of texture data to UploadSurfaceToTexture() if partial upload is not supported.

While plastering over that is trivial, it stems from the fact that GLUploadHelpers try to be too helpful IMO. I would much prefer it if we provided two separate helpers, one for initialize-and-optionally-upload, and one for partial upload. Then TextureImage would have to take responsibility for checking if partial upload is allowed.

This hasn't been a problem in practice because CanUploadSubTextures() returns true for all desktop GPUs, and on mobile we always pass the full region to UploadSurfaceToTexture(). This is a deliberate choice made in TiledContentHost.cpp "as sub-image upload can often be slow and/or unreliable". The comment goes on to say we might need to re-evaluate that at some point, I think we now should. I will do some profiling. In any case I think that this is the wrong place to make this decision, it should be done in GLUploadHelpers/CanUploadSubTextures() instead. And I'm not even sure whether it's working as intended anyway, we still call glTexSubImage2D, just with the full width and height.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1311642 - Remove BeginUpdate and EndUpdate from GLTextureImage.

Only DirectUpdate was actually being used, so remove the dead code.

Review commit: https://reviewboard.mozilla.org/r/87662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87662/

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: Bug 1311642 - Give GLUploadHelpers some love.

GLUploadHelpers was trying to be too flexible, so remove some unused
options it provided. Require that the full surface and texture size is
passed in rather than only the updated subregion. This prevents textures
being initialized to an incorrect size when partial texture uploads are
disallowed.

Review commit: https://reviewboard.mozilla.org/r/87664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87664/

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Bug 1311642 - Do partial texture uploads on mobile.

We were always uploading the entire texture region, but still by using
glTexSubImage2D. In most cases it should be quicker to use
glTexSubImage2D on the updated region only. And where that function has
problems we should avoid it by making CanUploadSubTextures return false
so that glTexImage2D is used instead.

Review commit: https://reviewboard.mozilla.org/r/87666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87666/

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8803360 [details]
Bug 1311642 - Remove BeginUpdate and EndUpdate from GLTextureImage.

https://reviewboard.mozilla.org/r/87662/#review86630

Neat!

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8803361 [details]
Bug 1311642 - Give GLUploadHelpers some love.

https://reviewboard.mozilla.org/r/87664/#review86632

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8803362 [details]
Bug 1311642 - Do partial texture uploads on mobile.

https://reviewboard.mozilla.org/r/87666/#review86634

Comment 7

[Jamie Nicol [:jnicol]]

Comment on attachment 8803360 [details]
Bug 1311642 - Remove BeginUpdate and EndUpdate from GLTextureImage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87662/diff/1-2/

Comment 8

[Jamie Nicol [:jnicol]]

Comment on attachment 8803361 [details]
Bug 1311642 - Give GLUploadHelpers some love.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87664/diff/1-2/

Comment 9

[Jamie Nicol [:jnicol]]

Comment on attachment 8803362 [details]
Bug 1311642 - Do partial texture uploads on mobile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87666/diff/1-2/

Comment 10

[Jamie Nicol [:jnicol]]

Comment on attachment 8803360 [details]
Bug 1311642 - Remove BeginUpdate and EndUpdate from GLTextureImage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87662/diff/2-3/

Comment 11

[Jamie Nicol [:jnicol]]

Comment on attachment 8803361 [details]
Bug 1311642 - Give GLUploadHelpers some love.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87664/diff/2-3/

Comment 12

[Jamie Nicol [:jnicol]]

Comment on attachment 8803362 [details]
Bug 1311642 - Do partial texture uploads on mobile.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87666/diff/2-3/

Comment 13

[Jamie Nicol [:jnicol]]

First updated patch series removes TextureImageCGL since it is no longer used and was causing a build error on mac. And second removes a parameter from the call sites of UploadSurfaceToTexture which I'd forgotten about when removing it from the function signature. (Which was therefore being used instead of the default value for the texture unit and causing opengl errors.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef809648f83707e782bfda3ee350cd4398192c19

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0b288101472
Remove BeginUpdate and EndUpdate from GLTextureImage. r=nical
https://hg.mozilla.org/integration/autoland/rev/4a4b3074ade6
Give GLUploadHelpers some love. r=nical
https://hg.mozilla.org/integration/autoland/rev/262687aa0bf2
Do partial texture uploads on mobile. r=nical

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b0b288101472
https://hg.mozilla.org/mozilla-central/rev/4a4b3074ade6
https://hg.mozilla.org/mozilla-central/rev/262687aa0bf2