Delete sound in CActiveAE::DiscardSound

[ruuk]

In pull request #4491, I added the useCached arg to xbmc.playSFX() which when set to false would cause the cached sound to be freed and the same filename loaded again. One assumption that I made was that CAEFactory::FreeSound() actually freed the sound. On further testing this has turned out not to be the case, and using playSFX() in this manner exposes a memory leak. I looked closer and found that FreeSound() ultimately causes a call to CActiveAE::DiscardSound() and while the sound is removed from a map of sounds I couldn't find anywhere where the sound was deleted. This adds that.

While this seems to me like it should be there regardless and greatly reduces the memory leak (by about a factor of 8) there still seems to be a leak. I can't seem to figure out why.

Below is a link to a small testing addon which demonstrates the leak.

http://2ndmind.com/xbmc/test/script.ruuk.testing-0.0.8.zip

[jmarshallnz]

Seems it doesn't get deleted anywhere else.

@FernetMenta @fritsch mind taking a look?

[FernetMenta]

Yes, this looks good. Thanks!

[jmarshallnz]

jenkins build this please

[fritsch]

For now it's only used from the statemachine and read from a message, but perhaps setting it to null after delete also would take care of future errors?

[FernetMenta]

too early for you? :)
sound is an argument of DiscardSound(), m_sounds is a vector. what exactly you want to set NULL?

[fritsch]

Not too early, it is good practice that one nulls a local ptr whenever one removes the corresponding element (where it points to) from a datastructure (m_sounds). Whoever calls DiscardSound or better said erase should care for that.

Edit: http://stackoverflow.com/questions/704466/why-doesnt-delete-set-the-pointer-to-null <- seems the maker of c++ is the same oppinion on this early morning.

[FernetMenta]

there is a return statement directly following the delete. I don't get your point, please post a diff

[whaupt]

I guess this code needs to be fast - is this why you use plain pointers in those std::vectors?

[FernetMenta]

I guess this code needs to be fast - is this why you use plain pointers in those std::vectors?

what would you suggest?

[fritsch]

I already said, that it does not matter in current code! But as Discard also does a delete everyone that calls DiscardSound will end with a dangling ptr.

Consider the following:

          sound = *(CActiveAESound**)msg->data;
          DiscardSound(sound);
          CLog::Log("Sound has volume: %d", sound->GetVolume());
          sound->SetVolume(0.5f);

Yeah I know, you don't want to consider things that not happen, but interface functions should be aware of dumb callers. The above scenario would corrupt the heap and most likely found after a whole lot of debugging, btw. DiscardSound is not documented to do a real delete of data. Consider reusing of that buffer and so on.

Edit: The other functions have at least a "Free" in the name.

[fritsch]

And: as the original message was FREESOUND the corresponding FreeSound function should be used.

[whaupt]

what would you suggest?

I was just curious if there is a reason not to use smart pointers in those std::vectors.
But I guess fritsch just gave the answer when considering buffer reuse etc.

[FernetMenta]

but interface functions should be aware of dumb callers.

DiscardSound is NOT an interface function. It is a protected helper method only used by the state machine.

I was just curious if there is a reason not to use smart pointers in those std::vectors

C++ 10 has no smart pointers and I consider boost a no go.

[whaupt]

C++ 10 has no smart pointers and I consider boost a no go.

Reason enough :)

[FernetMenta]

my last comment here @fritsch

And: as the original message was FREESOUND the corresponding FreeSound function should be used.

You may have noticed that FreeSound IS the public interface function. I needed to find a new name for it. Some others of those a prefixed with an S for state machine. Feel free to fix this inconsistency.

[fritsch]

That's fine. Will send a PR after that one is in.

[ruuk]

Should I set it to null after the delete, or did we decide to leave it as is?

[FernetMenta]

it's fine as is. it has a Gotham label so we wait for RMs to push the buton.

[ruuk]

Ok, thanks.

[jmarshallnz]

At your service :)