QtMir

Merge lp://qastaging/~dandrader/qtmir/textureCleanUp-lp1499388 into lp://qastaging/qtmir

textureCleanUp-lp1499388
Merge into trunk

Proposed by Daniel d'Andrada on 2015-09-25

Status:

Merged

Approved by:

Gerry Boland on 2015-09-29

Approved revision:

382

Merged at revision:

382

Proposed branch:

lp://qastaging/~dandrader/qtmir/textureCleanUp-lp1499388

Merge into:

lp://qastaging/qtmir

Diff against target:

96 lines (+20/-5)

4 files modified

src/modules/Unity/Application/mirsurface.h (+1/-0)
src/modules/Unity/Application/mirsurfaceinterface.h (+2/-0)
src/modules/Unity/Application/mirsurfaceitem.cpp (+16/-5)
tests/modules/common/fake_mirsurface.h (+1/-0)

To merge this branch:

bzr merge lp://qastaging/~dandrader/qtmir/textureCleanUp-lp1499388

Related bugs:

Bug #1495871: unity8 leaks file descriptors cause unresponsive ui but power button works display	High	Fix Released
Bug #1499388: unity8 sometimes crashing after purchasing app	Critical	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gerry Boland (community)		2015-09-25	Approve on 2015-09-29
PS Jenkins bot (community)	continuous-integration		Approve on 2015-09-28
Review via email: mp+272452@code.qastaging.launchpad.net

Commit message

MirSurfaceItem: texture must be manipulated only from the scene graph thread

Do not delete the texture from within the GUI thread ever, even if the
MirSurfaceItem is no longer holding the MirSurface that provided it.
Otherwise you run the risk of having the scene graph thread dereferencing
a pointer to a texture that no longer exists.

This also fixes fd leak LP: #1495871 (which was a texture/buffer leak).

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
No.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/441/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2015-09-28:

It looks strange to me that the MirSurface API needs methods to both get a shared pointer to the texture, and a "weak" pointer. Your solution indicates there are times MirSurfaceItem need to take shared ownership of a texture, and there are times MirSurfaceItem needs to peek to see what the current texture actually is.

But instead of peeking for every frame to check if the MirSurface backing texture matches the one the textureProvider is holding though, wouldn't it just be easier to set the texture each and every frame? Something like:

     if (!m_textureProvider) {
         m_textureProvider = new MirTextureProvider(m_surface->texture());
- } else if (!m_textureProvider->texture()) {
- m_textureProvider->setTexture(m_surface->texture());
     }
+ m_textureProvider->setTexture(m_surface->texture());
}

(P.S. I've noticed that QSGTextureProvider has a textureChanged signal we're not emitting, might that have an impact at all?)

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2015-09-28:

On 28/09/2015 09:21, Gerry Boland wrote:
> (P.S. I've noticed that QSGTextureProvider has a textureChanged signal we're not emitting, might that have an impact at all?)

Checked Qt code a while ago regarding this. If I recall correctly, it
wan't being used for anything and most/some providers didn't even emit it.

lp://qastaging/~dandrader/qtmir/textureCleanUp-lp1499388 updated on 2015-09-28

381. By Daniel d'Andrada on 2015-09-28: Refactor code

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2015-09-28:

On 28/09/2015 09:21, Gerry Boland wrote:
> But instead of peeking for every frame to check if the MirSurface backing texture matches the one the textureProvider is holding though, wouldn't it just be easier to set the texture each and every frame?
It would make for less code and a smaller diff, but I think it would
also be less performant as you would be creating and destroying a shared
pointer all the time. Ie, apart from the first and last frames, you
would be needlessly creating a strong ref out of a weak ref just to
throw it away later on. Maybe this cost is negligible, but if we can
easily avoid it, why not?

I compressed this if{}else{} chain. It should look less cluttered now.

Revision history for this message

Gerry Boland (gerboland) wrote on 2015-09-28:

I've no idea of cost of shared pointer, but it can't be that much. Usually I'd prefer the simpler approach, but since you maintain almost exactly the same code-path we've used for over a year, this approach is less risky.

I'll accept, but will keep it in mind for re-addressing at a later date.

Note, you removed a useful comment with your last commit, could you re-instate it please.

lp://qastaging/~dandrader/qtmir/textureCleanUp-lp1499388 updated on 2015-09-28