ofVideoPlayer room for improvement

[dimitre]

PRs

• ofVideoPlayer AVFoundation, setPosition can work with arbitrary frames. #8189
• ofVideoPlayer setPlayer std::move shared_ptr #6821
• ofBaseHasPixels is already present #6823
• ofAVFoundationPlayer - redundant code removal #6824
• ofVideoPlayer unneeded inheritance #6829
• video identation : spaces to tabs #6838

Issue to address

• ofVideoPlayer setUseTexture(false) not working in MacOS #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
	}
}

[ofTheo]

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 :)

[dimitre]

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]

EDIT, merged

• ofVideoPlayer unneeded inheritance #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.

[ofTheo]

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. :)

[dimitre]

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]

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]

Edit: merged

• ofVideoPlayer setPlayer std::move shared_ptr #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
#6821

[dimitre]

Edit, A little bit simpler, after this merged PR #6823

Another simplification here #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:
#5797

[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();
	}