Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix memory leaks#282

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

cchampet
Copy link
Member

@cchampetcchampet commentedOct 27, 2016
edited
Loading

The most important parts of the code which lead to memory leaks were:

  • allocation of data buffers when decoding (which is the job of the decoder).
  • reference of data buffers of other frames (which can produce copy of data depending on the state of the AVFrame).
  • assignment of external data buffer in the generators.

Clement Champetier added16 commitsOctober 26, 2016 11:19
Will be used when calling refFrame.
To keep symmetry with allocateAVFrame private method.
* Same behavior as before.* But this call is explicitly mention in the documentation.
* The way to reference other frame in ffmpeg/libav is tricky. Forexample, if src is not reference counted, new buffers are allocated andthe data is copied. And this case leads us to memory leaks in ourprogram!* See the code:https://www.ffmpeg.org/doxygen/2.6/frame_8c_source.html#l00278* Because we do not reference a lot of frames in our code, to avoid anoverhead of complexity this commit removes these methods.* Instead, we copy the data.
* According to the doc, for each new stream added, the user should callavcodec_close and avformat_free_context.* See the doc:https://www.ffmpeg.org/doxygen/3.0/group__lavf__core.html#gadcb0fd3e507d9b58fe78f61f8ad39827
* This is a common case, without filters added.* This avoid a copy of the data of each encoded frame.
Because we are using ffmpeg/libav functions, we should use ffmpeg/libavstructures too.
* A data buffer is not allocated by default in the constructor.* Frame is now an abstract class: easier to call allocateData andfreeData methods.* Depending on the case, we could manipulate frame with data allocatedelsewhere. For example, an empty frame is given to a decoder, which isresponsible to allocate and free the data buffers.
* Remove protected method inherited from ITransform.* Add the same series of test for the video and the audio transform.
In that case, the data buffer of the generated data should be allocatedmanually.
* Solution: keep the original description when the frame was created,and use it to reallocate it when needed.* No need to check and reallocate data in the generators.
Because the common case is to allocate the data at the same time as theframe itself, this commit adds a default parameter to the constructor ofVideo/Audio frames.
This info can be found from elsewhere, and the method is not so used.
Clement Champetier added12 commitsOctober 28, 2016 10:36
* IFrame is an abstract class.* To keep the naming convention of the project, rename it to IFrame.
* Less code to maintain.* Rename assign methods to assignBuffer and assignValue (to avoid implicite C++ cast and so calling the wrong method).
* The data buffer of the frame should be freed before allocate a newbuffer.* The new buffer should be allocated using malloc instead of free, sinceffmpeg/libav uses free to deallocate the data. The valgrind error was:"Mismatched free() / delete / delete []".
In a demultiplexing case, the buffer should be allocated by us, but onlyonce (not in the decodeNextFrame method)!
The number of channels and the channel layout is already correctly setby the StreamTranscoder.
In this case, the buffer of decoded data should be manually allocated,and freed when it is the decoder's turn.
Clement Champetier added4 commitsNovember 2, 2016 09:51
* If the buffer of filtered data is too small: reallocate it.* If the buffer of transformed data is too small: reallocate it.
…sizeNo real impact in the process, because we only check that "decodedSize"is not equal to 0. But it is more accurate :)
@coveralls
Copy link

coveralls commentedNov 2, 2016
edited
Loading

Coverage Status

Coverage increased (+1.1%) to 78.019% when pullinga66c49d on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.019% when pullinga66c49d on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

Clement Champetier added2 commitsNovember 2, 2016 11:04
…iplexingIn case of audio demultiplexing, we allocate the data buffer, so weshould not reallocate it when we switch to a generator.
@coveralls
Copy link

coveralls commentedNov 2, 2016
edited
Loading

Coverage Status

Coverage increased (+1.1%) to 78.019% when pullingb6dc069 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.019% when pullingb6dc069 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.019% when pullingb6dc069 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

coveralls commentedNov 2, 2016
edited
Loading

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling93ce328 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling93ce328 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling93ce328 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@@ -23,7 +23,7 @@ class AvExport Frame
Frame();

//@{
// @briefCopy properties and referencedataof theother frame.
// @briefAllocate a new frame that references the samedataas thegiven frame.
Copy link
Member

Choose a reason for hiding this comment

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

Allocate a new frame thatcopies the same data as the given frame.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This copy constructor has been removed.

@@ -76,9 +79,6 @@ size_t VideoFrame::getSize() const

void VideoFrame::allocateData()
{
if(_dataAllocated)
return;
Copy link
Member

Choose a reason for hiding this comment

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

Removing this, you allow an allocated frame to be reallocated, is that really what you want?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes. But we can check if this is needed or not... If it is so, it should be ugly!

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Log a warning is that case:cd2db7b

os << "sample format = " << getSampleFormatName(_desc._sampleFormat);
LOG_ERROR(os.str())
const std::string formatName = getSampleFormatName(_desc._sampleFormat);
std::stringstream stream;
Copy link
Member

Choose a reason for hiding this comment

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

stream could be a bit confusing in this transcoder context... ;)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@@ -49,6 +49,7 @@ class AvExport AudioFrame : public IFrame
*/
void allocateData();
void freeData();
size_t getSize() const;
Copy link
Member

Choose a reason for hiding this comment

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

So, it could have been renamedgetDataSize()!

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

* @brief The number of samples of a frame cannot be known before calling avcodec_decode_audio4,
* and can be variable in a same stream. Because we need to allocate some frames without knowing this parameter,
* we access here a default number of samples.
* @note This value depends on the sample rate (excample: 1920 samples at 48kHz).
Copy link
Member

Choose a reason for hiding this comment

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

example

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commentedNov 3, 2016
edited
Loading

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling1cea044 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling1cea044 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling1cea044 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 78.025% when pulling1cea044 on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@coveralls
Copy link

coveralls commentedNov 3, 2016
edited
Loading

Coverage Status

Coverage increased (+1.07%) to 77.993% when pullingcd2db7b on cchampet:fix_memoryLeakWhenGenerateData into103c33f on avTranscoder:develop.

@valnoelvalnoel merged commit6b535b3 intoavTranscoder:developNov 3, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@valnoelvalnoelvalnoel left review comments

Assignees

@valnoelvalnoel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@cchampet@coveralls@valnoel

[8]ページ先頭

©2009-2025 Movatter.jp