Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverity fix: pointer/resource leak #223

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

ZitaLiao
Copy link
Contributor

No description provided.

@ZitaLiao ZitaLiao marked this pull request as ready for review October 13, 2023 04:30
@ZitaLiao ZitaLiao changed the title [W.I.P.] Coverity fix: catch MP4V2 exception so pointer/resource would not leak Coverity fix: catch MP4V2 exception so pointer/resource would not leak Oct 13, 2023
@ZitaLiao ZitaLiao changed the title Coverity fix: catch MP4V2 exception so pointer/resource would not leak [W.I.P.] Coverity fix: catch MP4V2 exception so pointer/resource would not leak Oct 13, 2023
@harryz2000 harryz2000 changed the title [W.I.P.] Coverity fix: catch MP4V2 exception so pointer/resource would not leak Coverity fix: catch MP4V2 exception so pointer/resource would not leak Oct 16, 2023
@murillo128
Copy link
Member

@harryz2000 could we try to use unique_ptr to manage the leaked resources instead?

@harryz2000
Copy link
Contributor

Yes. That would be a good idea. Will make the change.

@harryz2000 harryz2000 changed the title Coverity fix: catch MP4V2 exception so pointer/resource would not leak Coverity fix: pointer/resource leak Oct 19, 2023
@@ -996,7 +996,7 @@ AVCDescriptor* MP4Streamer::GetAVCDescriptor()
if (pictureHeaderSize)
free(pictureHeaderSize);

return desc;
return desc.release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make MP4Streamer::GetAVCDescriptor() return a std::unique_ptr and chante the rtmpmp4streamer to use a unique ptr too

//Add it to map
videoTrack = videoTrackPtr.release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should modernize the whole pointer management of the tracks. So let's change the videoTracks to be a vector of shared_ptrs and use shared_ptrs instead of unique_ptrs for videoTrack so the code here is easier to change

Copy link
Member

@murillo128 murillo128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soory for changin my mind, but let's try to modernize the whole file

@@ -130,7 +130,7 @@ void RTMPMP4Stream::doPlay(QWORD transId, std::wstring& url,RTMPMediaStream::Lis
SendMetaData(meta);

//Get AVC descriptor if any
desc = streamer.GetAVCDescriptor();
desc = streamer.GetAVCDescriptor().release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make desc in in RTMPMP4Stream a unique ptr too?

Copy link
Contributor

@harryz2000 harryz2000 Oct 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this rtmpmp4stream.cpp wasn't used in any build files. Therefore, I tended to do minor changes in case the change would break anything. So I didn't optimize this file. Where was this file used? Or should we optimize it anyway? If so, we might want to add it to some cmake file to at least verify it can compile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's ignore it for now.

Copy link
Member

@murillo128 murillo128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@murillo128 murillo128 merged commit aca9fc4 into master Oct 23, 2023
1 check passed
@murillo128 murillo128 deleted the dev/zliao/coverity_mp4v2_exception branch March 21, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants