Sascha-L / WPF-MediaKit

Microsoft Public License
363 stars 115 forks source link

changes to DirectShowUtil.RemoveFilters logic #36

Closed ecoChahn closed 7 years ago

ecoChahn commented 7 years ago

Corrected logic to prevent COM Exceptions on media close or new source set

Fixes issue #35

xmedeko commented 7 years ago

Formal code review:

IMO this freeing resources needs to polish, but to your solution:

ecoChahn commented 7 years ago

Hello, xmedeko, thank you for taking the time to review. I suppose those comments in DirectShowUtil.cs could just be removed from the code.

Per your question. The object in info.pGraph does get released - it should be the same object as m_graph in MediaUriPlayer.cs:616, which is released right after RemoveFilters. This was already being called there, which I believe is the correct place to do so. The original problem was that with the release calls in the logic for each filter, after cleaning up one filter, the graph was released already, and cleaning up all the subsequent filters was failing.

I agree, InvokeMediaClosed should be moved to the bottom. Per the comments, it should also only be called if there was an initialized graph.

Would you like me to make these changes?

xmedeko commented 7 years ago

Thanks for the clarification, ecoChahn. Yep, pls, do the changes. You can fix the commit message by git amend.

ecoChahn commented 7 years ago

After further review, I am putting my changes to DirectShowUtil on hold, although I do believe more work could be done there. (for instance, do we need to QueryFilterInfo when we do not use the info?)

I have amended my commit to include only the requested changes in MediaUriPlayer.FreeResources. Please review.