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

ofVideoPlayer unneeded inheritance #6829

Merged
merged 7 commits into from
Sep 30, 2022
Merged

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented Jan 10, 2022

It was only needed if ofVideoPlayer was the object of playing video itself,
but I understand now ofVideoPlayer will always have a player object inside with playing capabilities (player)
Player itself is called for ofBaseVideoPlayer properties, so it makes no difference.

Related #6793 (comment)

It was only needed if ofVideoPlayer was the object of playing video itself, 
but I understand now ofVideoPlayer will always have a player object inside with playing capabilities (player)
@dimitre
Copy link
Member Author

dimitre commented Sep 28, 2022

Let's merge this one @ofTheo

@ofTheo
Copy link
Member

ofTheo commented Sep 28, 2022

hmm - I am still nervous about this one.

I could see someone having something like:

//someones custom class 
void setPlayer(ofBaseVideoPlayer * player){
    playerPtr = player; 
    //some custom code here 
};

That is designed to allow any class with a ofBaseVideoPlayer.

eg:

ofVideoPlayer player; 
ofFFMpegPlayer ffmpegPlayer; 

myClass.setPlayer( &player ); //use default 

myClass.setPlayer( &ffmpegPlayer); //use ffmpeg 

I feel like we have done this in the past for a project.
And we also have this approach here too:
https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/video/ofVideoGrabber.h#L49

So I think it is there to allow maximum flexibility.

@dimitre
Copy link
Member Author

dimitre commented Sep 28, 2022

ofVideoPlayer object is never a player (maybe only in remote past) it is only a container.
it always hold the player object inside itself that can always be set as in your examples
you can see this here

void ofVideoPlayer::setPlayer(shared_ptr<ofBaseVideoPlayer> newPlayer){
	player = std::move(newPlayer);
	setPixelFormat(internalPixelFormat);	//this means that it will try to set the pixel format you have been using before. 
											//if the format is not supported ofVideoPlayer's internalPixelFormat will be updated to that of the player's
}

and the player object

	std::shared_ptr<ofBaseVideoPlayer>		player;

@ofTheo
Copy link
Member

ofTheo commented Sep 28, 2022

@dimitre oh yeah totally, I understand that.

It is more that the way it currently is you can store a pointer to ofBaseVideoPlayer and it can refer to both ofVideoPlayer and ofDSVideoPlayer - which can be useful in some situations ( as you can use implementation players without ofVideoPlayer via the base functions ).

obviously you wouldn't want to pass in an ofVideoPlayer to setPlayer ( which can happen now ).

I do think you are correct in your thinking, I am just worried about some people storing the pointers but yeah, it is probably fine to merge.

@dimitre
Copy link
Member Author

dimitre commented Sep 29, 2022

I can be responsible for reverting this if any issue arises in the near future.

I've tried to check out ofDSVideoPlayer to see if it has other base class in common, like ofBaseVideoDraws but didn't find in any place. is that a public addon?

Other info: binary code size without inheritance is 86% of the object size, so less compile time and memory

-rw-r--r--  1 z  staff  38368 Sep 29 19:56 ofVideoPlayer1.o
-rw-r--r--  1 z  staff  44752 Sep 29 19:57 ofVideoPlayer2.o

@ofTheo ofTheo merged commit 291e64d into openframeworks:master Sep 30, 2022
@dimitre dimitre deleted the patch-1 branch September 30, 2022 17:06
@artificiel artificiel mentioned this pull request Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants