Page MenuHome

Fix T43033: VideoTexture module repeated loading of images causes memory leak
ClosedPublic

Authored by Ulysse Martin (youle) on Jul 8 2015, 7:01 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Ulysse Martin (youle) retitled this revision from to Fix for https://developer.blender.org/T43033.
Ulysse Martin (youle) updated this object.
Ulysse Martin (youle) retitled this revision from Fix for https://developer.blender.org/T43033 to Fix T43033: VideoTexture module repeated loading of images causes memory leak.Jul 8 2015, 7:07 PM
Jorge Bernal (lordloki) added inline comments.
source/gameengine/VideoTexture/VideoFFmpeg.cpp
1025

instead of using a new member variable you can use a local variable into the if condition:

AVFrame *input = m_frame;
short counter = 0;

/* While the data is not read properly (png image may need several pass)*/
while ((input->data[0]==0 && input->data[1]==0 && input->data[2]==0 && input->data[3]==0) || counter < 10) {
    avcodec_decode_video2(m_codecCtx, m_frame, &frameFinished, &packet);
    counter++;
}
source/gameengine/VideoTexture/VideoFFmpeg.cpp
1052

I don't think that this part should be removed as in the case of the data is not read properly (i.e counter > 10) then we need some way to free it

Hi! Sorry to have opened a new revision but I was asked to do that on IRC because it's easier to review diffs with diff phabricators instead of pasteall.

@Jorge Bernal (lordloki): You're right! I update the .diff. (If I made mistake or forgot something (I'm a bit tired), don't hesitate to take the control of the revision to correct it if you want and commit it.) Good day :)

Ah! I think I forgot something>the second check to break the first while loop :s Sorry. I'm going to sleep...

So, after a good night...

@Jorge Bernal (lordloki): The problem if you say: while ((input->data[0]==0 && input->data[1]==0 && input->data[2]==0 && input->data[3]==0) || counter < 10) is that decoding pass number will always be 10 even for jpeg images (which needs only one pass). It seems that we need 5 decoding pass to decode png images but I don't know for other formats (tiff...). So I break the while loop after 100 pass and if the counter > 100, I free the packet, I end the VideoFFmpeg::grabFrame function and I leave a message in the console ("failed to decode image") for debugging purpose.

I hope I forgot nothing. Good night/day!

This night I will try to test it with some files

source/gameengine/VideoTexture/VideoFFmpeg.cpp
1035

Perhaps should be necessary to include:

m_eof = true;

before return NULL;

Hi! @Jorge Bernal (lordloki): As I said to Moguri on IRC, it seems that the code never enters the if statement. In the test file (Monkey in T43033), you can put whatever you want in the texture folder: A text file, a compressed file etc... So I think counter and if statement are not necessary finally (unless I forget something...). (I tested 3 formats: jpg takes 1 pass, png and tiff takes 5 pass to be decoded)

Has this been run through something like Valgrind's memcheck tool?

Hi. @Mitchell Stokes (moguri): No, what I did is to reduced texture switch frequency to 2 and leave the Windows task manager in background to see if memory usage increased. And it seems that the fix solved the problem. This solution came from this thread: https://trac.ffmpeg.org/ticket/4404 at the bottom: "Yes, thread_count has to be set to 1 or the user application must call the decoder in a loop until the frame has been decoded."

(*increase frequency)

Jorge Bernal (lordloki) edited edge metadata.EditedAug 27 2015, 10:26 PM

@Jorge Bernal (lordloki), what do you think of this patch?

Although the solution is the right one (i guess) I would only modify this part:

 		if (packet.stream_index == m_videoStream) 
 		{
-			if (m_isImage)
-			{
-				// If we're an image, we're probably not going to be here often,
-				// so we don't want to deal with delayed frames from threading.
-				// There might be a better way to handle this, but I'll leave that
-				// for people more knowledgeable with ffmpeg than myself. We don't
-				// need threading for a single image anyways.
-				m_codecCtx->thread_count = 1;
-			}
-			avcodec_decode_video2(m_codecCtx,
-				m_frame, &frameFinished,
-				&packet);
+			AVFrame *input = m_frame;
+			short counter = 0;

+			/* While the data is not read properly (png, tiffs, etc formats may need several pass)*/
+			while ((input->data[0]==0 && input->data[1]==0 && input->data[2]==0 && input->data[3]==0) && counter < 10) {
+			    avcodec_decode_video2(m_codecCtx, m_frame, &frameFinished, &packet);
+			    counter++;
+			}
 
 			// remember dts to compute exact frame number
 			dts = packet.dts;

I have test it with several formats. jpg, tga, bmp only need 1 pass. png, tiff 4 passes.

I would leave the av_free_packet where it is now to minimize the fix

Ulysse Martin (youle) edited edge metadata.

@Jorge Bernal (lordloki): Ah yes, you're right, sorry about my previous comment (I thought that the loop was executed 10 times with && counter < 10 :) )
Is it ok now?

Jorge Bernal (lordloki) edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Aug 28 2015, 1:25 AM
This revision was automatically updated to reflect the committed changes.

I found that fix T43033 broke VideoFFmpeg stop() function.

Working: 09e40a4956494655a2d544ab654372455882dbc3
Broken: e443dd4d9768394a61197dc187eb21c65aad7941

Put both files in the same folder.
Open blend file with broken version, start Game Engine (or run it with BlenderPlayer).

Press Spacebar to play the video (it uses play())
Press Enter to stop it (it uses stop())
Press Spacebar again: it doesn't play again.

With working version, it restarts the video.


@Mario Mey (mariomey): Hi, you made a git bisect? Just to be sure this is this commit that causes the problem.

@Ulysse Martin (youle) As I told you by chat, I don't know what is a "bisect", but I compiled this commit and the last before and I can say the the problem is after this fix.