Closed Bug 904343 Opened 11 years ago Closed 11 years ago

We never clear our surface descriptors in shmem texture clients

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: nrc, Assigned: nrc)

References

Details

Attachments

(2 files)

ContentClientDoubleBuffered::~ContentClientDoubleBuffered() calls SetDescriptor(SurfaceDescriptor()) as the standard way to blank out a SurfaceDescriptor. Then in DeprecatedTextureClientShmem::SetDescriptor we check IsSurfaceDescriptorValid which will be no, because it is the empty Descriptor (T__None) and then we will not unset mDescriptor. Which seems bad. Am I missing something?
Attached patch patchSplinter Review
Attachment #789336 - Flags: review?(nical.bugzilla)
Attachment #789336 - Flags: feedback?(bjacob)
this version does the old thing for null surface descriptors and resets for None ones. Note that all the null checking on surfaces is not really related to the important thing here which is the destruction sequence for content clients via SetDescriptor.
Comment on attachment 789336 [details] [diff] [review] patch Review of attachment 789336 [details] [diff] [review]: ----------------------------------------------------------------- This looks sane to me. The thing is that DeprecatedTextureClient feels like it's on an awkward equilibrium because it doesn't have the same semantic on ImageLayers and ThebesLayers. So changing something for one may break the other. Please make sure you have tested this patch with a video (preferably with async-video) before landing.
Attachment #789336 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 789336 [details] [diff] [review] patch Review of attachment 789336 [details] [diff] [review]: ----------------------------------------------------------------- I don't know / remember much about the code around here, I can just say that nothing here conflicts with whatever little I know. As Nical says, the old TextureClient stuff is very hard to reason about, so make sure you've tested both ContentClients (some web browsing) and ImageClients (use the camera app).
Attachment #789336 - Flags: feedback?(bjacob) → feedback+
Confirmed: attachment 789336 [details] [diff] [review] fixes the lockscreen crash on Inari.
Blocks: 904658
Tested on Android and b2g (and Windows) - images, video, and camera. Hope I covered all the bases. https://hg.mozilla.org/integration/mozilla-inbound/rev/2b003d033fa4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: