Closed Bug 1311642 Opened 3 years ago Closed 3 years ago

Partial GL Texture Upload is a bit of a mess

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

Details

Attachments

(3 files)

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 on attachment 8803360 [details]
Bug 1311642 - Remove BeginUpdate and EndUpdate from GLTextureImage.

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

Neat!
Attachment #8803360 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8803361 [details]
Bug 1311642 - Give GLUploadHelpers some love.

https://reviewboard.mozilla.org/r/87664/#review86632
Attachment #8803361 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8803362 [details]
Bug 1311642 - Do partial texture uploads on mobile.

https://reviewboard.mozilla.org/r/87666/#review86634
Attachment #8803362 - Flags: review?(nical.bugzilla) → review+
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
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.