Closed Bug 1101552 Opened 10 years ago Closed 10 years ago

Remove double-buffered ImageClient

Categories

(Core :: Graphics: Layers, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
(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.
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 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
Flags: needinfo?(nical.bugzilla)
(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?
(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)
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).
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)
Blocks: 1098895
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.
(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 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+
https://hg.mozilla.org/mozilla-central/rev/57e7c5f093ea
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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?
Attachment #8526037 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(nical.bugzilla)
(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)
You need to log in before you can comment on or make changes to this bug.