Closed
Bug 1101552
Opened 10 years ago
Closed 10 years ago
Remove double-buffered ImageClient
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(1 file, 1 obsolete file)
21.76 KB,
patch
|
sotaro
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
ImageClient has a double-buffered version because we used to alternate between two TextureClients when the input Image wasn't already allocated into a TextureClient. Since then, almost all of the use cases where we used to have to copy into the TextureClient are gone as more ImageTypes are backed by TextureClients. In the common case we are now double-buffering for nothing (and uselessly keeping an TextureClient alive). Removing the doubleBuffered ImageClient would simplify things. This also means that the code path where we used to copy non-shareable data into a TextureClient needs to be modified so that we don't re-use the TextureClient and instead create a new one each time, which is okay because it doesn't happen anymore. Sotaro, am I missing something? b2g is the main consumer of the double-buffered client (with windows D3D11).
Comment 1•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #0) > > Sotaro, am I missing something? b2g is the main consumer of the > double-buffered client (with windows D3D11). In b2g, ClientImageLayer::GetImageClientType() requests double buffered client. It does not work as intended at first. I was added from my misunderstanding. Therefore, it can be removed.
Assignee | ||
Comment 2•10 years ago
|
||
The only change of behavior in the single-buffered case is that now almost all error paths lead to the removal of the previous front buffer (via AutoRemoveTexture) whereas before some error cases would remove it and others not, but I didn't quite figure out what the criterion was. So it's a part you may want to carefully review if there was a logic behind not always removing the texture. I reorganized the logic a bit so that it goes something like: - try to get a TextureClient out of the Image - if no texture, then allocate one and coopy the image into it - do the ipc stuff (AddTextureClient, etc) instead of doing the last part in each branch, which removes a bit more code. The patch removes between 150 and 200 lines of code.
Attachment #8525393 -
Flags: review?(sotaro.ikeda.g)
Comment 3•10 years ago
|
||
Comment on attachment 8525393 [details] [diff] [review] Remove the double buffering and tiddy up a bit (patch to apply on aurora). Review of attachment 8525393 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems based on old source. It seems not have a change of Bug 1062475. http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp#194
Updated•10 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 4•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #3) > Comment on attachment 8525393 [details] [diff] [review] > Remove the double buffering and tiddy up a bit. > > Review of attachment 8525393 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch seems based on old source. It seems not have a change of Bug > 1062475. > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient. > cpp#194 nical, can you confirm the above?
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #3) > The patch seems based on old source. It seems not have a change of Bug > 1062475. > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient. > cpp#194 Yeah, I implemented it on top of aurora to see if it fixed bug 1098895, and hoped stuff wouldn't have changed too much. I'll rebase it today.
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8525393 -
Attachment description: Remove the double buffering and tiddy up a bit. → Remove the double buffering and tiddy up a bit (patch to apply on aurora).
Assignee | ||
Comment 6•10 years ago
|
||
This patch takes into account the change you pointed out (thanks for that, by the way, it would have been easy to overlook it during the rebase). I want to uplift this to aurora, do you think that the change from Bug 1062475 should get uplifted with it in the process?
Attachment #8525393 -
Attachment is obsolete: true
Attachment #8525393 -
Flags: review?(sotaro.ikeda.g)
Attachment #8526037 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 7•10 years ago
|
||
I tested the patch on hamachi since it seems to be the pickiest configuration and I haven't run into any issue using the camera and browsing around.
Assignee | ||
Comment 8•10 years ago
|
||
try push https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d1983b58d2a4
Comment 9•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #6) > Created attachment 8526037 [details] [diff] [review] > Patch rebased on top of central > > This patch takes into account the change you pointed out (thanks for that, > by the way, it would have been easy to overlook it during the rebase). I > want to uplift this to aurora, do you think that the change from Bug 1062475 > should get uplifted with it in the process? I do not have a strong opinion about it. It is very small change and very small risk. It can be uplifted in the process.
Comment 10•10 years ago
|
||
Comment on attachment 8526037 [details] [diff] [review] Patch rebased on top of central Review of attachment 8526037 [details] [diff] [review]: ----------------------------------------------------------------- Looks good :-)
Attachment #8526037 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e7c5f093ea
https://hg.mozilla.org/mozilla-central/rev/57e7c5f093ea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8526037 [details] [diff] [review] Patch rebased on top of central Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: Bug 1098895 [Describe test coverage new/current, TBPL]: [Risks and why]: Low, simplifies code, can be backed out if needed [String/UUID change made/needed]:
Attachment #8526037 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8526037 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•9 years ago
|
||
Needs rebasing for Aurora uplift.
status-b2g-v2.2:
--- → fixed
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
Flags: needinfo?(nical.bugzilla)
Keywords: branch-patch-needed
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #14) > Needs rebasing for Aurora uplift. Yeah, I have it locally, waiting for the tree to reopen
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 16•9 years ago
|
||
aaaand there we go https://hg.mozilla.org/releases/mozilla-aurora/rev/8ff014d18f0f
Updated•9 years ago
|
Keywords: branch-patch-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•