Closed Bug 1311642 Opened 5 years ago Closed 5 years ago

Partial GL Texture Upload is a bit of a mess


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




Tracking Status
firefox52 --- fixed


(Reporter: jnicol, Assigned: jnicol)



(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.

Attachment #8803360 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8803361 [details]
Bug 1311642 - Give GLUploadHelpers some love.
Attachment #8803361 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8803362 [details]
Bug 1311642 - Do partial texture uploads on mobile.
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.)
Keywords: checkin-needed
Pushed by
Remove BeginUpdate and EndUpdate from GLTextureImage. r=nical
Give GLUploadHelpers some love. r=nical
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.