openFrameworks icon indicating copy to clipboard operation
openFrameworks copied to clipboard
verified publisher icon openframeworks

ofVideoPlayer room for improvement

Open dimitre opened this issue 4 years ago • 11 comments

PRs

  • [ ] https://github.com/openframeworks/openFrameworks/pull/8189
  • [x] https://github.com/openframeworks/openFrameworks/pull/6821
  • [x] https://github.com/openframeworks/openFrameworks/pull/6823
  • [x] https://github.com/openframeworks/openFrameworks/pull/6824
  • [x] https://github.com/openframeworks/openFrameworks/pull/6829
  • [x] https://github.com/openframeworks/openFrameworks/pull/6838

Issue to address

  • [ ] https://github.com/openframeworks/openFrameworks/issues/5797

I'm spending some time reading ofVideoPlayer code to understand better how it is structured. So I'm opening here to ask opinions and share thoughts.

Some things caught my attention in the process, the first of them is this function

void ofVideoPlayer::setPlayer(shared_ptr<ofBaseVideoPlayer> newPlayer)

which has a shared_ptr as a parameter. and assigns a shared_ptr to another shared_ptr here

player = newPlayer;

I can see they are using two different addresses if I print them

cout << "setPlayer " << &player << " " << newPlayer << endl;

I've noticed there are some checks in different parts of the code which call this function, some call setPixelFormat, and some don't.

	if( !player ){
		setPlayer(std::make_shared<OF_VID_PLAYER_TYPE>());
		player->setPixelFormat(internalPixelFormat);
	}

We can substitute this checks with a function which creates the shared pointer without passing as a function parameter, something like this


void ofVideoPlayer::checkPlayer(){
	if( !player ){
		player = std::make_shared<OF_VID_PLAYER_TYPE>();
		player->setPixelFormat(internalPixelFormat);
		setPixelFormat(internalPixelFormat);	//this means that it will try to set the pixel format you
	}
}

dimitre avatar Dec 03 '21 19:12 dimitre

I would do this check:

cout << "setPlayer " << player.get() << " new player is " << newPlayer.get() << endl;

For me I get the same: setPlayer 0x10162a778 new player is 0x10162a778

NOTE: The way you had it was you were checking the address of the shared_ptr object not the raw pointer it is holding. the .get() function of a shared_ptr gives you the actual raw pointer address.

Hope that clears it up! Would be a pretty bad bug if it was behaving this way :)

ofTheo avatar Dec 03 '21 20:12 ofTheo

Yes I can understand that. there is more haha. I don't think player->setPixelFormat(internalPixelFormat); is needed after setPlayer(std::make_shared<OF_VID_PLAYER_TYPE>());

because internally setPlayer calls setPixelFormat which by itself calls player->setPixelFormat

dimitre avatar Dec 03 '21 20:12 dimitre

EDIT, merged

  • https://github.com/openframeworks/openFrameworks/pull/6829

and this line

class ofVideoPlayer : public ofBaseVideoPlayer,public ofBaseVideoDraws{

can be changed to

class ofVideoPlayer : public ofBaseVideoDraws{

as it is, because of double inheritance.

It is a simplification, so there is less code to read when changes are needed.

dimitre avatar Dec 03 '21 20:12 dimitre

Ahh, yeah, I think the extra setPixelFormat calls could be removed wherever setPlayer is called.

I would be more hesitant of messing with the base class stuff though. It is currently working and who knows what changing it might break. :)

ofTheo avatar Dec 03 '21 21:12 ofTheo

Room for improvement in ofAVFoundationVideoPlayer: seekToTime calls createAssetReaderWithTimeRange and each call it parses the information about the correct video and audio track, starting in the line

		NSMutableDictionary * videoOutputSettings = [[[NSMutableDictionary alloc] init] autorelease];

This could be moved to inside loadWithURL, because it wont' change until a new video is loaded / unloaded

dimitre avatar Dec 05 '21 15:12 dimitre

AVFoundationVideoPlayer seekToTime (that is called by setPosition) creates a new asset reader with the function

		[self createAssetReaderWithTimeRange:CMTimeRangeMake(time, duration)];

starting with the current time to the end of the video. The problem is not every frame has a keyframe in the frame you want to read. most likely not. Most common video encoding settings use one keyframe per second, so maybe the time range could be "time - 1 second" and clamping to be > 0

dimitre avatar Dec 06 '21 19:12 dimitre

Edit: merged

  • https://github.com/openframeworks/openFrameworks/pull/6821

Hey @ofTheo still talking about the shared_ptr as a function parameter, I still think it is an issue but there is a minimal solution to that. Changing the line

player = newPlayer;

to

player = std::move(newPlayer);

this way we don't need to increment the shared_ptr usage to 2 and then decrease it again because of the existence of two shared_ptr. We can check it this way:

	cout << "setPlayer " << ofGetFrameNum() << endl;
	cout << "player use count " << player.use_count() << endl;
	cout << "new player use count " << newPlayer.use_count() << endl;

reference: https://stackoverflow.com/questions/29643974/using-stdmove-with-stdshared-ptr

Edit: I've actually submitted a PR here https://github.com/openframeworks/openFrameworks/pull/6821

dimitre avatar Jan 06 '22 13:01 dimitre

Edit, A little bit simpler, after this merged PR https://github.com/openframeworks/openFrameworks/pull/6823

Another simplification here https://github.com/openframeworks/openFrameworks/pull/6829

Video Player

ofVideoPlayer < ofBaseVideoPlayer, ofBaseVideoDraws
	ofBaseVideoPlayer < ofBaseVideo
	ofBaseVideoDraws < (ofBaseVideo, ofBaseDraws, ofBaseHasTexturePlanes)

ofAVFoundationPlayer < ofBaseVideoPlayer < ofBaseVideo  < (ofBaseHasPixels, ofBaseUpdates)

Grabber

ofVideoGrabber < ofBaseVideoGrabber, ofBaseVideoDraws
ofAVFoundationGrabber < ofBaseVideoGrabber < ofBaseVideo < (ofBaseHasPixels < ofAbstractHasPixels, ofBaseUpdates)

I still think there is something strange about ofVideoPlayer and we can see different threads about memory leak. Maybe understanding better the underlying ofVideoPlayer structure can help us understanding what can be improved.

I've set up a diagram to understand the memory layout of the ofVideoPlayer object

ofVideoGrabber < ofBaseVideoGrabber, ofBaseVideoDraws
ofAVFoundationGrabber < ofBaseVideoGrabber < ofBaseVideo < (ofBaseHasPixels < ofAbstractHasPixels, ofBaseUpdates)
	
ofVideoPlayer < ofBaseVideoPlayer, ofBaseVideoDraws
	ofBaseVideoPlayer < ofBaseVideo < (ofBaseHasPixels < ofAbstractHasPixels, ofBaseUpdates)
	ofBaseVideoDraws < (ofBaseVideo, ofBaseDraws, ofBaseHasTexturePlanes, ofBaseHasPixels)
ofAVFoundationPlayer < ofBaseVideoPlayer

which can help addressing issues like this one: https://github.com/openframeworks/openFrameworks/issues/5797

dimitre avatar Jan 07 '22 13:01 dimitre

I've recently noticed when using AVFoundationVideoPlayer, if I call close it free "some" memory but not all. but if I rebuild the object using setPlayer it releases a lot more of memory. small example, when I call video close it keeps using 128Mb RAM. If I use setPlayer memory usage goes down to 73Mb. I think there are more things to release when using .close() with AVFoundation

	if (key == 'c') {
		video.close();
	}
	else if (key == 'q') {
		video.setPlayer(std::make_shared<OF_VID_PLAYER_TYPE>());
	}
	else if (key == 'w') {
		video.load(fileName); //"v2.mov"
		video.play();
	}

dimitre avatar Jan 08 '22 02:01 dimitre

This PR was merged https://github.com/openframeworks/openFrameworks/pull/6824 Now replace spaces to tabs, fix identation https://github.com/openframeworks/openFrameworks/pull/6838

dimitre avatar Jan 10 '22 23:01 dimitre

This function is called to actually draw the video every frame, lets see what happens in each function here. There are lots of things going on to create a mesh every frame, and the mesh itself will always be the same until loading another video or something similar. it seems it can be easily cached https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/gl/ofGLRenderer.cpp#L365-L371

getTexturePtr is being called twice here https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/video/ofVideoPlayer.cpp#L486-L491

what it calls https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/video/ofAVFoundationPlayer.mm#L386-L405

https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/video/ofVideoPlayer.cpp#L389-L391

https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/video/ofVideoPlayer.cpp#L144-L150

And finally the function inside ofTexture, that creates an ofMesh every frame and calculate the hacks to remove border artifacts https://github.com/openframeworks/openFrameworks/blob/e194964654d54a464212764c77a670fc9ee19eb3/libs/openFrameworks/gl/ofTexture.cpp#L1080-L1162

dimitre avatar Jan 26 '22 10:01 dimitre