jimfcarroll/xbmc — comment on commit fa29541976001a729f84b4713b34b14e36295582
File:  line 0
By jimfcarroll:

First, I'm wondering why you're using a SharedLock for the DisplaySection. You're locking it for "read" in a few places but just checking the value of a single variable. If you look at the implementation of the CSharedSection::lock_shared you'll see it does that. IOW, you're use of the SharedLock is lower performing than the straight use of a CriticalSection would be.

Second, I don't see any reason to nest the locking. In each case where you're nesting the DisplaySection lock inside the DecoderSection lock it looks like you could just do the DisplaySection first, followed by the DecoderSection (of course, I could be missing something). Given that the m_DisplayState gated only by the DisplaySection and that this is mutually exclusive with the gating of access that DecoderSection does, I don't think you need to nest anything.

Also, I think you should make Fini* something other than public since you appear to require gating via DecoderSection so it can't be called from outside of the class without creating problems anyway.

Does that remove the need for a reentrant SharedSection?